On Sat, 1 Jun 2024 at 12:23, Jonathan Wakely <jwak...@redhat.com> wrote:
>
> Although it's only used from places where we are allocating a sensible
> size, __detail::__get_temporary_buffer should really still check that
> len * sizeof(T) doesn't wrap around to zero and allocate a buffer that's
> smaller than expected.
>
> This v2 patch is the same as v1 except for adding this check:
>
> inline _Tp*
> __get_temporary_buffer(ptrdiff_t __len) _GLIBCXX_NOTHROW
> {
> + if (__builtin_expect(__len > (size_t(-1) / sizeof(_Tp)), 0))
> + return 0;
> +
> #if __cpp_aligned_new
> if (alignof(_Tp) > __STDCPP_DEFAULT_NEW_ALIGNMENT__)
> return (_Tp*) _GLIBCXX_OPERATOR_NEW(__len * sizeof(_Tp),
Pushed to trunk now.
> -- >8 --
>
> This adds extended alignment support to std::get_temporary_buffer etc.
> so that when std::stable_sort uses a temporary buffer it works for
> overaligned types.
>
> Also simplify the _Temporary_buffer type by using RAII for the
> allocation, via a new data member. This simplifies the _Temporary_buffer
> constructor and destructor by makingthem only responsible for
> constructing and destroying the elements, not managing the memory.
>
> libstdc++-v3/ChangeLog:
>
> PR libstdc++/105258
> * include/bits/stl_tempbuf.h (__detail::__get_temporary_buffer):
> New function to do allocation for get_temporary_buffer, with
> extended alignment support.
> (__detail::__return_temporary_buffer): Support extended
> alignment.
> (get_temporary_buffer): Use __get_temporary_buffer.
> (return_temporary_buffer): Support extended alignment. Add
> deprecated attribute.
> (_Temporary_buffer): Move allocation and deallocation into a
> subobject and remove try-catch block in constructor.
> (__uninitialized_construct_buf): Use argument deduction for
> value type.
> * testsuite/20_util/temporary_buffer.cc: Add dg-warning for new
> deprecated warning.
> * testsuite/25_algorithms/stable_sort/overaligned.cc: New test.
> ---
> libstdc++-v3/include/bits/stl_tempbuf.h | 131 ++++++++++++------
> .../testsuite/20_util/temporary_buffer.cc | 2 +-
> .../25_algorithms/stable_sort/overaligned.cc | 29 ++++
> 3 files changed, 116 insertions(+), 46 deletions(-)
> create mode 100644
> libstdc++-v3/testsuite/25_algorithms/stable_sort/overaligned.cc
>
> diff --git a/libstdc++-v3/include/bits/stl_tempbuf.h
> b/libstdc++-v3/include/bits/stl_tempbuf.h
> index 77b121460f9..fa03fd27704 100644
> --- a/libstdc++-v3/include/bits/stl_tempbuf.h
> +++ b/libstdc++-v3/include/bits/stl_tempbuf.h
> @@ -66,19 +66,58 @@ namespace std _GLIBCXX_VISIBILITY(default)
> {
> _GLIBCXX_BEGIN_NAMESPACE_VERSION
>
> +#if __has_builtin(__builtin_operator_new) >= 201802L
> +# define _GLIBCXX_OPERATOR_NEW __builtin_operator_new
> +# define _GLIBCXX_OPERATOR_DELETE __builtin_operator_delete
> +#else
> +# define _GLIBCXX_OPERATOR_NEW ::operator new
> +# define _GLIBCXX_OPERATOR_DELETE ::operator delete
> +#endif
> +
> namespace __detail
> {
> + // Equivalent to std::get_temporary_buffer but won't return a smaller
> size.
> + // It either returns a buffer of __len elements, or a null pointer.
> + template<typename _Tp>
> + inline _Tp*
> + __get_temporary_buffer(ptrdiff_t __len) _GLIBCXX_NOTHROW
> + {
> + if (__builtin_expect(__len > (size_t(-1) / sizeof(_Tp)), 0))
> + return 0;
> +
> +#if __cpp_aligned_new
> + if (alignof(_Tp) > __STDCPP_DEFAULT_NEW_ALIGNMENT__)
> + return (_Tp*) _GLIBCXX_OPERATOR_NEW(__len * sizeof(_Tp),
> + align_val_t(alignof(_Tp)),
> + nothrow_t());
> +#endif
> + return (_Tp*) _GLIBCXX_OPERATOR_NEW(__len * sizeof(_Tp), nothrow_t());
> + }
> +
> + // Equivalent to std::return_temporary_buffer but with a size argument.
> + // The size is the number of elements, not the number of bytes.
> template<typename _Tp>
> inline void
> __return_temporary_buffer(_Tp* __p,
> size_t __len __attribute__((__unused__)))
> {
> #if __cpp_sized_deallocation
> - ::operator delete(__p, __len * sizeof(_Tp));
> +# define _GLIBCXX_SIZED_DEALLOC(T, p, n) (p), (n) * sizeof(T)
> #else
> - ::operator delete(__p);
> +# define _GLIBCXX_SIZED_DEALLOC(T, p, n) (p)
> #endif
> +
> +#if __cpp_aligned_new
> + if (alignof(_Tp) > __STDCPP_DEFAULT_NEW_ALIGNMENT__)
> + {
> + _GLIBCXX_OPERATOR_DELETE(_GLIBCXX_SIZED_DEALLOC(_Tp, __p, __len),
> + align_val_t(alignof(_Tp)));
> + return;
> + }
> +#endif
> + _GLIBCXX_OPERATOR_DELETE(_GLIBCXX_SIZED_DEALLOC(_Tp, __p, __len));
> }
> +#undef _GLIBCXX_SIZED_DEALLOC
> }
>
> /**
> @@ -90,7 +129,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> *
> * This function tries to obtain storage for @c __len adjacent Tp
> * objects. The objects themselves are not constructed, of course.
> - * A pair<> is returned containing <em>the buffer s address and
> + * A pair<> is returned containing <em>the buffer's address and
> * capacity (in the units of sizeof(_Tp)), or a pair of 0 values if
> * no storage can be obtained.</em> Note that the capacity obtained
> * may be less than that requested if the memory is unavailable;
> @@ -110,13 +149,11 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>
> while (__len > 0)
> {
> - _Tp* __tmp = static_cast<_Tp*>(::operator new(__len * sizeof(_Tp),
> - std::nothrow));
> - if (__tmp != 0)
> - return std::pair<_Tp*, ptrdiff_t>(__tmp, __len);
> + if (_Tp* __tmp = __detail::__get_temporary_buffer<_Tp>(__len))
> + return pair<_Tp*, ptrdiff_t>(__tmp, __len);
> __len = __len == 1 ? 0 : ((__len + 1) / 2);
> }
> - return std::pair<_Tp*, ptrdiff_t>(static_cast<_Tp*>(0), 0);
> + return pair<_Tp*, ptrdiff_t>();
> }
>
> /**
> @@ -127,9 +164,20 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> * Frees the memory pointed to by __p.
> */
> template<typename _Tp>
> + _GLIBCXX17_DEPRECATED
> inline void
> return_temporary_buffer(_Tp* __p)
> - { ::operator delete(__p); }
> + {
> +#if __cpp_aligned_new
> + if (alignof(_Tp) > __STDCPP_DEFAULT_NEW_ALIGNMENT__)
> + _GLIBCXX_OPERATOR_DELETE(__p, align_val_t(alignof(_Tp)));
> + else
> +#endif
> + _GLIBCXX_OPERATOR_DELETE(__p);
> + }
> +
> +#undef _GLIBCXX_OPERATOR_DELETE
> +#undef _GLIBCXX_OPERATOR_NEW
>
> /**
> * This class is used in two places: stl_algo.h and ext/memory,
> @@ -150,14 +198,29 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>
> protected:
> size_type _M_original_len;
> - size_type _M_len;
> - pointer _M_buffer;
> + struct _Impl
> + {
> + explicit
> + _Impl(ptrdiff_t __original_len)
> + {
> + pair<pointer, size_type> __p(
> + std::get_temporary_buffer<value_type>(__original_len));
> + _M_len = __p.second;
> + _M_buffer = __p.first;
> + }
> +
> + ~_Impl()
> + { std::__detail::__return_temporary_buffer(_M_buffer, _M_len); }
> +
> + size_type _M_len;
> + pointer _M_buffer;
> + } _M_impl;
>
> public:
> /// As per Table mumble.
> size_type
> size() const
> - { return _M_len; }
> + { return _M_impl._M_len; }
>
> /// Returns the size requested by the constructor; may be >size().
> size_type
> @@ -167,12 +230,12 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> /// As per Table mumble.
> iterator
> begin()
> - { return _M_buffer; }
> + { return _M_impl._M_buffer; }
>
> /// As per Table mumble.
> iterator
> end()
> - { return _M_buffer + _M_len; }
> + { return _M_impl._M_buffer + _M_impl._M_len; }
>
> /**
> * Constructs a temporary buffer of a size somewhere between
> @@ -181,10 +244,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> _Temporary_buffer(_ForwardIterator __seed, size_type __original_len);
>
> ~_Temporary_buffer()
> - {
> - std::_Destroy(_M_buffer, _M_buffer + _M_len);
> - std::__detail::__return_temporary_buffer(_M_buffer, _M_len);
> - }
> + { std::_Destroy(_M_impl._M_buffer, _M_impl._M_buffer +
> _M_impl._M_len); }
>
> private:
> // Disable copy constructor and assignment operator.
> @@ -203,7 +263,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> __ucr(_Pointer __first, _Pointer __last,
> _ForwardIterator __seed)
> {
> - if (__first == __last)
> + if (__builtin_expect(__first == __last, 0))
> return;
>
> _Pointer __cur = __first;
> @@ -243,17 +303,15 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> // the same value when the algorithm finishes, unless one of the
> // constructions throws.
> //
> - // Requirements: _Pointer::value_type(_Tp&&) is valid.
> - template<typename _Pointer, typename _ForwardIterator>
> + // Requirements:
> + // _Tp is move constructible and constructible from std::move(*__seed).
> + template<typename _Tp, typename _ForwardIterator>
> inline void
> - __uninitialized_construct_buf(_Pointer __first, _Pointer __last,
> + __uninitialized_construct_buf(_Tp* __first, _Tp* __last,
> _ForwardIterator __seed)
> {
> - typedef typename std::iterator_traits<_Pointer>::value_type
> - _ValueType;
> -
> std::__uninitialized_construct_buf_dispatch<
> - __has_trivial_constructor(_ValueType)>::
> + __has_trivial_constructor(_Tp)>::
> __ucr(__first, __last, __seed);
> }
>
> @@ -262,26 +320,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> template<typename _ForwardIterator, typename _Tp>
> _Temporary_buffer<_ForwardIterator, _Tp>::
> _Temporary_buffer(_ForwardIterator __seed, size_type __original_len)
> - : _M_original_len(__original_len), _M_len(0), _M_buffer(0)
> + : _M_original_len(__original_len), _M_impl(__original_len)
> {
> - std::pair<pointer, size_type> __p(
> - std::get_temporary_buffer<value_type>(_M_original_len));
> -
> - if (__p.first)
> - {
> - __try
> - {
> - std::__uninitialized_construct_buf(__p.first, __p.first +
> __p.second,
> - __seed);
> - _M_buffer = __p.first;
> - _M_len = __p.second;
> - }
> - __catch(...)
> - {
> - std::__detail::__return_temporary_buffer(__p.first, __p.second);
> - __throw_exception_again;
> - }
> - }
> + std::__uninitialized_construct_buf(begin(), end(), __seed);
> }
> #pragma GCC diagnostic pop
>
> diff --git a/libstdc++-v3/testsuite/20_util/temporary_buffer.cc
> b/libstdc++-v3/testsuite/20_util/temporary_buffer.cc
> index 155d19034e5..065739be29d 100644
> --- a/libstdc++-v3/testsuite/20_util/temporary_buffer.cc
> +++ b/libstdc++-v3/testsuite/20_util/temporary_buffer.cc
> @@ -44,7 +44,7 @@ int main(void)
> VERIFY( results.first == 0 );
> }
>
> - std::return_temporary_buffer(results.first);
> + std::return_temporary_buffer(results.first); // { dg-warning "deprecated"
> "" { target c++17 } }
>
> return 0;
> }
> diff --git a/libstdc++-v3/testsuite/25_algorithms/stable_sort/overaligned.cc
> b/libstdc++-v3/testsuite/25_algorithms/stable_sort/overaligned.cc
> new file mode 100644
> index 00000000000..3c200624617
> --- /dev/null
> +++ b/libstdc++-v3/testsuite/25_algorithms/stable_sort/overaligned.cc
> @@ -0,0 +1,29 @@
> +// { dg-do run { target c++17 } }
> +// PR libstdc++/105258 std::get_temporary_buffer() does not respect alignment
> +
> +#include <algorithm>
> +#include <cstdint>
> +#include <testsuite_hooks.h>
> +
> +struct alignas(__STDCPP_DEFAULT_NEW_ALIGNMENT__ * 2) Overaligned
> +{
> + ~Overaligned()
> + {
> + auto alignment = reinterpret_cast<std::uintptr_t>(this);
> + VERIFY( (alignment % alignof(Overaligned)) == 0 );
> + }
> +
> + bool operator<(const Overaligned&) const { return false; }
> +};
> +
> +void
> +test_pr105258()
> +{
> + Overaligned o[2];
> + std::stable_sort(o, o+2);
> +}
> +
> +int main()
> +{
> + test_pr105258();
> +}
> --
> 2.45.1
>
