Ban absl::FixedArray, and instead provide base::FixedArray.

This is just a type wrapper, but it modifies a constructor that
otherwise would allow UB.

Bug: none
Change-Id: I368deca0af8b51afa0b206bc420576982dee7313
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4895257
Auto-Submit: Peter Kasting <pkasting@chromium.org>
Reviewed-by: Danil Chapovalov <danilchap@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Commit-Queue: Daniel Cheng <dcheng@chromium.org>
Commit-Queue: Peter Kasting <pkasting@chromium.org>
Code-Coverage: findit-for-me@appspot.gserviceaccount.com <findit-for-me@appspot.gserviceaccount.com>
Cr-Commit-Position: refs/heads/main@{#1203043}

PRESUBMIT.py

@@ -916,6 +916,19 @@ class BanRule:
       True,
       [_THIRD_PARTY_EXCEPT_BLINK],  # Not an error in third_party folders.
     ),
+    BanRule(
+      r'/\babsl::FixedArray\b',
+      (
+        'absl::FixedArray is banned. Use base::FixedArray instead.',
+      ),
+      True,
+      [
+        # base::FixedArray provides canonical access.
+        r'^base/types/fixed_array.h',
+        # Not an error in third_party folders.
+        _THIRD_PARTY_EXCEPT_BLINK,
+      ],
+    ),
     BanRule(
       r'/\babsl::FunctionRef\b',
       (

base/BUILD.gn

@@ -917,6 +917,7 @@ component("base") {
     "types/expected.h",
     "types/expected_internal.h",
     "types/expected_macros.h",
+    "types/fixed_array.h",
     "types/id_type.h",
     "types/optional_ref.h",
     "types/optional_util.h",
@@ -3426,6 +3427,7 @@ test("base_unittests") {
     "types/cxx23_to_underlying_unittest.cc",
     "types/expected_macros_unittest.cc",
     "types/expected_unittest.cc",
+    "types/fixed_array_unittest.cc",
     "types/id_type_unittest.cc",
     "types/optional_ref_unittest.cc",
     "types/optional_unittest.cc",

base/types/DEPS

@@ -0,0 +1,6 @@
+specific_include_rules = {
+  # Provides the canonical access point for this type
+  "fixed_array.h": [
+    "+third_party/abseil-cpp/absl/container/fixed_array.h",
+  ],
+}

base/types/fixed_array.h

@@ -0,0 +1,50 @@
+// Copyright 2023 The Chromium Authors
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef BASE_TYPES_FIXED_ARRAY_H_
+#define BASE_TYPES_FIXED_ARRAY_H_
+
+#include <stddef.h>
+
+#include <memory>
+#include <type_traits>
+
+#include "third_party/abseil-cpp/absl/container/fixed_array.h"
+
+namespace base {
+
+// `FixedArray` provides `absl::FixedArray` in Chromium, but when `T` is
+// trivially-default-constructible, forces the no-default-value constructor to
+// initialize the elements to `T()`, instead of leaving them uninitialized. This
+// makes `base::FixedArray` behave like `std::vector` instead of `std::array`
+// and avoids the risk of UB.
+
+// Trivially-default-constructible case: no-value constructor should init
+template <typename T,
+          size_t N = absl::kFixedArrayUseDefault,
+          typename A = std::allocator<T>,
+          typename = void>
+class FixedArray : public absl::FixedArray<T, N, A> {
+ public:
+  using absl::FixedArray<T, N, A>::FixedArray;
+  explicit FixedArray(absl::FixedArray<T, N, A>::size_type n,
+                      const absl::FixedArray<T, N, A>::allocator_type& a =
+                          typename absl::FixedArray<T, N, A>::allocator_type())
+      : FixedArray(n, T(), a) {}
+};
+
+// Non-trivially-default-constructible case: Pass through all constructors
+template <typename T, size_t N, typename A>
+struct FixedArray<
+    T,
+    N,
+    A,
+    std::enable_if_t<!std::is_trivially_default_constructible_v<T>>>
+    : public absl::FixedArray<T, N, A> {
+  using absl::FixedArray<T, N, A>::FixedArray;
+};
+
+}  // namespace base
+
+#endif  // BASE_TYPES_FIXED_ARRAY_H_

base/types/fixed_array_unittest.cc

@@ -0,0 +1,42 @@
+// Copyright 2023 The Chromium Authors
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "base/types/fixed_array.h"
+
+#include <stddef.h>
+
+#include <cstring>
+#include <memory>
+#include <type_traits>
+
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace base {
+namespace {
+
+TEST(FixedArrayTest, TriviallyDefaultConstructibleInitializes) {
+  using T = int;
+  static_assert(std::is_trivially_default_constructible_v<T>);
+  using Array = FixedArray<T, 1>;
+
+  // First try an array on the stack.
+  Array stack_array(1);
+  // This read and the one below are UB if `FixedArray` does not initialize the
+  // elements, but hopefully even if the compiler chooses to zero memory anyway,
+  // the test will fail under the memory sanitizer.
+  EXPECT_EQ(0, stack_array[0]);
+
+  // Now try an array on the heap, where we've purposefully written a non-zero
+  // bitpattern in hopes of increasing the chance of catching incorrect
+  // behavior.
+  constexpr size_t kSize = sizeof(Array);
+  alignas(Array) char storage[kSize];
+  std::memset(storage, 0xAA, kSize);
+  Array* placement_new_array = new (storage) Array(1);
+  EXPECT_EQ(0, (*placement_new_array)[0]);
+  placement_new_array->~Array();
+}
+
+}  // namespace
+}  // namespace base

styleguide/c++/c++-features.md

@@ -1624,6 +1624,24 @@ Banned due to overlap with `base/ranges/algorithm.h`. Use the `base/ranges/`
 facilities instead.
 ***
 
+### FixedArray <sup>[banned]</sup>
+
+```c++
+absl::FixedArray<MyObj> objs_;
+```
+
+**Description:** A fixed size array like `std::array`, but with size determined
+at runtime instead of compile time.
+
+**Documentation:**
+[fixed_array.h](https://source.chromium.org/chromium/chromium/src/+/main:third_party/abseil-cpp/absl/container/fixed_array.h)
+
+**Notes:**
+*** promo
+Direct construction is banned due to the risk of UB with uninitialized
+trivially-default-constructible types. Instead use `base/types/fixed_array.h`,
+which is a light-weight wrapper that deletes the problematic constructor.
+
 ### FunctionRef <sup>[banned]</sup>
 
 ```c++
@@ -1846,7 +1864,6 @@ absl::btree_map
 absl::btree_set
 absl::btree_multimap
 absl::btree_multiset
-absl::FixedArray
 ```
 
 **Description:** Alternatives to STL containers designed to be more efficient