Skip to content

Commit 9832a97

Browse files
jiixyjjiixyj
committed
[libc++] Fix UB in <expected> related to "has value" flag (#68552)
The calls to `std::construct_at` might overwrite the previously set `__has_value_` flag, so reverse the order everywhere. Where possible, avoid calling `std::construct_at` and construct the value/error directly into the union.
1 parent 45a29bd commit 9832a97

File tree

2 files changed

+117
-90
lines changed

2 files changed

+117
-90
lines changed

‎libcxx/include/__expected/expected.h

Lines changed: 78 additions & 90 deletions
Original file line numberDiff line numberDiff line change
@@ -119,9 +119,7 @@ class expected {
119119
_LIBCPP_HIDE_FROM_ABI constexpr expected()
120120
noexcept(is_nothrow_default_constructible_v<_Tp>) // strengthened
121121
requires is_default_constructible_v<_Tp>
122-
: __has_val_(true) {
123-
std::construct_at(std::addressof(__union_.__val_));
124-
}
122+
: __union_(__construct_in_place_tag{}), __has_val_(true) {}
125123

126124
_LIBCPP_HIDE_FROM_ABI constexpr expected(const expected&) = delete;
127125

@@ -136,14 +134,7 @@ class expected {
136134
noexcept(is_nothrow_copy_constructible_v<_Tp> && is_nothrow_copy_constructible_v<_Err>) // strengthened
137135
requires(is_copy_constructible_v<_Tp> && is_copy_constructible_v<_Err> &&
138136
!(is_trivially_copy_constructible_v<_Tp> && is_trivially_copy_constructible_v<_Err>))
139-
: __has_val_(__other.__has_val_) {
140-
if (__has_val_) {
141-
std::construct_at(std::addressof(__union_.__val_), __other.__union_.__val_);
142-
} else {
143-
std::construct_at(std::addressof(__union_.__unex_), __other.__union_.__unex_);
144-
}
145-
}
146-
137+
: __union_(__union_from_expected(__other)), __has_val_(__other.__has_val_) { }
147138

148139
_LIBCPP_HIDE_FROM_ABI constexpr expected(expected&&)
149140
requires(is_move_constructible_v<_Tp> && is_move_constructible_v<_Err>
@@ -154,13 +145,7 @@ class expected {
154145
noexcept(is_nothrow_move_constructible_v<_Tp> && is_nothrow_move_constructible_v<_Err>)
155146
requires(is_move_constructible_v<_Tp> && is_move_constructible_v<_Err> &&
156147
!(is_trivially_move_constructible_v<_Tp> && is_trivially_move_constructible_v<_Err>))
157-
: __has_val_(__other.__has_val_) {
158-
if (__has_val_) {
159-
std::construct_at(std::addressof(__union_.__val_), std::move(__other.__union_.__val_));
160-
} else {
161-
std::construct_at(std::addressof(__union_.__unex_), std::move(__other.__union_.__unex_));
162-
}
163-
}
148+
: __union_(__union_from_expected(std::move(__other))), __has_val_(__other.__has_val_) { }
164149

165150
private:
166151
template <class _Up, class _OtherErr, class _UfQual, class _OtherErrQual>
@@ -200,88 +185,62 @@ class expected {
200185
expected(const expected<_Up, _OtherErr>& __other)
201186
noexcept(is_nothrow_constructible_v<_Tp, const _Up&> &&
202187
is_nothrow_constructible_v<_Err, const _OtherErr&>) // strengthened
203-
: __has_val_(__other.__has_val_) {
204-
if (__has_val_) {
205-
std::construct_at(std::addressof(__union_.__val_), __other.__union_.__val_);
206-
} else {
207-
std::construct_at(std::addressof(__union_.__unex_), __other.__union_.__unex_);
208-
}
209-
}
188+
: __union_(__union_from_expected(__other)), __has_val_(__other.__has_val_) {}
210189

211190
template <class _Up, class _OtherErr>
212191
requires __can_convert<_Up, _OtherErr, _Up, _OtherErr>::value
213192
_LIBCPP_HIDE_FROM_ABI constexpr explicit(!is_convertible_v<_Up, _Tp> || !is_convertible_v<_OtherErr, _Err>)
214193
expected(expected<_Up, _OtherErr>&& __other)
215194
noexcept(is_nothrow_constructible_v<_Tp, _Up> && is_nothrow_constructible_v<_Err, _OtherErr>) // strengthened
216-
: __has_val_(__other.__has_val_) {
217-
if (__has_val_) {
218-
std::construct_at(std::addressof(__union_.__val_), std::move(__other.__union_.__val_));
219-
} else {
220-
std::construct_at(std::addressof(__union_.__unex_), std::move(__other.__union_.__unex_));
221-
}
222-
}
195+
: __union_(__union_from_expected(std::move(__other))), __has_val_(__other.__has_val_) {}
223196

224197
template <class _Up = _Tp>
225198
requires(!is_same_v<remove_cvref_t<_Up>, in_place_t> && !is_same_v<expected, remove_cvref_t<_Up>> &&
226199
is_constructible_v<_Tp, _Up> && !__is_std_unexpected<remove_cvref_t<_Up>>::value &&
227200
(!is_same_v<remove_cv_t<_Tp>, bool> || !__is_std_expected<remove_cvref_t<_Up>>::value))
228201
_LIBCPP_HIDE_FROM_ABI constexpr explicit(!is_convertible_v<_Up, _Tp>)
229202
expected(_Up&& __u) noexcept(is_nothrow_constructible_v<_Tp, _Up>) // strengthened
230-
: __has_val_(true) {
231-
std::construct_at(std::addressof(__union_.__val_), std::forward<_Up>(__u));
232-
}
203+
: __union_(__construct_in_place_tag{}, std::forward<_Up>(__u)), __has_val_(true) {}
233204

234205
template <class _OtherErr>
235206
requires is_constructible_v<_Err, const _OtherErr&>
236207
_LIBCPP_HIDE_FROM_ABI constexpr explicit(!is_convertible_v<const _OtherErr&, _Err>)
237208
expected(const unexpected<_OtherErr>& __unex)
238209
noexcept(is_nothrow_constructible_v<_Err, const _OtherErr&>) // strengthened
239-
: __has_val_(false) {
240-
std::construct_at(std::addressof(__union_.__unex_), __unex.error());
241-
}
210+
: __union_(__construct_unexpected_tag{}, __unex.error()), __has_val_(false) {}
242211

243212
template <class _OtherErr>
244213
requires is_constructible_v<_Err, _OtherErr>
245214
_LIBCPP_HIDE_FROM_ABI constexpr explicit(!is_convertible_v<_OtherErr, _Err>)
246215
expected(unexpected<_OtherErr>&& __unex)
247216
noexcept(is_nothrow_constructible_v<_Err, _OtherErr>) // strengthened
248-
: __has_val_(false) {
249-
std::construct_at(std::addressof(__union_.__unex_), std::move(__unex.error()));
250-
}
217+
: __union_(__construct_unexpected_tag{}, std::move(__unex.error())), __has_val_(false) {}
251218

252219
template <class... _Args>
253220
requires is_constructible_v<_Tp, _Args...>
254221
_LIBCPP_HIDE_FROM_ABI constexpr explicit expected(in_place_t, _Args&&... __args)
255222
noexcept(is_nothrow_constructible_v<_Tp, _Args...>) // strengthened
256-
: __has_val_(true) {
257-
std::construct_at(std::addressof(__union_.__val_), std::forward<_Args>(__args)...);
258-
}
223+
: __union_(__construct_in_place_tag{}, std::forward<_Args>(__args)...), __has_val_(true) {}
259224

260225
template <class _Up, class... _Args>
261226
requires is_constructible_v< _Tp, initializer_list<_Up>&, _Args... >
262227
_LIBCPP_HIDE_FROM_ABI constexpr explicit
263228
expected(in_place_t, initializer_list<_Up> __il, _Args&&... __args)
264229
noexcept(is_nothrow_constructible_v<_Tp, initializer_list<_Up>&, _Args...>) // strengthened
265-
: __has_val_(true) {
266-
std::construct_at(std::addressof(__union_.__val_), __il, std::forward<_Args>(__args)...);
267-
}
230+
: __union_(__construct_in_place_tag{}, __il, std::forward<_Args>(__args)...), __has_val_(true) {}
268231

269232
template <class... _Args>
270233
requires is_constructible_v<_Err, _Args...>
271234
_LIBCPP_HIDE_FROM_ABI constexpr explicit expected(unexpect_t, _Args&&... __args)
272-
noexcept(is_nothrow_constructible_v<_Err, _Args...>) // strengthened
273-
: __has_val_(false) {
274-
std::construct_at(std::addressof(__union_.__unex_), std::forward<_Args>(__args)...);
275-
}
235+
noexcept(is_nothrow_constructible_v<_Err, _Args...>) // strengthened
236+
: __union_(__construct_unexpected_tag{}, std::forward<_Args>(__args)...), __has_val_(false) {}
276237

277238
template <class _Up, class... _Args>
278239
requires is_constructible_v< _Err, initializer_list<_Up>&, _Args... >
279240
_LIBCPP_HIDE_FROM_ABI constexpr explicit
280241
expected(unexpect_t, initializer_list<_Up> __il, _Args&&... __args)
281242
noexcept(is_nothrow_constructible_v<_Err, initializer_list<_Up>&, _Args...>) // strengthened
282-
: __has_val_(false) {
283-
std::construct_at(std::addressof(__union_.__unex_), __il, std::forward<_Args>(__args)...);
284-
}
243+
: __union_(__construct_unexpected_tag{}, __il, std::forward<_Args>(__args)...), __has_val_(false) {}
285244

286245
// [expected.object.dtor], destructor
287246

@@ -440,9 +399,10 @@ class expected {
440399
std::destroy_at(std::addressof(__union_.__val_));
441400
} else {
442401
std::destroy_at(std::addressof(__union_.__unex_));
443-
__has_val_ = true;
444402
}
445-
return *std::construct_at(std::addressof(__union_.__val_), std::forward<_Args>(__args)...);
403+
std::construct_at(std::addressof(__union_.__val_), std::forward<_Args>(__args)...);
404+
__has_val_ = true;
405+
return *std::addressof(__union_.__val_);
446406
}
447407

448408
template <class _Up, class... _Args>
@@ -452,9 +412,10 @@ class expected {
452412
std::destroy_at(std::addressof(__union_.__val_));
453413
} else {
454414
std::destroy_at(std::addressof(__union_.__unex_));
455-
__has_val_ = true;
456415
}
457-
return *std::construct_at(std::addressof(__union_.__val_), __il, std::forward<_Args>(__args)...);
416+
std::construct_at(std::addressof(__union_.__val_), __il, std::forward<_Args>(__args)...);
417+
__has_val_ = true;
418+
return *std::addressof(__union_.__val_);
458419
}
459420

460421

@@ -894,11 +855,21 @@ class expected {
894855

895856
private:
896857
struct __empty_t {};
858+
struct __construct_in_place_tag {};
859+
struct __construct_unexpected_tag {};
897860

898861
template <class _ValueType, class _ErrorType>
899862
union __union_t {
900863
_LIBCPP_HIDE_FROM_ABI constexpr __union_t() {}
901864

865+
template <class... _Args>
866+
_LIBCPP_HIDE_FROM_ABI constexpr explicit __union_t(__construct_in_place_tag, _Args&&... __args)
867+
: __val_(std::forward<_Args>(__args)...) {}
868+
869+
template <class... _Args>
870+
_LIBCPP_HIDE_FROM_ABI constexpr explicit __union_t(__construct_unexpected_tag, _Args&&... __args)
871+
: __unex_(std::forward<_Args>(__args)...) {}
872+
902873
template <class _Func, class... _Args>
903874
_LIBCPP_HIDE_FROM_ABI constexpr explicit __union_t(
904875
std::__expected_construct_in_place_from_invoke_tag, _Func&& __f, _Args&&... __args)
@@ -931,6 +902,14 @@ class expected {
931902
_LIBCPP_HIDE_FROM_ABI constexpr __union_t(const __union_t&) = default;
932903
_LIBCPP_HIDE_FROM_ABI constexpr __union_t& operator=(const __union_t&) = default;
933904

905+
template <class... _Args>
906+
_LIBCPP_HIDE_FROM_ABI constexpr explicit __union_t(__construct_in_place_tag, _Args&&... __args)
907+
: __val_(std::forward<_Args>(__args)...) {}
908+
909+
template <class... _Args>
910+
_LIBCPP_HIDE_FROM_ABI constexpr explicit __union_t(__construct_unexpected_tag, _Args&&... __args)
911+
: __unex_(std::forward<_Args>(__args)...) {}
912+
934913
template <class _Func, class... _Args>
935914
_LIBCPP_HIDE_FROM_ABI constexpr explicit __union_t(
936915
std::__expected_construct_in_place_from_invoke_tag, _Func&& __f, _Args&&... __args)
@@ -955,6 +934,18 @@ class expected {
955934
_LIBCPP_NO_UNIQUE_ADDRESS _ErrorType __unex_;
956935
};
957936

937+
template <class _Up, class _OtherErr>
938+
_LIBCPP_HIDE_FROM_ABI constexpr __union_t<_Tp, _Err> __union_from_expected(const expected<_Up, _OtherErr>& __other) {
939+
return __other.__has_val_ ? __union_t<_Tp, _Err>(__construct_in_place_tag{}, __other.__union_.__val_)
940+
: __union_t<_Tp, _Err>(__construct_unexpected_tag{}, __other.__union_.__unex_);
941+
}
942+
943+
template <class _Up, class _OtherErr>
944+
_LIBCPP_HIDE_FROM_ABI constexpr __union_t<_Tp, _Err> __union_from_expected(expected<_Up, _OtherErr>&& __other) {
945+
return __other.__has_val_ ? __union_t<_Tp, _Err>(__construct_in_place_tag{}, std::move(__other.__union_.__val_))
946+
: __union_t<_Tp, _Err>(__construct_unexpected_tag{}, std::move(__other.__union_.__unex_));
947+
}
948+
958949
_LIBCPP_NO_UNIQUE_ADDRESS __union_t<_Tp, _Err> __union_;
959950
bool __has_val_;
960951
};
@@ -998,11 +989,7 @@ class expected<_Tp, _Err> {
998989
_LIBCPP_HIDE_FROM_ABI constexpr expected(const expected& __rhs)
999990
noexcept(is_nothrow_copy_constructible_v<_Err>) // strengthened
1000991
requires(is_copy_constructible_v<_Err> && !is_trivially_copy_constructible_v<_Err>)
1001-
: __has_val_(__rhs.__has_val_) {
1002-
if (!__rhs.__has_val_) {
1003-
std::construct_at(std::addressof(__union_.__unex_), __rhs.__union_.__unex_);
1004-
}
1005-
}
992+
: __union_(__union_from_expected(__rhs)), __has_val_(__rhs.__has_val_) {}
1006993

1007994
_LIBCPP_HIDE_FROM_ABI constexpr expected(expected&&)
1008995
requires(is_move_constructible_v<_Err> && is_trivially_move_constructible_v<_Err>)
@@ -1011,69 +998,49 @@ class expected<_Tp, _Err> {
1011998
_LIBCPP_HIDE_FROM_ABI constexpr expected(expected&& __rhs)
1012999
noexcept(is_nothrow_move_constructible_v<_Err>)
10131000
requires(is_move_constructible_v<_Err> && !is_trivially_move_constructible_v<_Err>)
1014-
: __has_val_(__rhs.__has_val_) {
1015-
if (!__rhs.__has_val_) {
1016-
std::construct_at(std::addressof(__union_.__unex_), std::move(__rhs.__union_.__unex_));
1017-
}
1018-
}
1001+
: __union_(__union_from_expected(std::move(__rhs))), __has_val_(__rhs.__has_val_) {}
10191002

10201003
template <class _Up, class _OtherErr>
10211004
requires __can_convert<_Up, _OtherErr, const _OtherErr&>::value
10221005
_LIBCPP_HIDE_FROM_ABI constexpr explicit(!is_convertible_v<const _OtherErr&, _Err>)
10231006
expected(const expected<_Up, _OtherErr>& __rhs)
10241007
noexcept(is_nothrow_constructible_v<_Err, const _OtherErr&>) // strengthened
1025-
: __has_val_(__rhs.__has_val_) {
1026-
if (!__rhs.__has_val_) {
1027-
std::construct_at(std::addressof(__union_.__unex_), __rhs.__union_.__unex_);
1028-
}
1029-
}
1008+
: __union_(__union_from_expected(__rhs)), __has_val_(__rhs.__has_val_) {}
10301009

10311010
template <class _Up, class _OtherErr>
10321011
requires __can_convert<_Up, _OtherErr, _OtherErr>::value
10331012
_LIBCPP_HIDE_FROM_ABI constexpr explicit(!is_convertible_v<_OtherErr, _Err>)
10341013
expected(expected<_Up, _OtherErr>&& __rhs)
10351014
noexcept(is_nothrow_constructible_v<_Err, _OtherErr>) // strengthened
1036-
: __has_val_(__rhs.__has_val_) {
1037-
if (!__rhs.__has_val_) {
1038-
std::construct_at(std::addressof(__union_.__unex_), std::move(__rhs.__union_.__unex_));
1039-
}
1040-
}
1015+
: __union_(__union_from_expected(std::move(__rhs))), __has_val_(__rhs.__has_val_) {}
10411016

10421017
template <class _OtherErr>
10431018
requires is_constructible_v<_Err, const _OtherErr&>
10441019
_LIBCPP_HIDE_FROM_ABI constexpr explicit(!is_convertible_v<const _OtherErr&, _Err>)
10451020
expected(const unexpected<_OtherErr>& __unex)
10461021
noexcept(is_nothrow_constructible_v<_Err, const _OtherErr&>) // strengthened
1047-
: __has_val_(false) {
1048-
std::construct_at(std::addressof(__union_.__unex_), __unex.error());
1049-
}
1022+
: __union_(__construct_unexpected_tag{}, __unex.error()), __has_val_(false) {}
10501023

10511024
template <class _OtherErr>
10521025
requires is_constructible_v<_Err, _OtherErr>
10531026
_LIBCPP_HIDE_FROM_ABI constexpr explicit(!is_convertible_v<_OtherErr, _Err>)
10541027
expected(unexpected<_OtherErr>&& __unex)
10551028
noexcept(is_nothrow_constructible_v<_Err, _OtherErr>) // strengthened
1056-
: __has_val_(false) {
1057-
std::construct_at(std::addressof(__union_.__unex_), std::move(__unex.error()));
1058-
}
1029+
: __union_(__construct_unexpected_tag{}, std::move(__unex.error())), __has_val_(false) {}
10591030

10601031
_LIBCPP_HIDE_FROM_ABI constexpr explicit expected(in_place_t) noexcept : __has_val_(true) {}
10611032

10621033
template <class... _Args>
10631034
requires is_constructible_v<_Err, _Args...>
10641035
_LIBCPP_HIDE_FROM_ABI constexpr explicit expected(unexpect_t, _Args&&... __args)
10651036
noexcept(is_nothrow_constructible_v<_Err, _Args...>) // strengthened
1066-
: __has_val_(false) {
1067-
std::construct_at(std::addressof(__union_.__unex_), std::forward<_Args>(__args)...);
1068-
}
1037+
: __union_(__construct_unexpected_tag{}, std::forward<_Args>(__args)...), __has_val_(false) {}
10691038

10701039
template <class _Up, class... _Args>
10711040
requires is_constructible_v< _Err, initializer_list<_Up>&, _Args... >
10721041
_LIBCPP_HIDE_FROM_ABI constexpr explicit expected(unexpect_t, initializer_list<_Up> __il, _Args&&... __args)
10731042
noexcept(is_nothrow_constructible_v<_Err, initializer_list<_Up>&, _Args...>) // strengthened
1074-
: __has_val_(false) {
1075-
std::construct_at(std::addressof(__union_.__unex_), __il, std::forward<_Args>(__args)...);
1076-
}
1043+
: __union_(__construct_unexpected_tag{}, __il, std::forward<_Args>(__args)...), __has_val_(false) {}
10771044

10781045
private:
10791046
template <class _Func>
@@ -1502,11 +1469,16 @@ class expected<_Tp, _Err> {
15021469

15031470
private:
15041471
struct __empty_t {};
1472+
struct __construct_unexpected_tag {};
15051473

15061474
template <class _ErrorType>
15071475
union __union_t {
15081476
_LIBCPP_HIDE_FROM_ABI constexpr __union_t() : __empty_() {}
15091477

1478+
template <class... _Args>
1479+
_LIBCPP_HIDE_FROM_ABI constexpr explicit __union_t(__construct_unexpected_tag, _Args&&... __args)
1480+
: __unex_(std::forward<_Args>(__args)...) {}
1481+
15101482
template <class _Func, class... _Args>
15111483
_LIBCPP_HIDE_FROM_ABI constexpr explicit __union_t(
15121484
__expected_construct_unexpected_from_invoke_tag, _Func&& __f, _Args&&... __args)
@@ -1534,6 +1506,10 @@ class expected<_Tp, _Err> {
15341506
_LIBCPP_HIDE_FROM_ABI constexpr __union_t(const __union_t&) = default;
15351507
_LIBCPP_HIDE_FROM_ABI constexpr __union_t& operator=(const __union_t&) = default;
15361508

1509+
template <class... _Args>
1510+
_LIBCPP_HIDE_FROM_ABI constexpr explicit __union_t(__construct_unexpected_tag, _Args&&... __args)
1511+
: __unex_(std::forward<_Args>(__args)...) {}
1512+
15371513
template <class _Func, class... _Args>
15381514
_LIBCPP_HIDE_FROM_ABI constexpr explicit __union_t(
15391515
__expected_construct_unexpected_from_invoke_tag, _Func&& __f, _Args&&... __args)
@@ -1552,6 +1528,18 @@ class expected<_Tp, _Err> {
15521528
_LIBCPP_NO_UNIQUE_ADDRESS _ErrorType __unex_;
15531529
};
15541530

1531+
template <class _Up, class _OtherErr>
1532+
_LIBCPP_HIDE_FROM_ABI constexpr __union_t<_Err> __union_from_expected(const expected<_Up, _OtherErr>& __other) {
1533+
return __other.__has_val_ ? __union_t<_Err>()
1534+
: __union_t<_Err>(__construct_unexpected_tag{}, __other.__union_.__unex_);
1535+
}
1536+
1537+
template <class _Up, class _OtherErr>
1538+
_LIBCPP_HIDE_FROM_ABI constexpr __union_t<_Err> __union_from_expected(expected<_Up, _OtherErr>&& __other) {
1539+
return __other.__has_val_ ? __union_t<_Err>()
1540+
: __union_t<_Err>(__construct_unexpected_tag{}, std::move(__other.__union_.__unex_));
1541+
}
1542+
15551543
_LIBCPP_NO_UNIQUE_ADDRESS __union_t<_Err> __union_;
15561544
bool __has_val_;
15571545
};

0 commit comments

Comments
 (0)