Skip to content

Commit cf9806e

Browse files
authored
[libc++] Fix UB in bitwise logic of {std, ranges}::{fill, fill_n} algorithms (#122410)
This PR addresses an undefined behavior that arises when using the `std::fill` and `std::fill_n` algorithms, as well as their ranges counterparts `ranges::fill` and `ranges::fill_n`, with `vector<bool, Alloc>` that utilizes a custom-sized allocator with small integral types.
1 parent f07cd36 commit cf9806e

File tree

9 files changed

+349
-118
lines changed

9 files changed

+349
-118
lines changed

libcxx/include/__algorithm/fill_n.h

Lines changed: 2 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -41,11 +41,7 @@ __fill_n_bool(__bit_iterator<_Cp, false> __first, typename __size_difference_typ
4141
if (__first.__ctz_ != 0) {
4242
__storage_type __clz_f = static_cast<__storage_type>(__bits_per_word - __first.__ctz_);
4343
__storage_type __dn = std::min(__clz_f, __n);
44-
__storage_type __m = (~__storage_type(0) << __first.__ctz_) & (~__storage_type(0) >> (__clz_f - __dn));
45-
if (_FillVal)
46-
*__first.__seg_ |= __m;
47-
else
48-
*__first.__seg_ &= ~__m;
44+
std::__fill_masked_range(std::__to_address(__first.__seg_), __clz_f - __dn, __first.__ctz_, _FillVal);
4945
__n -= __dn;
5046
++__first.__seg_;
5147
}
@@ -56,11 +52,7 @@ __fill_n_bool(__bit_iterator<_Cp, false> __first, typename __size_difference_typ
5652
// do last partial word
5753
if (__n > 0) {
5854
__first.__seg_ += __nw;
59-
__storage_type __m = ~__storage_type(0) >> (__bits_per_word - __n);
60-
if (_FillVal)
61-
*__first.__seg_ |= __m;
62-
else
63-
*__first.__seg_ &= ~__m;
55+
std::__fill_masked_range(std::__to_address(__first.__seg_), __bits_per_word - __n, 0u, _FillVal);
6456
}
6557
}
6658

libcxx/include/__bit_reference

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@
1414
#include <__algorithm/copy_backward.h>
1515
#include <__algorithm/copy_n.h>
1616
#include <__algorithm/min.h>
17+
#include <__assert>
1718
#include <__bit/countr.h>
1819
#include <__compare/ordering.h>
1920
#include <__config>
@@ -24,10 +25,13 @@
2425
#include <__memory/construct_at.h>
2526
#include <__memory/pointer_traits.h>
2627
#include <__type_traits/conditional.h>
28+
#include <__type_traits/enable_if.h>
2729
#include <__type_traits/is_constant_evaluated.h>
30+
#include <__type_traits/is_unsigned.h>
2831
#include <__type_traits/void_t.h>
2932
#include <__utility/pair.h>
3033
#include <__utility/swap.h>
34+
#include <climits>
3135

3236
#if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
3337
# pragma GCC system_header
@@ -58,6 +62,26 @@ struct __size_difference_type_traits<_Cp, __void_t<typename _Cp::difference_type
5862
using size_type = typename _Cp::size_type;
5963
};
6064

65+
// This function is designed to operate correctly even for smaller integral types like `uint8_t`, `uint16_t`,
66+
// or `unsigned short`. Casting back to _StorageType is crucial to prevent undefined behavior that can arise
67+
// from integral promotions.
68+
// See https://github.com/llvm/llvm-project/pull/122410.
69+
template <class _StoragePointer>
70+
_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX14 void
71+
__fill_masked_range(_StoragePointer __word, unsigned __clz, unsigned __ctz, bool __fill_val) {
72+
static_assert(is_unsigned<typename pointer_traits<_StoragePointer>::element_type>::value,
73+
"__fill_masked_range must be called with unsigned type");
74+
using _StorageType = typename pointer_traits<_StoragePointer>::element_type;
75+
_LIBCPP_ASSERT_VALID_INPUT_RANGE(
76+
__ctz + __clz < sizeof(_StorageType) * CHAR_BIT, "__fill_masked_range called with invalid range");
77+
_StorageType __m = static_cast<_StorageType>(static_cast<_StorageType>(~static_cast<_StorageType>(0)) >> __clz) &
78+
static_cast<_StorageType>(static_cast<_StorageType>(~static_cast<_StorageType>(0)) << __ctz);
79+
if (__fill_val)
80+
*__word |= __m;
81+
else
82+
*__word &= static_cast<_StorageType>(~__m);
83+
}
84+
6185
template <class _Cp, bool = __has_storage_type<_Cp>::value>
6286
class __bit_reference {
6387
using __storage_type _LIBCPP_NODEBUG = typename _Cp::__storage_type;

libcxx/include/__fwd/bit_reference.h

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,9 @@
1010
#define _LIBCPP___FWD_BIT_REFERENCE_H
1111

1212
#include <__config>
13+
#include <__memory/pointer_traits.h>
14+
#include <__type_traits/enable_if.h>
15+
#include <__type_traits/is_unsigned.h>
1316

1417
#if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
1518
# pragma GCC system_header
@@ -23,6 +26,10 @@ class __bit_iterator;
2326
template <class, class = void>
2427
struct __size_difference_type_traits;
2528

29+
template <class _StoragePointer>
30+
_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX14 void
31+
__fill_masked_range(_StoragePointer __word, unsigned __ctz, unsigned __clz, bool __fill_val);
32+
2633
_LIBCPP_END_NAMESPACE_STD
2734

2835
#endif // _LIBCPP___FWD_BIT_REFERENCE_H

libcxx/test/std/algorithms/alg.modifying.operations/alg.fill/fill.pass.cpp

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@
1919
#include <cstddef>
2020
#include <vector>
2121

22+
#include "sized_allocator.h"
2223
#include "test_macros.h"
2324
#include "test_iterators.h"
2425

@@ -46,6 +47,39 @@ struct Test {
4647
}
4748
};
4849

50+
// Make sure std::fill behaves properly with std::vector<bool> iterators with custom size types.
51+
// See https://github.com/llvm/llvm-project/pull/122410.
52+
TEST_CONSTEXPR_CXX20 void test_bititer_with_custom_sized_types() {
53+
{
54+
using Alloc = sized_allocator<bool, std::uint8_t, std::int8_t>;
55+
std::vector<bool, Alloc> in(100, false, Alloc(1));
56+
std::vector<bool, Alloc> expected(100, true, Alloc(1));
57+
std::fill(in.begin(), in.end(), true);
58+
assert(in == expected);
59+
}
60+
{
61+
using Alloc = sized_allocator<bool, std::uint16_t, std::int16_t>;
62+
std::vector<bool, Alloc> in(200, false, Alloc(1));
63+
std::vector<bool, Alloc> expected(200, true, Alloc(1));
64+
std::fill(in.begin(), in.end(), true);
65+
assert(in == expected);
66+
}
67+
{
68+
using Alloc = sized_allocator<bool, std::uint32_t, std::int32_t>;
69+
std::vector<bool, Alloc> in(200, false, Alloc(1));
70+
std::vector<bool, Alloc> expected(200, true, Alloc(1));
71+
std::fill(in.begin(), in.end(), true);
72+
assert(in == expected);
73+
}
74+
{
75+
using Alloc = sized_allocator<bool, std::uint64_t, std::int64_t>;
76+
std::vector<bool, Alloc> in(200, false, Alloc(1));
77+
std::vector<bool, Alloc> expected(200, true, Alloc(1));
78+
std::fill(in.begin(), in.end(), true);
79+
assert(in == expected);
80+
}
81+
}
82+
4983
TEST_CONSTEXPR_CXX20 bool test() {
5084
types::for_each(types::forward_iterator_list<char*>(), Test<char>());
5185
types::for_each(types::forward_iterator_list<int*>(), Test<int>());
@@ -93,6 +127,9 @@ TEST_CONSTEXPR_CXX20 bool test() {
93127
assert(in == expected);
94128
}
95129
}
130+
131+
test_bititer_with_custom_sized_types();
132+
96133
return true;
97134
}
98135

0 commit comments

Comments
 (0)