[libc++] Fix UB in <expected> related to "has value" flag (#68552) #68733

jiixyj · 2023-10-10T18:33:24Z

The calls to std::construct_at might overwrite the previously set __has_value_ flag, so reverse the order everywhere.

Fixes: #68552

libcxx/test/std/utilities/expected/expected.expected/observers/has_value_clobber.pass.cpp

github-actions · 2023-10-10T18:44:13Z

✅ With the latest revision this PR passed the C/C++ code formatter.

philnik777 · 2023-10-10T18:52:49Z

Thanks for working on this!
I think it would be better to move the initialization into the union, since that would make sure we're not doing anything crazy (since this would enforce the initializtation order). Right now, this looks quite brittle to me. Would you be willing to try that out?

@ldionne This looks like a case where we want to have an optimized test run - especially, since none of the static analyzers or sanitizers caught this.

jiixyj · 2023-10-10T20:55:03Z

Thanks for working on this! I think it would be better to move the initialization into the union, since that would make sure we're not doing anything crazy (since this would enforce the initializtation order). Right now, this looks quite brittle to me. Would you be willing to try that out?

Sure, I can try! Are you thinking of something like this?

  _LIBCPP_HIDE_FROM_ABI constexpr expected(const expected& __other) noexcept(
      is_nothrow_copy_constructible_v<_Tp> && is_nothrow_copy_constructible_v<_Err>) // strengthened
    requires(is_copy_constructible_v<_Tp> && is_copy_constructible_v<_Err> &&
             !(is_trivially_copy_constructible_v<_Tp> && is_trivially_copy_constructible_v<_Err>))
      : __union_([&] {
          return __other.__has_val_ ? __union_t<_Tp, _Err>(__construct_val_tag{}, __other.__union_.__val_)
                                    : __union_t<_Tp, _Err>(__construct_unex_tag{}, __other.__union_.__unex_);
        }()),
        __has_val_(__other.__has_val_) {}

...i.e. using the expected's constructor initializer list to enforce the order? Of course, that wouldn't help for the assignment and swap operators, but those tend to be tricky anyway...

philnik777 · 2023-10-10T21:24:45Z

Yes, something like that was on my mind. My only nit is that we might want to add a few static member functions to avoid a bunch of logic inside the initializer.

ldionne · 2023-10-10T23:44:39Z

@ldionne This looks like a case where we want to have an optimized test run - especially, since none of the static analyzers or sanitizers caught this.

See #68753.

libcxx/test/std/utilities/expected/expected.expected/observers/has_value_clobber.pass.cpp

EricWF · 2023-10-11T13:33:21Z

The calls to std::construct_at might overwrite the previously set __has_value_ flag, so reverse the order everywhere.

~~You're saying that constructing the value in the expected might change the value of __has_value_? I don't understand.~~

Nevermind, this is fudging terrifying. I didn't realize [[no_unique_address]] could have such large foot-guns aimed at our face.

jiixyj · 2023-10-11T19:40:31Z

Yes, something like that was on my mind. My only nit is that we might want to add a few static member functions to avoid a bunch of logic inside the initializer.

I followed your advice and updated the PR, please have a look :)

huixie90

Thanks for spotting the issue and send the PR. I am still a bit nervous about object overlapping. Since expected can expose its data through several members like operator*, operator->, value(), error(), etc.... Could users trigger this UB by other means that we can not stop from. like

std::expected<T, U> e = ...;
e->someMemberThatCouldWriteToObjectThatChangesHasValue()

or even,

std::expected<T, U> e = ...;
std::construct_at( std::address_of(*e), args...);

What do you think?

libcxx/include/__expected/expected.h

jiixyj · 2023-10-14T18:37:11Z

Thanks for spotting the issue and send the PR. I am still a bit nervous about object overlapping. Since expected can expose its data through several members like operator*, operator->, value(), error(), etc.... Could users trigger this UB by other means that we can not stop from. like
std::expected<T, U> e = ...;
e->someMemberThatCouldWriteToObjectThatChangesHasValue()

I guess someMemberThatCouldWriteToObjectThatChangesHasValue() itself is UB already, because every type must assume that there is something important stored in its tail padding.

or even,

std::expected<T, U> e = ...;
std::construct_at( std::address_of(*e), args...);

What do you think?

I now wonder if std::construct_at / placement new / the constructor is allowed to overwrite the tail padding. I initially assumed it was, but I just realized the following:

    static_assert(sizeof(std::expected<std::optional<int>, int>) == sizeof(std::optional<int>));
    static_assert(sizeof(std::expected<std::expected<std::optional<int>, int>, int>) == sizeof(std::optional<int>));
    static_assert(sizeof(std::expected<std::expected<std::expected<std::optional<int>, int>, int>, int>) == sizeof(std::optional<int>));

...i.e. that std::expected<std::optional<int>, int> still has 2 bytes of tail padding left that can themselves be used by two more "layers" of std::expected. So does the std::destruct_at/std::construct_at magic from the "innermost" std::expected need to ensure that all tail padding bytes are left alone? How would that look like? Or can the logic just assume that destructors/constructors won't touch the tail padding bytes?

Maybe the initial UB wasn't from the order of std::construct_at and the "has value" flag, but because of missing std::launder calls or something that allowed the optimizer to be a bit too aggressive?

huixie90

What if a user have the following

struct Foo {
 [[no_unique_address]] std::expected<std::optional<int>, int> x_;
 [[no_unique_address]] bool y_;
};

would the operations on the x_ overwrite y_? from what I understand it will

libcxx/include/__expected/expected.h

philnik777 · 2023-10-14T21:41:35Z

libcxx/test/std/utilities/expected/expected.expected/observers/has_value.pass.cpp

+struct tail_clobberer {
+  constexpr tail_clobberer() {
+    if (!std::is_constant_evaluated()) {
+      // This `memset` might actually be UB (?) but suffices to reproduce bugs


I think this should be fine. IIUC the type is allowed to overwrite padding bytes during construction.

libcxx/test/std/utilities/expected/expected.expected/observers/has_value.pass.cpp

libcxx/include/__expected/expected.h

huixie90 · 2023-10-14T22:16:16Z

This is a simplified example.
https://godbolt.org/z/YGbG3PeYY

I don't think we can fix it. It is a mistake to use [[no_unique_address]] I think. I think there is no way to fix without breaking ABI now.

philnik777 · 2023-10-14T22:37:19Z

This is a simplified example. https://godbolt.org/z/YGbG3PeYY

I don't think we can fix it. It is a mistake to use [[no_unique_address]] I think. I think there is no way to fix without breaking ABI now.

I think I agree. It's probably still early enough to fix the ABI (especially given that these bugs are pretty severe). In LLVM 16 C++23 features were still experimental and we can backport this to LLVM 17. @ldionne do you agree? Should we add an ABI tag? Is this severe enough to warrant a 17.1 release? (CC @tru @tstellar)

I think we can still put at least __has_val_ into the padding bits, since we control that. It should be good enough to add a char __padding_[sizeof(expected) - __libcpp_datasizeof<expected>::value]; to avoid accidental overwrites of user data.

jiixyj · 2023-10-15T07:10:09Z

I think we can still put at least __has_val_ into the padding bits, since we control that. It should be good enough to add a char __padding_[sizeof(expected) - __libcpp_datasizeof<expected>::value]; to avoid accidental overwrites of user data.

Ah, so then __has_val_ would always be the last byte of the expected, so expected itself would never have any tail padding? I think this is a great idea!

As an alternative, I wonder if the current ABI is salvageable by saving/restoring tail padding in all "dangerous" functions (i.e. swap, emplace, assign, etc.) with something like:

  struct __tail_padding_saver {
    static constexpr auto __padding_offset = __libcpp_datasizeof<expected>::value;
    static constexpr auto __padding_size = sizeof(expected) - __padding_offset;

    expected *__e;
    unsigned char __pad[__padding_size];

    explicit __tail_padding_saver(expected *__e) : __e(__e) {
      __builtin_memcpy(__pad, reinterpret_cast<unsigned char *>(__e) + __padding_offset, __padding_size);
    }

    ~__tail_padding_saver() {
      __builtin_memcpy(reinterpret_cast<unsigned char *>(__e) + __padding_offset, __pad, __padding_size);
    }
  };

Or even have some helper functions

construct_at_until(std::addressof(__union_.__val_), &__has_val_, std::forward<_Args>(__args)...);
destroy_at_until(std::addressof(__union_.__val_), &__has_val_, std::forward<_Args>(__args)...);

...that save/restore all needed bytes "under the hood"?

huixie90 · 2023-10-15T07:32:22Z

I think we can still put at least __has_val_ into the padding bits, since we control that. It should be good enough to add a char __padding_[sizeof(expected) - __libcpp_datasizeof<expected>::value]; to avoid accidental overwrites of user data.

Ah, so then __has_val_ would always be the last byte of the expected, so expected itself would never have any tail padding? I think this is a great idea!

As an alternative, I wonder if the current ABI is salvageable by saving/restoring tail padding in all "dangerous" functions (i.e. swap, emplace, assign, etc.) with something like:
  struct __tail_padding_saver {
    static constexpr auto __padding_offset = __libcpp_datasizeof<expected>::value;
    static constexpr auto __padding_size = sizeof(expected) - __padding_offset;

    expected *__e;
    unsigned char __pad[__padding_size];

    explicit __tail_padding_saver(expected *__e) : __e(__e) {
      __builtin_memcpy(__pad, reinterpret_cast<unsigned char *>(__e) + __padding_offset, __padding_size);
    }

    ~__tail_padding_saver() {
      __builtin_memcpy(reinterpret_cast<unsigned char *>(__e) + __padding_offset, __pad, __padding_size);
    }
  };
Or even have some helper functions
construct_at_until(std::addressof(__union_.__val_), &__has_val_, std::forward<_Args>(__args)...);
destroy_at_until(std::addressof(__union_.__val_), &__has_val_, std::forward<_Args>(__args)...);
...that save/restore all needed bytes "under the hood"?

I think we are simply talking about

template <class T, class E>
class expected {
[[no_unique_address]] union_t union_;
[[no_unique_address]] bool has_value_;
char padding_[padding_size];
};

so that has_value_ can still goes into the tailing padding of T and also never going to write over beyond expected if the user has

struct User {
  [[no_unique_address]] expected<T, E> _x;
  [[no_unique_address]] bool _y;
};

I think this fixes the problem but break the ABI. Another point is that if we are going to break the ABI anyway, maybe another option is to get rid of no_unique_address and does what exactly stated in the spec. (The spec uses anonymous union and we used named union (in order to have a type to apply no_unique_address ).

philnik777 · 2023-10-15T13:05:17Z

I think we can still put at least __has_val_ into the padding bits, since we control that. It should be good enough to add a char __padding_[sizeof(expected) - __libcpp_datasizeof<expected>::value]; to avoid accidental overwrites of user data.

Ah, so then __has_val_ would always be the last byte of the expected, so expected itself would never have any tail padding? I think this is a great idea!
As an alternative, I wonder if the current ABI is salvageable by saving/restoring tail padding in all "dangerous" functions (i.e. swap, emplace, assign, etc.) with something like:
  struct __tail_padding_saver {
    static constexpr auto __padding_offset = __libcpp_datasizeof<expected>::value;
    static constexpr auto __padding_size = sizeof(expected) - __padding_offset;

    expected *__e;
    unsigned char __pad[__padding_size];

    explicit __tail_padding_saver(expected *__e) : __e(__e) {
      __builtin_memcpy(__pad, reinterpret_cast<unsigned char *>(__e) + __padding_offset, __padding_size);
    }

    ~__tail_padding_saver() {
      __builtin_memcpy(reinterpret_cast<unsigned char *>(__e) + __padding_offset, __pad, __padding_size);
    }
  };
Or even have some helper functions
construct_at_until(std::addressof(__union_.__val_), &__has_val_, std::forward<_Args>(__args)...);
destroy_at_until(std::addressof(__union_.__val_), &__has_val_, std::forward<_Args>(__args)...);
...that save/restore all needed bytes "under the hood"?
I think we are simply talking about
template <class T, class E>
class expected {
[[no_unique_address]] union_t union_;
[[no_unique_address]] bool has_value_;
char padding_[padding_size];
};
so that has_value_ can still goes into the tailing padding of T and also never going to write over beyond expected if the user has
struct User {
  [[no_unique_address]] expected<T, E> _x;
  [[no_unique_address]] bool _y;
};
I think this fixes the problem but break the ABI. Another point is that if we are going to break the ABI anyway, maybe another option is to get rid of no_unique_address and does what exactly stated in the spec. (The spec uses anonymous union and we used named union (in order to have a type to apply no_unique_address ).

We could get rid of [[no_unique_address]] completely, but IMO it's a pretty nice property that expected<optional<int>, int> or similar constructs still fit in a register. I would expect that std::expected is used mostly for return types, so being able to squeeze our flag byte into the stored object would be really good.

jiixyj · 2023-10-15T15:00:31Z

As an alternative, I wonder if the current ABI is salvageable by saving/restoring tail padding in all "dangerous" functions (i.e. swap, emplace, assign, etc.) with something like:

[...]

I tried something similar on a separate branch as a POC: jiixyj@86fe890#diff-99bc4a85cb97f33ff277458a531ca47376ceba6ea7d84329cabf02857941ed3eR976

All std::construct_at/std::destroy_at calls are funneled through helper functions that save/restore just the minimum amount of bytes that might be overwritten by the constructor/destructor, including the __has_value_ flag.

Could that be a way out of this?

huixie90 · 2023-10-15T17:57:15Z

As an alternative, I wonder if the current ABI is salvageable by saving/restoring tail padding in all "dangerous" functions (i.e. swap, emplace, assign, etc.) with something like:
[...]

I tried something similar on a separate branch as a POC: jiixyj@86fe890#diff-99bc4a85cb97f33ff277458a531ca47376ceba6ea7d84329cabf02857941ed3eR976

All std::construct_at/std::destroy_at calls are funneled through helper functions that save/restore just the minimum amount of bytes that might be overwritten by the constructor/destructor, including the __has_value_ flag.

Could that be a way out of this?

But I guess it won't work in constant expression and it does not work in multithreading cases as the memory is changed then changed it back later

jiixyj · 2023-10-15T19:22:37Z

All std::construct_at/std::destroy_at calls are funneled through helper functions that save/restore just the minimum amount of bytes that might be overwritten by the constructor/destructor, including the __has_value_ flag.
Could that be a way out of this?

But I guess it won't work in constant expression and it does not work in multithreading cases as the memory is changed then changed it back later

Ah yes, the multithreading issues might be a deal breaker for this approach :/

jiixyj · 2023-10-15T20:20:41Z

I implemented the proposed (ABI changing) fix here and all tests pass! With that patch expected has no longer any tail padding, so std::construct_at/std::destroy_at wouldn't be able to overwrite any important data (except for __has_value_, but that is always restored correctly).

huixie90 · 2023-10-16T20:45:36Z

I implemented the proposed (ABI changing) fix here and all tests pass! With that patch expected has no longer any tail padding, so std::construct_at/std::destroy_at wouldn't be able to overwrite any important data (except for __has_value_, but that is always restored correctly).

Is the following valid user code? If so, we will still have a problem

std::expected<T, E> e = ...;
T& val = *e;
val.~T();
std::construct_at(&val, args...); // this still overwrites has_value

…TailClobberer

libcxx/test/std/utilities/expected/expected.expected/swap/free.swap.pass.cpp

libcxx/test/std/utilities/expected/expected.expected/swap/member.swap.pass.cpp

jiixyj · 2023-10-30T14:38:41Z

@ldionne : I think I addressed all review comments -- thanks again to all reviewers! I have added some "TailClobberer" tests for swap() now as well.

@huixie90 : There is an unresolved conversation/comment from you here about the design of the "TailClobberer" class. Do you think we can leave it like it is in this PR and do further test refactoring, if needed, in followup PRs?

ldionne

Thanks for all the work on this!

This one is actually not an ABI break, so I would cherry-pick it back to LLVM 17 regardless of what we end up doing with the tail padding issue (which is the actual ABI breaking issue). I can handle that.

libcxx/test/std/utilities/expected/expected.expected/swap/free.swap.pass.cpp

ldionne · 2023-10-30T18:52:47Z

@huixie90 : There is an unresolved conversation/comment from you here about the design of the "TailClobberer" class. Do you think we can leave it like it is in this PR and do further test refactoring, if needed, in followup PRs?

Given the tight time constraints, I think this is fine.

…68733) The calls to std::construct_at might overwrite the previously set __has_value_ flag in the case where the flag is overlapping with the actual value or error being stored (since we use [[no_unique_address]]). To fix this issue, this patch ensures that we initialize the __has_value_ flag after we call std::construct_at. Fixes #68552 (cherry picked from commit 134c915)

jiixyj requested a review from a team as a code owner October 10, 2023 18:33

llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Oct 10, 2023

jiixyj mentioned this pull request Oct 10, 2023

[libc++] std::expected "has value" flag getting clobbered unexpectedly #68552

Closed

jiixyj commented Oct 10, 2023

View reviewed changes

libcxx/test/std/utilities/expected/expected.expected/observers/has_value_clobber.pass.cpp Outdated Show resolved Hide resolved

jiixyj force-pushed the expected-ub branch from 779aedd to 13b037a Compare October 10, 2023 18:46

ldionne requested changes Oct 10, 2023

View reviewed changes

jiixyj force-pushed the expected-ub branch 2 times, most recently from b4d2dec to 9832a97 Compare October 11, 2023 18:42

jiixyj requested a review from ldionne October 11, 2023 19:54

huixie90 reviewed Oct 14, 2023

View reviewed changes

libcxx/include/__expected/expected.h Outdated Show resolved Hide resolved

huixie90 reviewed Oct 14, 2023

View reviewed changes

libcxx/include/__expected/expected.h Outdated Show resolved Hide resolved

libcxx/include/__expected/expected.h Show resolved Hide resolved

philnik777 reviewed Oct 14, 2023

View reviewed changes

jiixyj added 11 commits October 30, 2023 11:14

add TailClobberer tests for expected.expected

07e0258

add some missing checks

4471615

add tests for expected<void>

dd7b119

fix compile errors

5f5fab0

remove declaration of local Except struct

ebaccf9

remove more declarations of local Except structs

c54292e

just return a reference to the union value in emplace()

737f6dd

use LIBCPP_STATIC_ASSERT to check size of std::expected containing a …

63e4c45

…TailClobberer

add swap tests involving TailClobberer

d5c5626

drive by fix: these should be free swaps

8e866c6

make both member.swap.pass.cpp files clang-format friendly

c637421

jiixyj force-pushed the expected-ub branch from a900383 to c637421 Compare October 30, 2023 10:15

jiixyj commented Oct 30, 2023

View reviewed changes

libcxx/test/std/utilities/expected/expected.expected/swap/free.swap.pass.cpp Show resolved Hide resolved

jiixyj commented Oct 30, 2023

View reviewed changes

libcxx/test/std/utilities/expected/expected.expected/swap/member.swap.pass.cpp Show resolved Hide resolved

jiixyj added 2 commits October 30, 2023 11:42

try to avoid -Werror=terminate error on GCC

19cfd8a

use if constexpr

05f7c85

ldionne approved these changes Oct 30, 2023

View reviewed changes

libcxx/test/std/utilities/expected/expected.expected/swap/free.swap.pass.cpp Show resolved Hide resolved

ldionne merged commit 134c915 into llvm:main Oct 30, 2023

ldionne mentioned this pull request Oct 30, 2023

[LLVM 17][libc++] Cherry-pick the fix for std::expected UB related to the has-value flag #70708

Closed

DanAlbert mentioned this pull request Oct 30, 2023

[Bug] potential upcoming ABI break in libc++ android/ndk#1963

Closed

ldionne mentioned this pull request Nov 1, 2023

[libc++] Vendor communication: upcoming ABI breaks in std::expected and parts of <ranges> #70820

Closed

ldionne added the ABI Application Binary Interface label Nov 30, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[libc++] Fix UB in <expected> related to "has value" flag (#68552) #68733

[libc++] Fix UB in <expected> related to "has value" flag (#68552) #68733

jiixyj commented Oct 10, 2023 •

edited

Loading

github-actions bot commented Oct 10, 2023 •

edited

Loading

philnik777 commented Oct 10, 2023

jiixyj commented Oct 10, 2023

philnik777 commented Oct 10, 2023

ldionne commented Oct 10, 2023

EricWF commented Oct 11, 2023 •

edited

Loading

jiixyj commented Oct 11, 2023

huixie90 left a comment

jiixyj commented Oct 14, 2023

huixie90 left a comment

philnik777 Oct 14, 2023

huixie90 commented Oct 14, 2023

philnik777 commented Oct 14, 2023

jiixyj commented Oct 15, 2023

huixie90 commented Oct 15, 2023

philnik777 commented Oct 15, 2023

jiixyj commented Oct 15, 2023

huixie90 commented Oct 15, 2023

jiixyj commented Oct 15, 2023

jiixyj commented Oct 15, 2023

huixie90 commented Oct 16, 2023

jiixyj commented Oct 30, 2023

ldionne left a comment

ldionne commented Oct 30, 2023

[libc++] Fix UB in <expected> related to "has value" flag (#68552) #68733

[libc++] Fix UB in <expected> related to "has value" flag (#68552) #68733

Conversation

jiixyj commented Oct 10, 2023 • edited Loading

github-actions bot commented Oct 10, 2023 • edited Loading

philnik777 commented Oct 10, 2023

jiixyj commented Oct 10, 2023

philnik777 commented Oct 10, 2023

ldionne commented Oct 10, 2023

EricWF commented Oct 11, 2023 • edited Loading

jiixyj commented Oct 11, 2023

huixie90 left a comment

Choose a reason for hiding this comment

jiixyj commented Oct 14, 2023

huixie90 left a comment

Choose a reason for hiding this comment

philnik777 Oct 14, 2023

Choose a reason for hiding this comment

huixie90 commented Oct 14, 2023

philnik777 commented Oct 14, 2023

jiixyj commented Oct 15, 2023

huixie90 commented Oct 15, 2023

philnik777 commented Oct 15, 2023

jiixyj commented Oct 15, 2023

huixie90 commented Oct 15, 2023

jiixyj commented Oct 15, 2023

jiixyj commented Oct 15, 2023

huixie90 commented Oct 16, 2023

jiixyj commented Oct 30, 2023

ldionne left a comment

Choose a reason for hiding this comment

ldionne commented Oct 30, 2023

jiixyj commented Oct 10, 2023 •

edited

Loading

github-actions bot commented Oct 10, 2023 •

edited

Loading

EricWF commented Oct 11, 2023 •

edited

Loading