Skip to content
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.

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

[libc++] Fix UB in bitwise logic of {std, ranges}::{fill, fill_n} algorithms #122410

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
12 changes: 2 additions & 10 deletions libcxx/include/__algorithm/fill_n.h
winner245 marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -41,11 +41,7 @@ __fill_n_bool(__bit_iterator<_Cp, false> __first, typename __size_difference_typ
if (__first.__ctz_ != 0) {
__storage_type __clz_f = static_cast<__storage_type>(__bits_per_word - __first.__ctz_);
__storage_type __dn = std::min(__clz_f, __n);
__storage_type __m = (~__storage_type(0) << __first.__ctz_) & (~__storage_type(0) >> (__clz_f - __dn));
if (_FillVal)
*__first.__seg_ |= __m;
else
*__first.__seg_ &= ~__m;
std::__fill_masked_range(std::__to_address(__first.__seg_), __clz_f - __dn, __first.__ctz_, _FillVal);
__n -= __dn;
++__first.__seg_;
}
Expand All @@ -56,11 +52,7 @@ __fill_n_bool(__bit_iterator<_Cp, false> __first, typename __size_difference_typ
// do last partial word
if (__n > 0) {
__first.__seg_ += __nw;
__storage_type __m = ~__storage_type(0) >> (__bits_per_word - __n);
if (_FillVal)
*__first.__seg_ |= __m;
else
*__first.__seg_ &= ~__m;
std::__fill_masked_range(std::__to_address(__first.__seg_), __bits_per_word - __n, 0u, _FillVal);
}
}

Expand Down
23 changes: 23 additions & 0 deletions libcxx/include/__bit_reference
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@

#include <__algorithm/copy_n.h>
#include <__algorithm/min.h>
#include <__assert>
#include <__bit/countr.h>
#include <__compare/ordering.h>
#include <__config>
Expand All @@ -22,9 +23,12 @@
#include <__memory/construct_at.h>
#include <__memory/pointer_traits.h>
#include <__type_traits/conditional.h>
#include <__type_traits/enable_if.h>
#include <__type_traits/is_constant_evaluated.h>
#include <__type_traits/is_unsigned.h>
#include <__type_traits/void_t.h>
#include <__utility/swap.h>
#include <climits>

#if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
# pragma GCC system_header
Expand Down Expand Up @@ -55,6 +59,25 @@ struct __size_difference_type_traits<_Cp, __void_t<typename _Cp::difference_type
using size_type = typename _Cp::size_type;
};

// This function is designed to operate correctly even for smaller integral types like `uint8_t`, `uint16_t`,
// or `unsigned short`. Casting back to _StorageType is crucial to prevent undefined behavior that can arise
// from integral promotions.
// See https://github.com/llvm/llvm-project/pull/122410.
template <class _StoragePointer,
__enable_if_t<is_unsigned<typename pointer_traits<_StoragePointer>::element_type>::value, int> >
_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX14 void
__fill_masked_range(_StoragePointer __word, unsigned __clz, unsigned __ctz, bool __fill_val) {
using _StorageType = typename pointer_traits<_StoragePointer>::element_type;
_LIBCPP_ASSERT_VALID_INPUT_RANGE(
__ctz + __clz < sizeof(_StorageType) * CHAR_BIT, "__fill_masked_range called with invalid range");
_StorageType __m = static_cast<_StorageType>(static_cast<_StorageType>(~static_cast<_StorageType>(0)) >> __clz) &
static_cast<_StorageType>(static_cast<_StorageType>(~static_cast<_StorageType>(0)) << __ctz);
if (__fill_val)
*__word |= __m;
else
*__word &= static_cast<_StorageType>(~__m);
}

template <class _Cp, bool = __has_storage_type<_Cp>::value>
class __bit_reference {
using __storage_type _LIBCPP_NODEBUG = typename _Cp::__storage_type;
Expand Down
8 changes: 8 additions & 0 deletions libcxx/include/__fwd/bit_reference.h
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,9 @@
#define _LIBCPP___FWD_BIT_REFERENCE_H

#include <__config>
#include <__memory/pointer_traits.h>
#include <__type_traits/enable_if.h>
#include <__type_traits/is_unsigned.h>

#if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
# pragma GCC system_header
Expand All @@ -23,6 +26,11 @@ class __bit_iterator;
template <class, class = void>
struct __size_difference_type_traits;

template <class _StoragePointer,
__enable_if_t<is_unsigned<typename pointer_traits<_StoragePointer>::element_type>::value, int> = 0>
_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX14 void
__fill_masked_range(_StoragePointer __word, unsigned __ctz, unsigned __clz, bool __fill_val);

_LIBCPP_END_NAMESPACE_STD

#endif // _LIBCPP___FWD_BIT_REFERENCE_H
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
#include <cstddef>
#include <vector>

#include "sized_allocator.h"
#include "test_macros.h"
#include "test_iterators.h"

Expand Down Expand Up @@ -46,6 +47,37 @@ struct Test {
}
};

TEST_CONSTEXPR_CXX20 void test_bititer_with_custom_sized_types() {
{
using Alloc = sized_allocator<bool, std::uint8_t, std::int8_t>;
std::vector<bool, Alloc> in(100, false, Alloc(1));
std::vector<bool, Alloc> expected(100, true, Alloc(1));
std::fill(in.begin(), in.end(), true);
assert(in == expected);
}
{
using Alloc = sized_allocator<bool, std::uint16_t, std::int16_t>;
std::vector<bool, Alloc> in(200, false, Alloc(1));
std::vector<bool, Alloc> expected(200, true, Alloc(1));
std::fill(in.begin(), in.end(), true);
assert(in == expected);
}
{
using Alloc = sized_allocator<bool, std::uint32_t, std::int32_t>;
std::vector<bool, Alloc> in(200, false, Alloc(1));
std::vector<bool, Alloc> expected(200, true, Alloc(1));
std::fill(in.begin(), in.end(), true);
assert(in == expected);
}
{
using Alloc = sized_allocator<bool, std::uint64_t, std::int64_t>;
std::vector<bool, Alloc> in(200, false, Alloc(1));
std::vector<bool, Alloc> expected(200, true, Alloc(1));
std::fill(in.begin(), in.end(), true);
assert(in == expected);
}
}

TEST_CONSTEXPR_CXX20 bool test() {
types::for_each(types::forward_iterator_list<char*>(), Test<char>());
types::for_each(types::forward_iterator_list<int*>(), Test<int>());
Expand Down Expand Up @@ -93,6 +125,9 @@ TEST_CONSTEXPR_CXX20 bool test() {
assert(in == expected);
}
}

test_bititer_with_custom_sized_types();

return true;
}

Expand Down
Loading
Loading