[libc++][hardening] Add a bounds check for `valarray` and `bitset`. #120685

var-const · 2024-12-20T06:30:54Z

Add a valid-element-access check to valarray::operator[] and
bitset::operator[].

Add a `valid-element-access` check to `valarray::operator[]` and `bitset::operator[]`.

llvmbot · 2024-12-20T06:31:27Z

@llvm/pr-subscribers-libcxx

Author: Konstantin Varlamov (var-const)

Changes

Add a valid-element-access check to valarray::operator[] and
bitset::operator[].

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

5 Files Affected:

(modified) libcxx/include/bitset (+7-1)
(modified) libcxx/include/valarray (+8-2)
(added) libcxx/test/libcxx/numerics/numarray/assert.pass.cpp (+42)
(added) libcxx/test/libcxx/utilities/template.bitset/assert.abi-v1.pass.cpp (+43)
(added) libcxx/test/libcxx/utilities/template.bitset/assert.abi-v2.pass.cpp (+45)

diff --git a/libcxx/include/bitset b/libcxx/include/bitset
index e2dc7b459f1c4a..b8c31d5ddd52fe 100644
--- a/libcxx/include/bitset
+++ b/libcxx/include/bitset
@@ -133,6 +133,7 @@ template <size_t N> struct hash<std::bitset<N>>;
 #  include <__algorithm/fill_n.h>
 #  include <__algorithm/find.h>
 #  include <__bit_reference>
+#  include <__assert>
 #  include <__config>
 #  include <__functional/hash.h>
 #  include <__functional/unary_function.h>
@@ -682,13 +683,18 @@ public:
 
   // element access:
 #  ifdef _LIBCPP_ABI_BITSET_VECTOR_BOOL_CONST_SUBSCRIPT_RETURN_BOOL
-  _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR bool operator[](size_t __p) const { return __base::__make_ref(__p); }
+  _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR bool operator[](size_t __p) const {
+    _LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(__p < _Size, "bitset::operator[] index out of bounds");
+    return __base::__make_ref(__p);
+  }
 #  else
   _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR const_reference operator[](size_t __p) const {
+    _LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(__p < _Size, "bitset::operator[] index out of bounds");
     return __base::__make_ref(__p);
   }
 #  endif
   _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX23 reference operator[](size_t __p) {
+    _LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(__p < _Size, "bitset::operator[] index out of bounds");
     return __base::__make_ref(__p);
   }
   _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX23 unsigned long to_ulong() const;
diff --git a/libcxx/include/valarray b/libcxx/include/valarray
index af916096b5ef9d..e86367dac929df 100644
--- a/libcxx/include/valarray
+++ b/libcxx/include/valarray
@@ -821,9 +821,15 @@ public:
   _LIBCPP_HIDE_FROM_ABI valarray& operator=(const __val_expr<_ValExpr>& __v);
 
   // element access:
-  _LIBCPP_HIDE_FROM_ABI const value_type& operator[](size_t __i) const { return __begin_[__i]; }
+  _LIBCPP_HIDE_FROM_ABI const value_type& operator[](size_t __i) const {
+    _LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(__i < size(), "valarray::operator[] index out of bounds");
+    return __begin_[__i];
+  }
 
-  _LIBCPP_HIDE_FROM_ABI value_type& operator[](size_t __i) { return __begin_[__i]; }
+  _LIBCPP_HIDE_FROM_ABI value_type& operator[](size_t __i) {
+    _LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(__i < size(), "valarray::operator[] index out of bounds");
+    return __begin_[__i];
+  }
 
   // subset operations:
   _LIBCPP_HIDE_FROM_ABI __val_expr<__slice_expr<const valarray&> > operator[](slice __s) const;
diff --git a/libcxx/test/libcxx/numerics/numarray/assert.pass.cpp b/libcxx/test/libcxx/numerics/numarray/assert.pass.cpp
new file mode 100644
index 00000000000000..2bdf52340abfc8
--- /dev/null
+++ b/libcxx/test/libcxx/numerics/numarray/assert.pass.cpp
@@ -0,0 +1,42 @@
+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+// <valarray>
+
+// Test hardening assertions for std::valarray.
+
+// REQUIRES: has-unix-headers
+// UNSUPPORTED: libcpp-hardening-mode=none
+// UNSUPPORTED: c++03
+// XFAIL: libcpp-hardening-mode=debug && availability-verbose_abort-missing
+
+#include <valarray>
+
+#include "check_assertion.h"
+
+int main(int, char**) {
+  { // Empty valarray
+    std::valarray<int> c;
+    const auto& const_c = c;
+    TEST_LIBCPP_ASSERT_FAILURE(c[0], "valarray::operator[] index out of bounds");
+    TEST_LIBCPP_ASSERT_FAILURE(const_c[0], "valarray::operator[] index out of bounds");
+    TEST_LIBCPP_ASSERT_FAILURE(c[42], "valarray::operator[] index out of bounds");
+    TEST_LIBCPP_ASSERT_FAILURE(const_c[42], "valarray::operator[] index out of bounds");
+  }
+
+  { // Non-empty valarray
+    std::valarray<int> c(4);
+    const auto& const_c = c;
+    (void)c[3]; // Check that there's no assertion on valid access.
+    TEST_LIBCPP_ASSERT_FAILURE(c[4], "valarray::operator[] index out of bounds");
+    (void)const_c[3]; // Check that there's no assertion on valid access.
+    TEST_LIBCPP_ASSERT_FAILURE(const_c[4], "valarray::operator[] index out of bounds");
+  }
+
+  return 0;
+}
diff --git a/libcxx/test/libcxx/utilities/template.bitset/assert.abi-v1.pass.cpp b/libcxx/test/libcxx/utilities/template.bitset/assert.abi-v1.pass.cpp
new file mode 100644
index 00000000000000..0abb94954ba554
--- /dev/null
+++ b/libcxx/test/libcxx/utilities/template.bitset/assert.abi-v1.pass.cpp
@@ -0,0 +1,43 @@
+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+// <bitset>
+
+// Test hardening assertions for std::bitset using ABI v1 (where the const overload of `operator[]` returns
+// `const_reference` which is non-Standard behavior).
+
+// REQUIRES: has-unix-headers
+// UNSUPPORTED: libcpp-hardening-mode=none
+// UNSUPPORTED: c++03
+// XFAIL: libcpp-hardening-mode=debug && availability-verbose_abort-missing
+
+#include <bitset>
+
+#include "check_assertion.h"
+
+int main(int, char**) {
+  { // Empty bitset
+    std::bitset<0> c;
+    const auto& const_c = c;
+    TEST_LIBCPP_ASSERT_FAILURE(c[0], "bitset::operator[] index out of bounds");
+    TEST_LIBCPP_ASSERT_FAILURE(const_c[0], "bitset::operator[] index out of bounds");
+    TEST_LIBCPP_ASSERT_FAILURE(c[42], "bitset::operator[] index out of bounds");
+    TEST_LIBCPP_ASSERT_FAILURE(const_c[42], "bitset::operator[] index out of bounds");
+  }
+
+  { // Non-empty bitset
+    std::bitset<4> c(42);
+    const auto& const_c = c;
+    (void)c[3]; // Check that there's no assertion on valid access.
+    TEST_LIBCPP_ASSERT_FAILURE(c[4], "bitset::operator[] index out of bounds");
+    (void)const_c[3]; // Check that there's no assertion on valid access.
+    TEST_LIBCPP_ASSERT_FAILURE(const_c[4], "bitset::operator[] index out of bounds");
+  }
+
+  return 0;
+}
diff --git a/libcxx/test/libcxx/utilities/template.bitset/assert.abi-v2.pass.cpp b/libcxx/test/libcxx/utilities/template.bitset/assert.abi-v2.pass.cpp
new file mode 100644
index 00000000000000..783980118207f7
--- /dev/null
+++ b/libcxx/test/libcxx/utilities/template.bitset/assert.abi-v2.pass.cpp
@@ -0,0 +1,45 @@
+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+// <bitset>
+
+// Test hardening assertions for std::bitset using ABI >= v2 (where the const overload of `operator[]` returns `bool` as
+// mandated by the Standard).
+
+// REQUIRES: has-unix-headers
+// UNSUPPORTED: libcpp-hardening-mode=none
+// UNSUPPORTED: c++03
+// XFAIL: libcpp-hardening-mode=debug && availability-verbose_abort-missing
+
+// ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_ABI_BITSET_VECTOR_BOOL_CONST_SUBSCRIPT_RETURN_BOOL=1
+
+#include <bitset>
+
+#include "check_assertion.h"
+
+int main(int, char**) {
+  { // Empty bitset
+    std::bitset<0> c;
+    const auto& const_c = c;
+    TEST_LIBCPP_ASSERT_FAILURE(c[0], "bitset::operator[] index out of bounds");
+    TEST_LIBCPP_ASSERT_FAILURE(const_c[0], "bitset::operator[] index out of bounds");
+    TEST_LIBCPP_ASSERT_FAILURE(c[42], "bitset::operator[] index out of bounds");
+    TEST_LIBCPP_ASSERT_FAILURE(const_c[42], "bitset::operator[] index out of bounds");
+  }
+
+  { // Non-empty bitset
+    std::bitset<4> c(42);
+    const auto& const_c = c;
+    (void)c[3]; // Check that there's no assertion on valid access.
+    TEST_LIBCPP_ASSERT_FAILURE(c[4], "bitset::operator[] index out of bounds");
+    (void)const_c[3]; // Check that there's no assertion on valid access.
+    TEST_LIBCPP_ASSERT_FAILURE(const_c[4], "bitset::operator[] index out of bounds");
+  }
+
+  return 0;
+}

var-const · 2024-12-20T06:32:51Z

libcxx/test/libcxx/numerics/numarray/assert.pass.cpp

@@ -0,0 +1,42 @@
+//===----------------------------------------------------------------------===//


Note: I found it easier to just test all the assertions in a single file (we have precedent in deque). We could split by hardening mode later, but I think having a separate test file per hardened function is more trouble than it's worth. No strong feelings, though -- please feel free to push back on this.

I think this looks reasonable to me. I don't know that we want to elevate this choice into a policy, but I certainly won't push back on the way you've done it here.

var-const · 2024-12-20T06:33:22Z

libcxx/test/libcxx/utilities/template.bitset/assert.abi-v1.pass.cpp

+
+// <bitset>
+
+// Test hardening assertions for std::bitset using ABI v1 (where the const overload of `operator[]` returns


Please let me know if you have any thoughts on how to handle this difference better.

I think the way we'd normally do it is:

int main() { #if _LIBCPP_ABI_BITSET_VECTOR_BOOL_CONST_SUBSCRIPT_RETURN_BOOL // test for this ABI #else // test for the normal ABI #endif }

That would be in a single test file. In practice, the second ABI setting is tested by our CI job that enables the ABI v2.

Enabling -D_LIBCPP_ABI_BITSET_VECTOR_BOOL_CONST_SUBSCRIPT_RETURN_BOOL=1 on the command-line is technically incorrect because we'd need to ensure that the dylib is also built with that configuration. In general, these ABI settings shouldn't be tweaked on a per-TU basis (just like we tell our users!). In practice it probably works because the dylib isn't affected by std::bitset's ABI, but we shouldn't rely on that.

github-actions · 2024-12-20T06:34:31Z

✅ With the latest revision this PR passed the C/C++ code formatter.

ldionne

Some comments. Thanks for adding those!

LGTM with the comments addressed.

ldionne · 2024-12-20T19:57:18Z

libcxx/test/libcxx/numerics/numarray/assert.pass.cpp

@@ -0,0 +1,42 @@
+//===----------------------------------------------------------------------===//


I think this looks reasonable to me. I don't know that we want to elevate this choice into a policy, but I certainly won't push back on the way you've done it here.

ldionne · 2024-12-20T19:59:42Z

libcxx/test/libcxx/utilities/template.bitset/assert.abi-v1.pass.cpp

+
+// <bitset>
+
+// Test hardening assertions for std::bitset using ABI v1 (where the const overload of `operator[]` returns


I think the way we'd normally do it is:

int main() { #if _LIBCPP_ABI_BITSET_VECTOR_BOOL_CONST_SUBSCRIPT_RETURN_BOOL // test for this ABI #else // test for the normal ABI #endif }

That would be in a single test file. In practice, the second ABI setting is tested by our CI job that enables the ABI v2.

Enabling -D_LIBCPP_ABI_BITSET_VECTOR_BOOL_CONST_SUBSCRIPT_RETURN_BOOL=1 on the command-line is technically incorrect because we'd need to ensure that the dylib is also built with that configuration. In general, these ABI settings shouldn't be tweaked on a per-TU basis (just like we tell our users!). In practice it probably works because the dylib isn't affected by std::bitset's ABI, but we shouldn't rely on that.

var-const

Thank you for the review!

In practice, the second ABI setting is tested by our CI job that enables the ABI v2.

Do we have a job that tests ABI v2 with hardening enabled? If not, I'm not sure it's worth adding for just this one PR, though, but happy to do so if necessary (ideally in a follow-up :) ).

var-const · 2024-12-24T19:03:38Z

libcxx/docs/Hardening.rst

@@ -458,7 +458,7 @@ Hardened containers status
      - Partial
      - N/A
    * - ``bitset``
-      - ❌
+      - ✅


I'm still hesitant to list valarray as fully hardened given the numerous helper classes -- that would require some investigation!

[libc++][hardening] Add a bounds check for valarray and bitset.

2511489

Add a `valid-element-access` check to `valarray::operator[]` and `bitset::operator[]`.

var-const requested a review from a team as a code owner December 20, 2024 06:30

llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Dec 20, 2024

var-const requested a review from ldionne December 20, 2024 06:33

var-const added the hardening Issues related to the hardening effort label Dec 20, 2024

var-const commented Dec 20, 2024

View reviewed changes

Fix include order

0f8bfd0

ldionne approved these changes Dec 20, 2024

View reviewed changes

Address feedback

61c48e5

var-const commented Dec 21, 2024

View reviewed changes

Update hardening status of bitset in the docs

cb10ad4

var-const commented Dec 24, 2024

View reviewed changes

var-const merged commit ac1d560 into llvm:main Dec 25, 2024
64 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[libc++][hardening] Add a bounds check for `valarray` and `bitset`. #120685

[libc++][hardening] Add a bounds check for `valarray` and `bitset`. #120685

Uh oh!

var-const commented Dec 20, 2024

Uh oh!

llvmbot commented Dec 20, 2024

Uh oh!

var-const Dec 20, 2024

Uh oh!

ldionne Dec 20, 2024

Uh oh!

var-const Dec 20, 2024

Uh oh!

ldionne Dec 20, 2024

Uh oh!

github-actions bot commented Dec 20, 2024 •

edited

Loading

Uh oh!

ldionne left a comment

Uh oh!

ldionne Dec 20, 2024

Uh oh!

ldionne Dec 20, 2024

Uh oh!

var-const left a comment

Uh oh!

var-const Dec 24, 2024

Uh oh!

Uh oh!

Uh oh!

		@@ -0,0 +1,42 @@
		//===----------------------------------------------------------------------===//


		// <bitset>

		// Test hardening assertions for std::bitset using ABI v1 (where the const overload of `operator[]` returns

[libc++][hardening] Add a bounds check for valarray and bitset. #120685

[libc++][hardening] Add a bounds check for valarray and bitset. #120685

Uh oh!

Conversation

var-const commented Dec 20, 2024

Uh oh!

llvmbot commented Dec 20, 2024

Uh oh!

var-const Dec 20, 2024

Choose a reason for hiding this comment

Uh oh!

ldionne Dec 20, 2024

Choose a reason for hiding this comment

Uh oh!

var-const Dec 20, 2024

Choose a reason for hiding this comment

Uh oh!

ldionne Dec 20, 2024

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Dec 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ldionne left a comment

Choose a reason for hiding this comment

Uh oh!

ldionne Dec 20, 2024

Choose a reason for hiding this comment

Uh oh!

ldionne Dec 20, 2024

Choose a reason for hiding this comment

Uh oh!

var-const left a comment

Choose a reason for hiding this comment

Uh oh!

var-const Dec 24, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

[libc++][hardening] Add a bounds check for `valarray` and `bitset`. #120685

[libc++][hardening] Add a bounds check for `valarray` and `bitset`. #120685

github-actions bot commented Dec 20, 2024 •

edited

Loading