Skip to content

Commit 0687e4d

Browse files
ldionneamy-kwan
ldionne
and
amy-kwan
committed
[libc++] Remove UB in list, forward_list and __hash_table
This patch removes undefined behavior in list and forward_list and __hash_table caused by improperly beginning and ending the lifetime of the various node classes. It allows removing the _LIBCPP_STANDALONE_DEBUG macro from these node types since we now properly begin and end their lifetime, meaning that we won't trip up constructor homing. See https://reviews.llvm.org/D98750 for more information on what prompted this patch. Differential Revision: https://reviews.llvm.org/D101206 Co-authored-by: Amy Kwan <[email protected]>
1 parent 091f4f4 commit 0687e4d

File tree

8 files changed

+200
-92
lines changed

8 files changed

+200
-92
lines changed

‎libcxx/include/__hash_table

+79-40
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121
#include <__memory/addressof.h>
2222
#include <__memory/allocator_traits.h>
2323
#include <__memory/compressed_pair.h>
24+
#include <__memory/construct_at.h>
2425
#include <__memory/pointer_traits.h>
2526
#include <__memory/swap_allocator.h>
2627
#include <__memory/unique_ptr.h>
@@ -45,6 +46,7 @@
4546
#include <cmath>
4647
#include <cstring>
4748
#include <initializer_list>
49+
#include <new> // __launder
4850

4951
#if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
5052
# pragma GCC system_header
@@ -107,19 +109,44 @@ struct __hash_node_base
107109
}
108110

109111
_LIBCPP_INLINE_VISIBILITY __hash_node_base() _NOEXCEPT : __next_(nullptr) {}
112+
_LIBCPP_HIDE_FROM_ABI explicit __hash_node_base(__next_pointer __next) _NOEXCEPT : __next_(__next) {}
110113
};
111114

112115
template <class _Tp, class _VoidPtr>
113-
struct _LIBCPP_STANDALONE_DEBUG __hash_node
116+
struct __hash_node
114117
: public __hash_node_base
115118
<
116119
__rebind_pointer_t<_VoidPtr, __hash_node<_Tp, _VoidPtr> >
117120
>
118121
{
119122
typedef _Tp __node_value_type;
123+
using _Base = __hash_node_base<__rebind_pointer_t<_VoidPtr, __hash_node<_Tp, _VoidPtr> > >;
124+
using __next_pointer = typename _Base::__next_pointer;
120125

121126
size_t __hash_;
122-
__node_value_type __value_;
127+
128+
// We allow starting the lifetime of nodes without initializing the value held by the node,
129+
// since that is handled by the hash table itself in order to be allocator-aware.
130+
#ifndef _LIBCPP_CXX03_LANG
131+
private:
132+
union {
133+
_Tp __value_;
134+
};
135+
136+
public:
137+
_LIBCPP_HIDE_FROM_ABI _Tp& __get_value() { return __value_; }
138+
#else
139+
private:
140+
_ALIGNAS_TYPE(_Tp) char __buffer_[sizeof(_Tp)];
141+
142+
public:
143+
_LIBCPP_HIDE_FROM_ABI _Tp& __get_value() {
144+
return *std::__launder(reinterpret_cast<_Tp*>(&__buffer_));
145+
}
146+
#endif
147+
148+
_LIBCPP_HIDE_FROM_ABI explicit __hash_node(__next_pointer __next, size_t __hash) : _Base(__next), __hash_(__hash) {}
149+
_LIBCPP_HIDE_FROM_ABI ~__hash_node() {}
123150
};
124151

125152
inline _LIBCPP_INLINE_VISIBILITY
@@ -311,12 +338,12 @@ public:
311338

312339
_LIBCPP_INLINE_VISIBILITY
313340
reference operator*() const {
314-
return __node_->__upcast()->__value_;
341+
return __node_->__upcast()->__get_value();
315342
}
316343

317344
_LIBCPP_INLINE_VISIBILITY
318345
pointer operator->() const {
319-
return pointer_traits<pointer>::pointer_to(__node_->__upcast()->__value_);
346+
return pointer_traits<pointer>::pointer_to(__node_->__upcast()->__get_value());
320347
}
321348

322349
_LIBCPP_INLINE_VISIBILITY
@@ -387,11 +414,11 @@ public:
387414

388415
_LIBCPP_INLINE_VISIBILITY
389416
reference operator*() const {
390-
return __node_->__upcast()->__value_;
417+
return __node_->__upcast()->__get_value();
391418
}
392419
_LIBCPP_INLINE_VISIBILITY
393420
pointer operator->() const {
394-
return pointer_traits<pointer>::pointer_to(__node_->__upcast()->__value_);
421+
return pointer_traits<pointer>::pointer_to(__node_->__upcast()->__get_value());
395422
}
396423

397424
_LIBCPP_INLINE_VISIBILITY
@@ -453,12 +480,12 @@ public:
453480

454481
_LIBCPP_INLINE_VISIBILITY
455482
reference operator*() const {
456-
return __node_->__upcast()->__value_;
483+
return __node_->__upcast()->__get_value();
457484
}
458485

459486
_LIBCPP_INLINE_VISIBILITY
460487
pointer operator->() const {
461-
return pointer_traits<pointer>::pointer_to(__node_->__upcast()->__value_);
488+
return pointer_traits<pointer>::pointer_to(__node_->__upcast()->__get_value());
462489
}
463490

464491
_LIBCPP_INLINE_VISIBILITY
@@ -543,12 +570,12 @@ public:
543570

544571
_LIBCPP_INLINE_VISIBILITY
545572
reference operator*() const {
546-
return __node_->__upcast()->__value_;
573+
return __node_->__upcast()->__get_value();
547574
}
548575

549576
_LIBCPP_INLINE_VISIBILITY
550577
pointer operator->() const {
551-
return pointer_traits<pointer>::pointer_to(__node_->__upcast()->__value_);
578+
return pointer_traits<pointer>::pointer_to(__node_->__upcast()->__get_value());
552579
}
553580

554581
_LIBCPP_INLINE_VISIBILITY
@@ -670,8 +697,10 @@ public:
670697
_LIBCPP_INLINE_VISIBILITY
671698
void operator()(pointer __p) _NOEXCEPT
672699
{
673-
if (__value_constructed)
674-
__alloc_traits::destroy(__na_, _NodeTypes::__get_ptr(__p->__value_));
700+
if (__value_constructed) {
701+
__alloc_traits::destroy(__na_, _NodeTypes::__get_ptr(__p->__get_value()));
702+
std::__destroy_at(std::addressof(*__p));
703+
}
675704
if (__p)
676705
__alloc_traits::deallocate(__na_, __p, 1);
677706
}
@@ -1365,7 +1394,8 @@ __hash_table<_Tp, _Hash, _Equal, _Alloc>::__deallocate_node(__next_pointer __np)
13651394
{
13661395
__next_pointer __next = __np->__next_;
13671396
__node_pointer __real_np = __np->__upcast();
1368-
__node_traits::destroy(__na, _NodeTypes::__get_ptr(__real_np->__value_));
1397+
__node_traits::destroy(__na, _NodeTypes::__get_ptr(__real_np->__get_value()));
1398+
std::__destroy_at(std::addressof(*__real_np));
13691399
__node_traits::deallocate(__na, __real_np, 1);
13701400
__np = __next;
13711401
}
@@ -1434,8 +1464,8 @@ __hash_table<_Tp, _Hash, _Equal, _Alloc>::__move_assign(
14341464
const_iterator __i = __u.begin();
14351465
while (__cache != nullptr && __u.size() != 0)
14361466
{
1437-
__cache->__upcast()->__value_ =
1438-
_VSTD::move(__u.remove(__i++)->__value_);
1467+
__cache->__upcast()->__get_value() =
1468+
_VSTD::move(__u.remove(__i++)->__get_value());
14391469
__next_pointer __next = __cache->__next_;
14401470
__node_insert_multi(__cache->__upcast());
14411471
__cache = __next;
@@ -1453,7 +1483,7 @@ __hash_table<_Tp, _Hash, _Equal, _Alloc>::__move_assign(
14531483
const_iterator __i = __u.begin();
14541484
while (__u.size() != 0)
14551485
{
1456-
__node_holder __h = __construct_node(_NodeTypes::__move(__u.remove(__i++)->__value_));
1486+
__node_holder __h = __construct_node(_NodeTypes::__move(__u.remove(__i++)->__get_value()));
14571487
__node_insert_multi(__h.get());
14581488
__h.release();
14591489
}
@@ -1495,7 +1525,7 @@ __hash_table<_Tp, _Hash, _Equal, _Alloc>::__assign_unique(_InputIterator __first
14951525
#endif // _LIBCPP_HAS_NO_EXCEPTIONS
14961526
for (; __cache != nullptr && __first != __last; ++__first)
14971527
{
1498-
__cache->__upcast()->__value_ = *__first;
1528+
__cache->__upcast()->__get_value() = *__first;
14991529
__next_pointer __next = __cache->__next_;
15001530
__node_insert_unique(__cache->__upcast());
15011531
__cache = __next;
@@ -1535,7 +1565,7 @@ __hash_table<_Tp, _Hash, _Equal, _Alloc>::__assign_multi(_InputIterator __first,
15351565
#endif // _LIBCPP_HAS_NO_EXCEPTIONS
15361566
for (; __cache != nullptr && __first != __last; ++__first)
15371567
{
1538-
__cache->__upcast()->__value_ = *__first;
1568+
__cache->__upcast()->__get_value() = *__first;
15391569
__next_pointer __next = __cache->__next_;
15401570
__node_insert_multi(__cache->__upcast());
15411571
__cache = __next;
@@ -1629,7 +1659,7 @@ __hash_table<_Tp, _Hash, _Equal, _Alloc>::__node_insert_unique_prepare(
16291659
__ndptr = __ndptr->__next_)
16301660
{
16311661
if ((__ndptr->__hash() == __hash) &&
1632-
key_eq()(__ndptr->__upcast()->__value_, __value))
1662+
key_eq()(__ndptr->__upcast()->__get_value(), __value))
16331663
return __ndptr;
16341664
}
16351665
}
@@ -1678,9 +1708,9 @@ template <class _Tp, class _Hash, class _Equal, class _Alloc>
16781708
pair<typename __hash_table<_Tp, _Hash, _Equal, _Alloc>::iterator, bool>
16791709
__hash_table<_Tp, _Hash, _Equal, _Alloc>::__node_insert_unique(__node_pointer __nd)
16801710
{
1681-
__nd->__hash_ = hash_function()(__nd->__value_);
1711+
__nd->__hash_ = hash_function()(__nd->__get_value());
16821712
__next_pointer __existing_node =
1683-
__node_insert_unique_prepare(__nd->__hash(), __nd->__value_);
1713+
__node_insert_unique_prepare(__nd->__hash(), __nd->__get_value());
16841714

16851715
// Insert the node, unless it already exists in the container.
16861716
bool __inserted = false;
@@ -1726,7 +1756,7 @@ __hash_table<_Tp, _Hash, _Equal, _Alloc>::__node_insert_multi_prepare(
17261756
// false true set __found to true
17271757
// true false break
17281758
if (__found != (__pn->__next_->__hash() == __cp_hash &&
1729-
key_eq()(__pn->__next_->__upcast()->__value_, __cp_val)))
1759+
key_eq()(__pn->__next_->__upcast()->__get_value(), __cp_val)))
17301760
{
17311761
if (!__found)
17321762
__found = true;
@@ -1780,8 +1810,8 @@ template <class _Tp, class _Hash, class _Equal, class _Alloc>
17801810
typename __hash_table<_Tp, _Hash, _Equal, _Alloc>::iterator
17811811
__hash_table<_Tp, _Hash, _Equal, _Alloc>::__node_insert_multi(__node_pointer __cp)
17821812
{
1783-
__cp->__hash_ = hash_function()(__cp->__value_);
1784-
__next_pointer __pn = __node_insert_multi_prepare(__cp->__hash(), __cp->__value_);
1813+
__cp->__hash_ = hash_function()(__cp->__get_value());
1814+
__next_pointer __pn = __node_insert_multi_prepare(__cp->__hash(), __cp->__get_value());
17851815
__node_insert_multi_perform(__cp, __pn);
17861816

17871817
return iterator(__cp->__ptr());
@@ -1792,7 +1822,7 @@ typename __hash_table<_Tp, _Hash, _Equal, _Alloc>::iterator
17921822
__hash_table<_Tp, _Hash, _Equal, _Alloc>::__node_insert_multi(
17931823
const_iterator __p, __node_pointer __cp)
17941824
{
1795-
if (__p != end() && key_eq()(*__p, __cp->__value_))
1825+
if (__p != end() && key_eq()(*__p, __cp->__get_value()))
17961826
{
17971827
__next_pointer __np = __p.__node_;
17981828
__cp->__hash_ = __np->__hash();
@@ -1839,7 +1869,7 @@ __hash_table<_Tp, _Hash, _Equal, _Alloc>::__emplace_unique_key_args(_Key const&
18391869
__nd = __nd->__next_)
18401870
{
18411871
if ((__nd->__hash() == __hash) &&
1842-
key_eq()(__nd->__upcast()->__value_, __k))
1872+
key_eq()(__nd->__upcast()->__get_value(), __k))
18431873
goto __done;
18441874
}
18451875
}
@@ -1983,9 +2013,9 @@ __hash_table<_Tp, _Hash, _Equal, _Alloc>::__node_handle_merge_unique(
19832013
__it != __source.end();)
19842014
{
19852015
__node_pointer __src_ptr = __it.__node_->__upcast();
1986-
size_t __hash = hash_function()(__src_ptr->__value_);
2016+
size_t __hash = hash_function()(__src_ptr->__get_value());
19872017
__next_pointer __existing_node =
1988-
__node_insert_unique_prepare(__hash, __src_ptr->__value_);
2018+
__node_insert_unique_prepare(__hash, __src_ptr->__get_value());
19892019
auto __prev_iter = __it++;
19902020
if (__existing_node == nullptr)
19912021
{
@@ -2037,9 +2067,9 @@ __hash_table<_Tp, _Hash, _Equal, _Alloc>::__node_handle_merge_multi(
20372067
__it != __source.end();)
20382068
{
20392069
__node_pointer __src_ptr = __it.__node_->__upcast();
2040-
size_t __src_hash = hash_function()(__src_ptr->__value_);
2070+
size_t __src_hash = hash_function()(__src_ptr->__get_value());
20412071
__next_pointer __pn =
2042-
__node_insert_multi_prepare(__src_hash, __src_ptr->__value_);
2072+
__node_insert_multi_prepare(__src_hash, __src_ptr->__get_value());
20432073
(void)__source.remove(__it++).release();
20442074
__src_ptr->__hash_ = __src_hash;
20452075
__node_insert_multi_perform(__src_ptr, __pn);
@@ -2113,8 +2143,8 @@ __hash_table<_Tp, _Hash, _Equal, _Alloc>::__do_rehash(size_type __nbc)
21132143
if _LIBCPP_CONSTEXPR_SINCE_CXX17 (!_UniqueKeys)
21142144
{
21152145
for (; __np->__next_ != nullptr &&
2116-
key_eq()(__cp->__upcast()->__value_,
2117-
__np->__next_->__upcast()->__value_);
2146+
key_eq()(__cp->__upcast()->__get_value(),
2147+
__np->__next_->__upcast()->__get_value());
21182148
__np = __np->__next_)
21192149
;
21202150
}
@@ -2148,7 +2178,7 @@ __hash_table<_Tp, _Hash, _Equal, _Alloc>::find(const _Key& __k)
21482178
__nd = __nd->__next_)
21492179
{
21502180
if ((__nd->__hash() == __hash)
2151-
&& key_eq()(__nd->__upcast()->__value_, __k))
2181+
&& key_eq()(__nd->__upcast()->__get_value(), __k))
21522182
return iterator(__nd);
21532183
}
21542184
}
@@ -2175,7 +2205,7 @@ __hash_table<_Tp, _Hash, _Equal, _Alloc>::find(const _Key& __k) const
21752205
__nd = __nd->__next_)
21762206
{
21772207
if ((__nd->__hash() == __hash)
2178-
&& key_eq()(__nd->__upcast()->__value_, __k))
2208+
&& key_eq()(__nd->__upcast()->__get_value(), __k))
21792209
return const_iterator(__nd);
21802210
}
21812211
}
@@ -2193,10 +2223,20 @@ __hash_table<_Tp, _Hash, _Equal, _Alloc>::__construct_node(_Args&& ...__args)
21932223
"Construct cannot be called with a hash value type");
21942224
__node_allocator& __na = __node_alloc();
21952225
__node_holder __h(__node_traits::allocate(__na, 1), _Dp(__na));
2196-
__node_traits::construct(__na, _NodeTypes::__get_ptr(__h->__value_), _VSTD::forward<_Args>(__args)...);
2226+
2227+
// Begin the lifetime of the node itself. Note that this doesn't begin the lifetime of the value
2228+
// held inside the node, since we need to use the allocator's construct() method for that.
2229+
//
2230+
// We don't use the allocator's construct() method to construct the node itself since the
2231+
// Cpp17FooInsertable named requirements don't require the allocator's construct() method
2232+
// to work on anything other than the value_type.
2233+
std::__construct_at(std::addressof(*__h), /* next = */nullptr, /* hash = */0);
2234+
2235+
// Now construct the value_type using the allocator's construct() method.
2236+
__node_traits::construct(__na, _NodeTypes::__get_ptr(__h->__get_value()), _VSTD::forward<_Args>(__args)...);
21972237
__h.get_deleter().__value_constructed = true;
2198-
__h->__hash_ = hash_function()(__h->__value_);
2199-
__h->__next_ = nullptr;
2238+
2239+
__h->__hash_ = hash_function()(__h->__get_value());
22002240
return __h;
22012241
}
22022242

@@ -2210,12 +2250,11 @@ __hash_table<_Tp, _Hash, _Equal, _Alloc>::__construct_node_hash(
22102250
"Construct cannot be called with a hash value type");
22112251
__node_allocator& __na = __node_alloc();
22122252
__node_holder __h(__node_traits::allocate(__na, 1), _Dp(__na));
2213-
__node_traits::construct(__na, _NodeTypes::__get_ptr(__h->__value_),
2253+
std::__construct_at(std::addressof(*__h), /* next = */nullptr, /* hash = */__hash);
2254+
__node_traits::construct(__na, _NodeTypes::__get_ptr(__h->__get_value()),
22142255
_VSTD::forward<_First>(__f),
22152256
_VSTD::forward<_Rest>(__rest)...);
22162257
__h.get_deleter().__value_constructed = true;
2217-
__h->__hash_ = __hash;
2218-
__h->__next_ = nullptr;
22192258
return __h;
22202259
}
22212260

‎libcxx/include/__node_handle

+3-3
Original file line numberDiff line numberDiff line change
@@ -209,7 +209,7 @@ struct __set_node_handle_specifics
209209
_LIBCPP_INLINE_VISIBILITY
210210
value_type& value() const
211211
{
212-
return static_cast<_Derived const*>(this)->__ptr_->__value_;
212+
return static_cast<_Derived const*>(this)->__ptr_->__get_value();
213213
}
214214
};
215215

@@ -223,14 +223,14 @@ struct __map_node_handle_specifics
223223
key_type& key() const
224224
{
225225
return static_cast<_Derived const*>(this)->
226-
__ptr_->__value_.__ref().first;
226+
__ptr_->__get_value().__ref().first;
227227
}
228228

229229
_LIBCPP_INLINE_VISIBILITY
230230
mapped_type& mapped() const
231231
{
232232
return static_cast<_Derived const*>(this)->
233-
__ptr_->__value_.__ref().second;
233+
__ptr_->__get_value().__ref().second;
234234
}
235235
};
236236

‎libcxx/include/__tree

+2
Original file line numberDiff line numberDiff line change
@@ -774,6 +774,8 @@ public:
774774

775775
__node_value_type __value_;
776776

777+
_LIBCPP_HIDE_FROM_ABI _Tp& __get_value() { return __value_; }
778+
777779
private:
778780
~__tree_node() = delete;
779781
__tree_node(__tree_node const&) = delete;

‎libcxx/include/ext/hash_map

+4-4
Original file line numberDiff line numberDiff line change
@@ -357,9 +357,9 @@ public:
357357
void operator()(pointer __p)
358358
{
359359
if (__second_constructed)
360-
__alloc_traits::destroy(__na_, _VSTD::addressof(__p->__value_.second));
360+
__alloc_traits::destroy(__na_, _VSTD::addressof(__p->__get_value().second));
361361
if (__first_constructed)
362-
__alloc_traits::destroy(__na_, _VSTD::addressof(__p->__value_.first));
362+
__alloc_traits::destroy(__na_, _VSTD::addressof(__p->__get_value().first));
363363
if (__p)
364364
__alloc_traits::deallocate(__na_, __p, 1);
365365
}
@@ -667,9 +667,9 @@ hash_map<_Key, _Tp, _Hash, _Pred, _Alloc>::__construct_node(const key_type& __k)
667667
{
668668
__node_allocator& __na = __table_.__node_alloc();
669669
__node_holder __h(__node_traits::allocate(__na, 1), _Dp(__na));
670-
__node_traits::construct(__na, _VSTD::addressof(__h->__value_.first), __k);
670+
__node_traits::construct(__na, _VSTD::addressof(__h->__get_value().first), __k);
671671
__h.get_deleter().__first_constructed = true;
672-
__node_traits::construct(__na, _VSTD::addressof(__h->__value_.second));
672+
__node_traits::construct(__na, _VSTD::addressof(__h->__get_value().second));
673673
__h.get_deleter().__second_constructed = true;
674674
return __h;
675675
}

0 commit comments

Comments
 (0)