Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Dec 25, 2024
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2511489
[libc++][hardening] Add a bounds check for `valarray` and `bitset`.
var-const Dec 20, 2024
0f8bfd0
Fix include order
var-const Dec 20, 2024
61c48e5
Address feedback
var-const Dec 21, 2024
cb10ad4
Update hardening status of bitset in the docs
var-const Dec 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion libcxx/include/bitset
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down Expand Up @@ -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;
Expand Down
10 changes: 8 additions & 2 deletions libcxx/include/valarray
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
42 changes: 42 additions & 0 deletions libcxx/test/libcxx/numerics/numarray/assert.pass.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
//===----------------------------------------------------------------------===//
Copy link
Member Author

@var-const var-const Dec 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@ldionne ldionne Dec 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

//
// 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;
}
43 changes: 43 additions & 0 deletions libcxx/test/libcxx/utilities/template.bitset/assert.abi-v1.pass.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
Copy link
Member Author

@var-const var-const Dec 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@ldionne ldionne Dec 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// `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;
}
45 changes: 45 additions & 0 deletions libcxx/test/libcxx/utilities/template.bitset/assert.abi-v2.pass.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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;
}
Loading