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

[jiixyj]

In some situations it seems that the "has value" flag of a std::expected gets overwritten when the value is constructed. Example:

#undef NDEBUG
#include <cassert>

#include <expected>
#include <optional>

std::expected<std::optional<int>, long> f1()
{
    return 0;
}

std::expected<std::optional<int>, int> f2()
{
    return f1().transform_error([](auto) { return 0; });
}

int main()
{
    auto r = f2();
    assert(r.has_value());

Godbolt: https://godbolt.org/z/nbcPr9e5f

I would expect that the assert is not triggered, but it is. I'm testing with commit d408770.

I strongly suspect code like this, i.e. where the call to std::construct_at happens after setting the "has value" flag:

llvm-project/libcxx/include/__expected/expected.h

Lines 252 to 258 in 7cc1bfa

	template <class... _Args>
	requires is_constructible_v<_Tp, _Args...>
	_LIBCPP_HIDE_FROM_ABI constexpr explicit expected(in_place_t, _Args&&... __args)
	noexcept(is_nothrow_constructible_v<_Tp, _Args...>) // strengthened
	: __has_val_(true) {
	std::construct_at(std::addressof(__union_.__val_), std::forward<_Args>(__args)...);
	}

In the above example, __has_val_ (a bool) and __union_.__val_ (the std::optional<int>) overlap. I think std::construct_at is allowed to overwrite __has_val_ in this case by the language rules as the std::construct_at has no idea that the __has_val_ is there. It just assumes it has full 8 bytes to work with to construct the std::optional<int>.

The data layout of the problematic std::expected<std::optional<int>, int> looks like this:

            +-- optional "has value" flag
            |     +--+--padding
/---int---\ |     |  |
00 00 00 00 01 00 00 00
               |
               |
               +- expected "has value" flag

The expected's "has value" flag reuses one byte of the padding. It should be "1" in this case but is "0".

Suggested fix: move all assignments to the "has value" flag after the std::construct_at calls like this:

  template <class... _Args>
    requires is_constructible_v<_Tp, _Args...>
  _LIBCPP_HIDE_FROM_ABI constexpr explicit expected(in_place_t, _Args&&... __args)
    noexcept(is_nothrow_constructible_v<_Tp, _Args...>) // strengthened
  {
    std::construct_at(std::addressof(__union_.__val_), std::forward<_Args>(__args)...);
    __has_val = true;
  }

I wonder if this analysis is correct or if I'm missing some subtleties with std::construct_at.

[frederick-vs-ja]

It seems to me that the current implementation stategy triggers core language undefined behavior, although the related CWG issue is still open (CWG2757).

libc++ is making several subobjects potentially-overlapping by _LIBCPP_NO_UNIQUE_ADDRESS, so the std::construct_at call doesn't transparently replaces __union_.__val_ and thus reuses the storage of the whole expected ([basic.life]/8).

A safe resolution may be just removing _LIBCPP_NO_UNIQUE_ADDRESS...

[ldionne]

@huixie90 Can you take a look at this? It seems fairly important to fix this.

[jiixyj]

I created a PR for a possible fix: #68733

It solves my problem at least. I also added a test (more or less the reproducer from above), although this is really hard to test for properly. Sanitizers (asan, ubsan) couldn't catch this in my (admittedly limited) testing. Even valgrind didn't catch this.

[jiixyj]

libc++ is making several subobjects potentially-overlapping by _LIBCPP_NO_UNIQUE_ADDRESS, so the std::construct_at call doesn't transparently replaces __union_.__val_ and thus reuses the storage of the whole expected ([basic.life]/8).

A safe resolution may be just removing _LIBCPP_NO_UNIQUE_ADDRESS...

I wonder if this note then applies for libc++'s std::expected implementation:

[Note 5: If these conditions are not met, a pointer to the new object can be obtained from a pointer that represents the address of its storage by calling std​::​launder ([ptr.launder]).
— end note]

Does this mean that there needs to be std::launder sprinkled over the current std::expected implementation?

[huixie90]

@huixie90 Can you take a look at this? It seems fairly important to fix this.

sure I apologies for the delay and I am reviewing the PR that @jiixyj created

[ldionne]

In case some folks are watching this but not the PR (or in case someone sees this a long time from now and wants to see more details), there's been a lot of interesting discussion on the PR thread: #68733