[libc++] Add missing assertion in std::span constructor

[ldionne]

The (iterator, size) constructor should ensure that it gets passed a valid range when the size is not 0.

Fixes #107789

[llvmbot]

@llvm/pr-subscribers-libcxx

Author: Louis Dionne (ldionne)

Changes

The (iterator, size) constructor should ensure that it gets passed a valid range when the size is not 0.

Fixes #107789

---

Full diff: https://github.com/llvm/llvm-project/pull/118396.diff

2 Files Affected:

• (modified) libcxx/include/span (+6-1)
• (modified) libcxx/test/libcxx/containers/views/views.span/span.cons/assert.iter_size.pass.cpp (+41-6)

diff --git a/libcxx/include/span b/libcxx/include/span
index 896a3cd890186c..24ccc9be778764 100644
--- a/libcxx/include/span
+++ b/libcxx/include/span
@@ -267,6 +267,8 @@ public:
   _LIBCPP_HIDE_FROM_ABI constexpr explicit span(_It __first, size_type __count) : __data_{std::to_address(__first)} {
     (void)__count;
     _LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(_Extent == __count, "size mismatch in span's constructor (iterator, len)");
+    _LIBCPP_ASSERT_VALID_INPUT_RANGE(__count == 0 ? true : std::to_address(__first) != nullptr,
+                                     "passed nullptr with non-zero length in span's constructor (iterator, len)");
   }
 
   template <__span_compatible_iterator<element_type> _It, __span_compatible_sentinel_for<_It> _End>
@@ -441,7 +443,10 @@ public:
 
   template <__span_compatible_iterator<element_type> _It>
   _LIBCPP_HIDE_FROM_ABI constexpr span(_It __first, size_type __count)
-      : __data_{std::to_address(__first)}, __size_{__count} {}
+      : __data_{std::to_address(__first)}, __size_{__count} {
+    _LIBCPP_ASSERT_VALID_INPUT_RANGE(__count == 0 ? true : std::to_address(__first) != nullptr,
+                                     "passed nullptr with non-zero length in span's constructor (iterator, len)");
+  }
 
   template <__span_compatible_iterator<element_type> _It, __span_compatible_sentinel_for<_It> _End>
   _LIBCPP_HIDE_FROM_ABI constexpr span(_It __first, _End __last)
diff --git a/libcxx/test/libcxx/containers/views/views.span/span.cons/assert.iter_size.pass.cpp b/libcxx/test/libcxx/containers/views/views.span/span.cons/assert.iter_size.pass.cpp
index 4461bad8ff5047..522bde13a5688c 100644
--- a/libcxx/test/libcxx/containers/views/views.span/span.cons/assert.iter_size.pass.cpp
+++ b/libcxx/test/libcxx/containers/views/views.span/span.cons/assert.iter_size.pass.cpp
@@ -25,13 +25,48 @@
 #include "check_assertion.h"
 
 int main(int, char**) {
-    std::array<int, 3> array{0, 1, 2};
+  std::array<int, 3> array{0, 1, 2};
 
-    auto too_large = [&] { std::span<int, 3> const s(array.data(), 4); (void)s; };
-    TEST_LIBCPP_ASSERT_FAILURE(too_large(), "size mismatch in span's constructor (iterator, len)");
+  // Providing too large value in constructor
+  {
+    auto f = [&] {
+      std::span<int, 3> const s(array.data(), 4);
+      (void)s;
+    };
+    TEST_LIBCPP_ASSERT_FAILURE(f(), "size mismatch in span's constructor (iterator, len)");
+  }
 
-    auto too_small = [&] { std::span<int, 3> const s(array.data(), 2); (void)s; };
-    TEST_LIBCPP_ASSERT_FAILURE(too_small(), "size mismatch in span's constructor (iterator, len)");
+  // Providing too small value in constructor
+  {
+    auto f = [&] {
+      std::span<int, 3> const s(array.data(), 2);
+      (void)s;
+    };
+    TEST_LIBCPP_ASSERT_FAILURE(f(), "size mismatch in span's constructor (iterator, len)");
+  }
 
-    return 0;
+  // Providing nullptr with a non-zero size in construction
+  {
+    // static extent
+    {
+      auto f = [&] {
+        int* p = nullptr;
+        std::span<int, 3> const s(p, 3);
+        (void)s;
+      };
+      TEST_LIBCPP_ASSERT_FAILURE(f(), "passed nullptr with non-zero length in span's constructor (iterator, len)");
+    }
+
+    // dynamic extent
+    {
+      auto f = [&] {
+        int* p = nullptr;
+        std::span<int, std::dynamic_extent> const s(p, 1);
+        (void)s;
+      };
+      TEST_LIBCPP_ASSERT_FAILURE(f(), "passed nullptr with non-zero length in span's constructor (iterator, len)");
+    }
+  }
+
+  return 0;
 }

[mxms0]

Just a drive by comment, but can this assert be some form of _LIBCPP_ASSERT_NON_NULL?

I'm not sure if passing a null-iterator here with a length has a lot of security impact (besides the general idea that it's UB and the compiler can do anything). It's possible I'm misunderstanding :)

[var-const]

Just a drive by comment, but can this assert be some form of _LIBCPP_ASSERT_NON_NULL?

I'm not sure if passing a null-iterator here with a length has a lot of security impact (besides the general idea that it's UB and the compiler can do anything). It's possible I'm misunderstanding :)

It won't necessarily lead to a null-pointer dereference (the invalid span can be called with a non-zero index later on) -- I'd keep the non-null category specifically for dereferencing null.

[mxms0]

It won't necessarily lead to a null-pointer dereference (the invalid span can be called with a non-zero index later on) -- I'd keep the non-null category specifically for dereferencing null.

No strong feelings about keeping the non-null category for strictly null. Can we put it into an assert category besides FAST? Indeed it won't be null, but most platforms the first page (or more) is unmappable. I'm not too strongly opinionated, but I am interested in keeping span cheap and keeping checks in _LIBCPP_HARDENING_MODE_FAST as security-related as possible.

[ldionne]

No strong feelings about keeping the non-null category for strictly null. Can we put it into an assert category besides FAST? Indeed it won't be null, but most platforms the first page (or more) is unmappable. I'm not too strongly opinionated, but I am interested in keeping span cheap and keeping checks in _LIBCPP_HARDENING_MODE_FAST as security-related as possible.

My rationale for making it _LIBCPP_ASSERT_VALID_INPUT_RANGE was that creating such a span would then allow you to access the region of memory [0, N) freely without complaint, and that seemed like something potentially dangerous. N can potentially be something large, so in theory you could end up getting a "blessed handle" to dereference an arbitrary address unless I missed something.

Thanks for bringing this up -- we also care strongly about keeping the fast mode fast and strictly for security issues, and these discussions are important and healthy to have. In light of the above, do you still feel like it should probably not be included in the fast mode?

[ldionne]

I will merge this once CI is green unless there is additional discussion within a few days.

[mxms0]

My rationale for making it _LIBCPP_ASSERT_VALID_INPUT_RANGE was that creating such a span would then allow you to access the region of memory [0, N) freely without complaint, and that seemed like something potentially dangerous. N can potentially be something large, so in theory you could end up getting a "blessed handle" to dereference an arbitrary address unless I missed something.

That's true, I can see a scenario where the user has some code like:

template <typename T>
std::span<T> allocate_elements(int num) {
    global_buf_ = malloc(num * sizeof(T));
    // num * sizeof(T) is large enough that malloc fails
    return std::span(global_buf_, num * sizeof(T));
    // we create a span here with (0, large_number);
}

Then the attacker would need to be able to index arbitrarily into the span without crashing, which isn't impossible.

We use absl::Span a lot and not std::span, so I don't have a great way currently to measure the overhead of this, but I suppose it's something we can easily re-visit once the time comes. I'll CC some folks here, but feel free to close out if we don't respond within your initial "few days" :)

[ldionne]

I am going to move ahead with this patch for now, but we're definitely willing to revisit based on new information.

[davidben]

While tripping this would let you access [0, N) without complaint, passing in a random other pointer and mismatched bounds would do the same thing, and std::span can't check that. I think, practically speaking, std::span(ptr, len) should be considered an unsafe function (in the sense of -Wunsafe-buffer-usage), with correctness being the caller's responsibility.

So I agree with @mxms0 that this doesn't really belong in _LIBCPP_HARDENING_MODE_FAST. Practically speaking, I expect the compiler would never be able to optimize this out in general user code, and security-wise, we already have to rely on the caller for bounds-safety of this function. (At least until -fbounds-safety gets added to C++, at which point perhaps those caller preconditions could be captured.)

[davidben]

(I ran into this because I was playing around with some ideas locally on how to capture some invariants in my project and was trying to diagnose an unexpected uncleared precondition.)