Enable explicit conversion checks for base::span sizes.

This CL adds compile-time checks that the size parameter passed
to base::span either by the constructor or the base::make_span
function will fit into a size_t type and are not unsigned.

This is achieved by use of base::StrictNumeric parameter to the
methods and constructor.

This has no runtime overhead, as the checks are done at compile
time.

This CL also contains all the remaining mechanical changes needed
to convert signed numeric literal values to unsigned by adding
a 'u' on the end. Any functional or more complex changes have
already landed in other CLs tagged to this bug with separate
owner review.

This CL also adds no-compile tests for the new APIs.

BUG=1385166

Change-Id: Ic2dbd950842dd811a8dc40ac539c8b824ed983a1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4219636
Code-Coverage: Findit <findit-for-me@appspot.gserviceaccount.com>
Owners-Override: danakj <danakj@chromium.org>
Reviewed-by: danakj <danakj@chromium.org>
Commit-Queue: danakj <danakj@chromium.org>
Commit-Queue: Will Harris <wfh@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1102320}

base/containers/span.h

@@ -18,6 +18,7 @@
 #include "base/containers/checked_iterators.h"
 #include "base/containers/contiguous_iterator.h"
 #include "base/cxx20_to_address.h"
+#include "base/numerics/safe_math.h"
 
 namespace base {
 
@@ -256,16 +257,16 @@ class GSL_POINTER span : public internal::ExtentStorage<Extent> {
 
   template <typename It,
             typename = internal::EnableIfCompatibleContiguousIterator<It, T>>
-  constexpr span(It first, size_t count) noexcept
+  constexpr span(It first, StrictNumeric<size_t> count) noexcept
       : ExtentStorage(count),
         // The use of to_address() here is to handle the case where the iterator
         // `first` is pointing to the container's `end()`. In that case we can
         // not use the address returned from the iterator, or dereference it
-        // through the iterator's `operator*`, but we can store it. We must assume
-        // in this case that `count` is 0, since the iterator does not point to
-        // valid data. Future hardening of iterators may disallow pulling the
-        // address from `end()`, as demonstrated by asserts() in libstdc++:
-        // https://gcc.gnu.org/bugzilla/show_bug.cgi?id=93960.
+        // through the iterator's `operator*`, but we can store it. We must
+        // assume in this case that `count` is 0, since the iterator does not
+        // point to valid data. Future hardening of iterators may disallow
+        // pulling the address from `end()`, as demonstrated by asserts() in
+        // libstdc++: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=93960.
         //
         // The span API dictates that the `data()` is accessible when size is 0,
         // since the pointer may be valid, so we cannot prevent storing and
@@ -473,7 +474,7 @@ as_writable_bytes(span<T, X> s) noexcept {
 
 // Type-deducing helpers for constructing a span.
 template <int&... ExplicitArgumentBarrier, typename It>
-constexpr auto make_span(It it, size_t size) noexcept {
+constexpr auto make_span(It it, StrictNumeric<size_t> size) noexcept {
   using T = std::remove_reference_t<iter_reference_t<It>>;
   return span<T>(it, size);
 }
@@ -508,7 +509,7 @@ constexpr auto make_span(Container&& container) noexcept {
 //
 // Usage: auto static_span = base::make_span<N>(...);
 template <size_t N, int&... ExplicitArgumentBarrier, typename It>
-constexpr auto make_span(It it, size_t size) noexcept {
+constexpr auto make_span(It it, StrictNumeric<size_t> size) noexcept {
   using T = std::remove_reference_t<iter_reference_t<It>>;
   return span<T, N>(it, size);
 }

base/containers/span_unittest.cc

@@ -38,7 +38,7 @@ TEST(SpanTest, DefaultConstructor) {
 
 TEST(SpanTest, ConstructFromDataAndSize) {
   constexpr int* kNull = nullptr;
-  constexpr span<int> empty_span(kNull, 0);
+  constexpr span<int> empty_span(kNull, 0u);
   EXPECT_TRUE(empty_span.empty());
   EXPECT_EQ(nullptr, empty_span.data());
 
@@ -61,7 +61,7 @@ TEST(SpanTest, ConstructFromDataAndSize) {
 
 TEST(SpanTest, ConstructFromIterAndSize) {
   constexpr int* kNull = nullptr;
-  constexpr span<int> empty_span(kNull, 0);
+  constexpr span<int> empty_span(kNull, 0u);
   EXPECT_TRUE(empty_span.empty());
   EXPECT_EQ(nullptr, empty_span.data());
 
@@ -1111,7 +1111,7 @@ TEST(SpanTest, AsWritableBytes) {
 
 TEST(SpanTest, MakeSpanFromDataAndSize) {
   int* nullint = nullptr;
-  auto empty_span = make_span(nullint, 0);
+  auto empty_span = make_span(nullint, 0u);
   EXPECT_TRUE(empty_span.empty());
   EXPECT_EQ(nullptr, empty_span.data());
 
@@ -1349,6 +1349,12 @@ TEST(SpanTest, OutOfBoundsDeath) {
   ASSERT_DEATH_IF_SUPPORTED(kNonEmptyDynamicSpan[4], "");
   ASSERT_DEATH_IF_SUPPORTED(kNonEmptyDynamicSpan.subspan(10), "");
   ASSERT_DEATH_IF_SUPPORTED(kNonEmptyDynamicSpan.subspan(1, 7), "");
+
+  size_t minus_one = static_cast<size_t>(-1);
+  ASSERT_DEATH_IF_SUPPORTED(kNonEmptyDynamicSpan.subspan(minus_one), "");
+  ASSERT_DEATH_IF_SUPPORTED(kNonEmptyDynamicSpan.subspan(minus_one, minus_one),
+                            "");
+  ASSERT_DEATH_IF_SUPPORTED(kNonEmptyDynamicSpan.subspan(minus_one, 1), "");
 }
 
 TEST(SpanTest, IteratorIsRangeMoveSafe) {

base/containers/span_unittest.nc

@@ -25,34 +25,34 @@ class Derived : Base {
 
 // A default constructed span must have an extent of 0 or dynamic_extent.
 void WontCompile() {
-  span<int, 1> span;
+  span<int, 1u> span;
 }
 
-#elif defined(NCTEST_SPAN_FROM_ARRAY_WITH_NON_MATCHING_STATIC_EXTENT_DISALLOWED) // [r"fatal error: no matching constructor for initialization of 'span<int, 1>'"]
+#elif defined(NCTEST_SPAN_FROM_ARRAY_WITH_NON_MATCHING_STATIC_EXTENT_DISALLOWED) // [r"fatal error: no matching constructor for initialization of 'span<int, 1U>'"]
 
 // A span with static extent constructed from an array must match the size of
 // the array.
 void WontCompile() {
   int array[] = {1, 2, 3};
-  span<int, 1> span(array);
+  span<int, 1u> span(array);
 }
 
-#elif defined(NCTEST_SPAN_FROM_OTHER_SPAN_WITH_MISMATCHING_EXTENT_DISALLOWED) // [r"fatal error: no matching constructor for initialization of 'span<int, 4>'"]
+#elif defined(NCTEST_SPAN_FROM_OTHER_SPAN_WITH_MISMATCHING_EXTENT_DISALLOWED) // [r"fatal error: no matching constructor for initialization of 'span<int, 4U>'"]
 
 // A span with static extent constructed from another span must match the
 // extent.
 void WontCompile() {
   std::array<int, 3> array = {1, 2, 3};
-  span<int, 3> span3(array);
-  span<int, 4> span4(span3);
+  span<int, 3u> span3(array);
+  span<int, 4u> span4(span3);
 }
 
-#elif defined(NCTEST_DYNAMIC_SPAN_TO_STATIC_SPAN_DISALLOWED)  // [r"fatal error: no matching constructor for initialization of 'span<int, 3>'"]
+#elif defined(NCTEST_DYNAMIC_SPAN_TO_STATIC_SPAN_DISALLOWED)  // [r"fatal error: no matching constructor for initialization of 'span<int, 3U>'"]
 
 // Converting a dynamic span to a static span should not be allowed.
 void WontCompile() {
   span<int> dynamic_span;
-  span<int, 3> static_span(dynamic_span);
+  span<int, 3u> static_span(dynamic_span);
 }
 
 #elif defined(NCTEST_DERIVED_TO_BASE_CONVERSION_DISALLOWED)  // [r"fatal error: no matching constructor for initialization of 'span<Base \*>'"]
@@ -86,28 +86,28 @@ void WontCompile() {
   span<int> span(v);
 }
 
-#elif defined(NCTEST_IMPLICIT_CONVERSION_FROM_DYNAMIC_CONST_CONTAINER_TO_STATIC_SPAN_DISALLOWED) // [r"fatal error: no viable conversion from 'const std::vector<int>' to 'span<const int, 3>'"]
+#elif defined(NCTEST_IMPLICIT_CONVERSION_FROM_DYNAMIC_CONST_CONTAINER_TO_STATIC_SPAN_DISALLOWED) // [r"fatal error: no viable conversion from 'const std::vector<int>' to 'span<const int, 3U>'"]
 
 // A dynamic const container should not be implicitly convertible to a static span.
 void WontCompile() {
   const std::vector<int> v = {1, 2, 3};
-  span<const int, 3> span = v;
+  span<const int, 3u> span = v;
 }
 
-#elif defined(NCTEST_IMPLICIT_CONVERSION_FROM_DYNAMIC_MUTABLE_CONTAINER_TO_STATIC_SPAN_DISALLOWED) // [r"fatal error: no viable conversion from 'std::vector<int>' to 'span<int, 3>'"]
+#elif defined(NCTEST_IMPLICIT_CONVERSION_FROM_DYNAMIC_MUTABLE_CONTAINER_TO_STATIC_SPAN_DISALLOWED) // [r"fatal error: no viable conversion from 'std::vector<int>' to 'span<int, 3U>'"]
 
 // A dynamic mutable container should not be implicitly convertible to a static span.
 void WontCompile() {
   std::vector<int> v = {1, 2, 3};
-  span<int, 3> span = v;
+  span<int, 3u> span = v;
 }
 
 #elif defined(NCTEST_STD_SET_ITER_SIZE_CONVERSION_DISALLOWED)  // [r"fatal error: no matching constructor for initialization of 'span<int>'"]
 
 // A std::set() should not satisfy the requirements for conversion to a span.
 void WontCompile() {
   std::set<int> set;
-  span<int> span(set.begin(), 0);
+  span<int> span(set.begin(), 0u);
 }
 
 #elif defined(NCTEST_STD_SET_ITER_ITER_CONVERSION_DISALLOWED)  // [r"fatal error: no matching constructor for initialization of 'span<int>'"]
@@ -131,7 +131,7 @@ void WontCompile() {
 // Static first called on a span with static extent must not exceed the size.
 void WontCompile() {
   std::array<int, 3> array = {1, 2, 3};
-  span<int, 3> span(array);
+  span<int, 3u> span(array);
   auto first = span.first<4>();
 }
 
@@ -140,7 +140,7 @@ void WontCompile() {
 // Static last called on a span with static extent must not exceed the size.
 void WontCompile() {
   std::array<int, 3> array = {1, 2, 3};
-  span<int, 3> span(array);
+  span<int, 3u> span(array);
   auto last = span.last<4>();
 }
 
@@ -149,7 +149,7 @@ void WontCompile() {
 // Static subspan called on a span with static extent must not exceed the size.
 void WontCompile() {
   std::array<int, 3> array = {1, 2, 3};
-  span<int, 3> span(array);
+  span<int, 3u> span(array);
   auto subspan = span.subspan<4>();
 }
 
@@ -158,7 +158,7 @@ void WontCompile() {
 // Static subspan called on a span with static extent must not exceed the size.
 void WontCompile() {
   std::array<int, 3> array = {1, 2, 3};
-  span<int, 3> span(array);
+  span<int, 3u> span(array);
   auto subspan = span.subspan<0, 4>();
 }
 
@@ -174,15 +174,15 @@ void WontCompile() {
 
 // Front called on an empty span with static extent is not allowed.
 void WontCompile() {
-  span<int, 0> s;
+  span<int, 0u> s;
   s.front();
 }
 
 #elif defined(NCTEST_BACK_ON_EMPTY_STATIC_SPAN_DISALLOWED) // [r"Extent must not be 0"]
 
 // Back called on an empty span with static extent is not allowed.
 void WontCompile() {
-  span<int, 0> s;
+  span<int, 0u> s;
   s.back();
 }
 
@@ -191,7 +191,7 @@ void WontCompile() {
 // Calling swap on spans with different extents is not allowed.
 void WontCompile() {
   std::array<int, 3> array = {1, 2, 3};
-  span<int, 3> static_span(array);
+  span<int, 3u> static_span(array);
   span<int> dynamic_span(array);
   std::swap(static_span, dynamic_span);
 }
@@ -234,7 +234,7 @@ int WontCompile() {
 int WontCompile() {
   constexpr StringPiece str = "Foo";
   // Intentional extent mismatch causing CHECK failure.
-  constexpr auto made_span = make_span<2>(str.begin(), 3);
+  constexpr auto made_span = make_span<2>(str.begin(), 3u);
 }
 
 #elif defined(NCTEST_STATIC_MAKE_SPAN_FROM_ITERS_CHECKS_SIZE)  // [r"constexpr variable 'made_span' must be initialized by a constant expression"]
@@ -270,7 +270,7 @@ void WontCompile() {
 #elif defined(NCTEST_DANGLING_STD_ARRAY)  // [r"object backing the pointer will be destroyed at the end of the full-expression"]
 
 void WontCompile() {
-  span<const int, 3> s{std::array<int, 3>()};
+  span<const int, 3u> s{std::array<int, 3>()};
   (void)s;
 }
 
@@ -281,6 +281,24 @@ void WontCompile() {
   (void)s;
 }
 
+#elif defined(NCTEST_MAKE_SPAN_NOT_SIZE_T_SIZE)  // [r"The source type is out of range for the destination type"]
+
+void WontCompile() {
+  int length = -1;
+  std::vector<int> vector = {1, 2, 3};
+  auto s = make_span(vector.data(), length);
+  (void)s;
+}
+
+#elif defined(NCTEST_SPAN_NOT_SIZE_T_SIZE)  // [r"The source type is out of range for the destination type"]
+
+void WontCompile() {
+  int length = -1;
+  std::vector<int> vector = {1, 2, 3};
+  span<int> s(vector.data(), length);
+  (void)s;
+}
+
 #endif
 
 }  // namespace base

base/files/file_descriptor_watcher_posix_unittest.cc

@@ -127,7 +127,7 @@ class FileDescriptorWatcherTest
   void WriteByte() {
     constexpr char kByte = '!';
     ASSERT_TRUE(WriteFileDescriptor(write_file_descriptor(),
-                                    as_bytes(make_span(&kByte, 1))));
+                                    as_bytes(make_span(&kByte, 1u))));
   }
 
   void ReadByte() {

base/json/json_writer_unittest.cc

@@ -116,7 +116,7 @@ TEST(JSONWriterTest, BinaryValues) {
   // Binary values should return errors unless suppressed via the
   // OPTIONS_OMIT_BINARY_VALUES flag.
   const auto kBufferSpan =
-      base::make_span(reinterpret_cast<const uint8_t*>("asdf"), 4);
+      base::make_span(reinterpret_cast<const uint8_t*>("asdf"), 4u);
   Value root(kBufferSpan);
   EXPECT_FALSE(JSONWriter::Write(root, &output_js));
   EXPECT_TRUE(JSONWriter::WriteWithOptions(

base/task/thread_pool/thread_pool_impl_unittest.cc

@@ -962,7 +962,7 @@ TEST_P(ThreadPoolImplTest, FileDescriptorWatcherNoOpsAfterShutdown) {
   thread_pool_->Shutdown();
 
   constexpr char kByte = '!';
-  ASSERT_TRUE(WriteFileDescriptor(pipes[1], as_bytes(make_span(&kByte, 1))));
+  ASSERT_TRUE(WriteFileDescriptor(pipes[1], as_bytes(make_span(&kByte, 1u))));
 
   // Give a chance for the file watcher to fire before closing the handles.
   PlatformThread::Sleep(TestTimeouts::tiny_timeout());

base/win/scoped_handle_verifier.h

@@ -23,7 +23,7 @@ namespace internal {
 
 struct HandleHash {
   size_t operator()(const HANDLE& handle) const {
-    return base::FastHash(as_bytes(make_span(&handle, 1)));
+    return base::FastHash(as_bytes(make_span(&handle, 1u)));
   }
 };
 

chrome/browser/ash/policy/reporting/install_event_log.h

@@ -89,7 +89,7 @@ InstallEventLog<T, C>::InstallEventLog(const base::FilePath& file_name)
 
   int64_t version;
   if (!file.ReadAtCurrentPosAndCheck(
-          base::as_writable_bytes(base::make_span(&version, 1)))) {
+          base::as_writable_bytes(base::make_span(&version, 1u)))) {
     LOG(WARNING) << "Corrupted install log.";
     return;
   }
@@ -101,7 +101,7 @@ InstallEventLog<T, C>::InstallEventLog(const base::FilePath& file_name)
 
   ssize_t entries;
   if (!file.ReadAtCurrentPosAndCheck(
-          base::as_writable_bytes(base::make_span(&entries, 1)))) {
+          base::as_writable_bytes(base::make_span(&entries, 1u)))) {
     LOG(WARNING) << "Corrupted install log.";
     return;
   }
@@ -164,14 +164,14 @@ void InstallEventLog<T, C>::Store() {
   }
 
   if (!file.WriteAtCurrentPosAndCheck(
-          base::as_bytes(base::make_span(&kLogFileVersion, 1)))) {
+          base::as_bytes(base::make_span(&kLogFileVersion, 1u)))) {
     LOG(WARNING) << "Unable to store install log.";
     return;
   }
 
   ssize_t entries = logs_.size();
   if (!file.WriteAtCurrentPosAndCheck(
-          base::as_bytes(base::make_span(&entries, 1)))) {
+          base::as_bytes(base::make_span(&entries, 1u)))) {
     LOG(WARNING) << "Unable to store install log.";
     return;
   }

chrome/browser/ui/tabs/tab_strip_model.cc

@@ -699,7 +699,7 @@ void TabStripModel::CloseAllTabsInGroup(const tab_groups::TabGroupId& group) {
 bool TabStripModel::CloseWebContentsAt(int index, uint32_t close_types) {
   CHECK(ContainsIndex(index));
   WebContents* contents = GetWebContentsAt(index);
-  return CloseTabs(base::span<WebContents* const>(&contents, 1), close_types);
+  return CloseTabs(base::span<WebContents* const>(&contents, 1u), close_types);
 }
 
 bool TabStripModel::TabsAreLoading() const {

chrome/renderer/cart/commerce_hint_agent.cc

@@ -844,7 +844,7 @@ void CommerceHintAgent::ExtractCartWithUpdatedScript(
       GetProductExtractionScript(product_id_json, cart_extraction_script));
 
   main_frame->RequestExecuteScript(
-      ISOLATED_WORLD_ID_CHROME_INTERNAL, base::make_span(&source, 1),
+      ISOLATED_WORLD_ID_CHROME_INTERNAL, base::make_span(&source, 1u),
       blink::mojom::UserActivationOption::kDoNotActivate,
       blink::mojom::EvaluationTiming::kAsynchronous,
       blink::mojom::LoadEventBlockingOption::kDoNotBlock,

chromecast/media/cma/backend/alsa/scoped_alsa_mixer_unittest.cc

@@ -78,7 +78,7 @@ class ScopedAlsaMixerEventTest : public ::testing::Test {
   void WriteByte() {
     constexpr char kByte = '!';
     ASSERT_TRUE(base::WriteFileDescriptor(
-        pipe_fds_[1], as_bytes(base::make_span(&kByte, 1))));
+        pipe_fds_[1], as_bytes(base::make_span(&kByte, 1u))));
   }
 
   base::test::TaskEnvironment task_environment_;

chromeos/ash/components/dbus/session_manager/session_manager_client.cc

@@ -114,7 +114,7 @@ base::ScopedFD CreatePasswordPipe(const std::string& data) {
   const size_t data_size = data.size();
 
   base::WriteFileDescriptor(pipe_write_end.get(),
-                            base::as_bytes(base::make_span(&data_size, 1)));
+                            base::as_bytes(base::make_span(&data_size, 1u)));
   base::WriteFileDescriptor(pipe_write_end.get(), data);
 
   return pipe_read_end;

components/autofill/core/browser/personal_data_manager.cc

@@ -1300,7 +1300,7 @@ gfx::Image* PersonalDataManager::GetCreditCardArtImageForUrl(
   if (cached_image)
     return cached_image;
 
-  FetchImagesForURLs(base::make_span(&card_art_url, 1));
+  FetchImagesForURLs(base::make_span(&card_art_url, 1u));
   return nullptr;
 }
 

components/metrics/serialization/serialization_utils.cc

@@ -220,7 +220,7 @@ bool SerializationUtils::WriteMetricToFile(const MetricSample& sample,
   uint32_t encoded_size = base::checked_cast<uint32_t>(size);
   if (!base::WriteFileDescriptor(
           file_descriptor.get(),
-          base::as_bytes(base::make_span(&encoded_size, 1)))) {
+          base::as_bytes(base::make_span(&encoded_size, 1u)))) {
     DPLOG(ERROR) << "error writing message length: " << filename;
     std::ignore = flock(file_descriptor.get(), LOCK_UN);
     return false;