[libc++] `std::expected`: operations may overwrite bytes of unrelated objects

[jiixyj]

Some operations, like emplace(), might overwrite bytes of unrelated objects (example from #68733 (comment)):

struct bool_with_padding {
    alignas(8) bool b = false;
};

struct s : std::expected<bool_with_padding, bool> {
    const bool please_dont_overwrite_me[6] =  //
        {true, true, true, true, true, true};
};

static_assert(sizeof(s) == sizeof(bool_with_padding));

int main() {
    s s;

    s.emplace();

    assert(s.please_dont_overwrite_me[5]); // will abort
    assert(s.please_dont_overwrite_me[4]);
    assert(s.please_dont_overwrite_me[3]);
    assert(s.please_dont_overwrite_me[2]);
    assert(s.please_dont_overwrite_me[1]);
    assert(s.please_dont_overwrite_me[0]);
}

This happens because currently, std::__libcppdatasize<std::expected<bool_with_padding, bool>>::value == 2: One byte because of std::__libcppdatasize<bool_with_padding>::value == 1 and one additional byte for expected's "has value flag.

However, the emplace() operation destructs/constructs another bool_with_padding in place -- and that will overwrite all sizeof(bool_with_padding) == 8 bytes of the object, clobbering the six unrelated bools that are stored in expected's tail padding.

std::expected has to make sure that all of its tail padding (if it has some) cannot be touched by any destructor/constructor calls of the value/error.

There is a PR up that has a work in progress fix and some more discussion: #69673

[ldionne]

@huixie90 and I just spent some time going through this issue again while working on #70506, which is basically the same issue but for __movable_box. We currently have two questions related to fixing this issue.

First, is the following code valid?

#include <cstring>

struct Empty {
    void zero() { std::memset(this, 0, sizeof(*this)); }
};

struct CompressedPair : Empty {
    CompressedPair(Empty e, bool x) : Empty(e), b(x) { }
    bool b;
};

void f(CompressedPair& p) {
    p.zero(); // this also overrides the bool member of the compressed pair
}

If this code is valid, the EBO is arguably broken. One the one hand std::memset(this, 0, sizeof(*this)) is super fishy, but if a type is allowed to take for granted that it owns its padding bytes, then this should be valid (unless prohibited by some other rule)?

Second question: Is the following code valid?

#include <memory>
#include <new>

struct Foo {
    Foo(int i, bool b) : i_(i), b_(b) { }

    Foo& operator=(Foo const& other) {
        std::destroy_at(this);
        std::construct_at(this, other.i_, other.b_);
        return *std::launder(this);
    }

    int i_;
    bool b_;
};

This is clearly not a great way to implement operator=. But if this is legal, then we basically can't use [[no_unique_address]] whenever we don't know exactly what type we're given, since calling operator= of that type would overwrite the next object that sits in its padding. Note that this is not limited to assignment -- any other member function has the same problem.

This is very similar to the case brought up by @huixie90 in #68733 (comment):

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

There, @jwakely had said that it wasn't (in his opinion) something we want to support, and I agree. However, while the use case immediately above is hard to defend, something like an assignment operator using a destroy-and-construct pattern is probably much more common (although I don't know yet if it's technically legal). If it is legal, then I suspect we can't use [[no_unique_address]] at all in our generic types, which would be a bummer.

I'm going to CC some folks here who might have knowledge/opinions about how this should be handled, sorry if this ends up being a bit spammy but we need to statute on this before we can make progress with either this issue or #70506.

CC @jiixyj @huixie90 @philnik777 @frederick-vs-ja @zygoloid @jwakely

Edit: Added std::launder in question 2 as required by https://eel.is/c++draft/basic.life#note-5

[ldionne]

@huixie90 and I went over https://eel.is/c++draft/basic.life#8, and it seems to say that the answer to question 2 is "Yes, this is valid". If that's true, then we can kiss the [[no_unique_address]] optimizations goodbye. Or, alternatively, we could change something in the Standard to give us that kind of freedom.

For now, I'm hoping we read https://eel.is/c++draft/basic.life#8 wrong but I am looking forward to hearing what other people think.

[philnik777]

@huixie90 and I just spent some time going through this issue again while working on #70506, which is basically the same issue but for __movable_box. We currently have two questions related to fixing this issue.

First, is the following code valid?

#include <cstring>

struct Empty {
    void zero() { std::memset(this, 0, sizeof(*this)); }
};

struct CompressedPair : Empty {
    CompressedPair(Empty e, bool x) : Empty(e), b(x) { }
    bool b;
};

void f(CompressedPair& p) {
    p.zero(); // this also overrides the bool member of the compressed pair
}

If this code is valid, the EBO is arguably broken. One the one hand std::memset(this, 0, sizeof(*this)) is super fishy, but if a type is allowed to take for granted that it owns its padding bytes, then this should be valid (unless prohibited by some other rule)?

I'm pretty sure this is UB. The standard seems to specifically exclude potentially-overlapping subobjects from any guarantees it makes regarding the value representation of objects. The thing you're most likely trying to do here is ending the lifetime of Empty and starting the lifetime of a new one by memsetting the memory. The problem is that Empty is potentially overlapping in this context, so it's not transparently replaceable (http://eel.is/c++draft/basic.life#8). This means that you're not just ending the lifetime of Empty, but also of CompressedPair.

Second question: Is the following code valid?

#include <memory>
#include <new>

struct Foo {
    Foo(int i, bool b) : i_(i), b_(b) { }

    Foo& operator=(Foo const& other) {
        std::destroy_at(this);
        std::construct_at(this, other.i_, other.b_);
        return *std::launder(this);
    }

    int i_;
    bool b_;
};

I'd say no, for the same reason outlined above.