AK: Two small fixes to MaybeOwned

[kleinesfilmroellchen]

• Explicitly delete copy constructors (@nico just tripped over this)
• Simplify and optimize NonNullOwnPtr constructor

[kleinesfilmroellchen]

Is it worth adding a test? (Just a copy-only type that has a MaybeOwned as member should be sufficient, I think.)

Since it's a compile error, we can't really test it.

[nico]

In my case, I had something that didn't compile, but with this, it did.

But now that I've looked a bit more at my thing, I'm not sure the default move ctor here does what you usually want, for the MaybeOwned-contains-a-pointer case.

Say you have a

class Foo {
    MAKE_MOVE_ONLY();

    Foo() : bar(foo) {}

private:
    SomeType foo; // SomeType is move-only too
    MaybeOwned<SomeType> bar;
}

The default move ctor for this will move rhs.foo into this->foo, and it'll just copy the pointer in foo.bar to this->bar – but that pointer still points to the moved-from address of rhs.foo, not to this->foo where it probably should point now.

[kleinesfilmroellchen]

In my case, I had something that didn't compile, but with this, it did.

But now that I've looked a bit more at my thing, I'm not sure the default move ctor here does what you usually want, for the MaybeOwned-contains-a-pointer case.

Say you have a

class Foo {
    MAKE_MOVE_ONLY();

    Foo() : bar(foo) {}

private:
    SomeType foo; // SomeType is move-only too
    MaybeOwned<SomeType> bar;
}

The default move ctor for this will move rhs.foo into this->foo, and it'll just copy the pointer in foo.bar to this->bar – but that pointer still points to the moved-from address of rhs.foo, not to this->foo where it probably should point now.

I'd consider that code unsound, and therefore nothing we could or should handle. As far as I know, in C++, moving the underlying data of a reference (foo here) means that the reference is invalid, and using it is UB. We cannot detect that the data has moved within MaybeOwned; either we move bar before foo and the reference we just copied will soon become invalid, or we move foo before bar and the reference already is invalid.

The only solution here is to create a custom move constructor like so:

Foo(Foo&& other) : foo(move(other.foo)), bar(this->foo) {
}

[nico]

Completely agreed! But MaybeOwned's thing is that is's either a NNOP or a reference, so moving half of it is unsound. So maybe the type shouldn't be movable either? Or the move ctor should have a comment saying "NOTE: NOTE: NOTE: If this contains a ref, things will break", or a VERIFY(!has<ref>) in its body.

[timschumi]

But MaybeOwned's thing is that is's either a NNOP or a reference, so moving half of it is unsound.

On the risk that I completely misunderstand the discussion at hand:

NNOP also only stores a pointer. Moving around the NNOP doesn't change the location of the object, and neither does moving around the pointer.

The only thing where moving around the MaybeOwned would have a noticeable effect is if someone is holding onto a reference of the underlying NNOP itself (which they shouldn't be able to obtain in the first place). However, that is questionable code regardless of if MaybeOwned is involved or not.

[nico]

A NNOP will point to something on the heap. If you move something around, the address of the heap thing won't change. A reference can ref something on the stack, and those addresses change when moving around. Imagine a local var of type https://github.com/SerenityOS/serenity/pull/19031#issuecomment-1562050829 that you return-by-move from some function. I agree that that's questionable, but it'd be nice if questionable code was hard to write or would assert at runtime as much as possible.

[timschumi]

At this point we are talking about a general C++ footgun though, "moving an object makes references that point to it invalid". Unless there are any concrete suggestions on how to prevent this or to make it more obvious, I'd argue that this is not related to this PR.

As far as I can tell, both NNOP and MaybeOwned are reasonably safe against someone accident-ing themselves into such a situation. In fact, the main reason why your example looks that footgun-y is because initializing MaybeOwned like this both 'hides' the fact that we call a constructor explicitly and what type the passed foo has (a reference instead of being the object itself).

For the scope of MaybeOwned, the move constructor is correct. If other classes decide to use MaybeOwned incorrectly, in a way that MaybeOwned isn't designed to be used and that it quite literally cannot discern from a legitimate use-case, that's something the user class needs to protect against. For what it's worth, you'd have the same problem if you replace the MaybeOwned with a simple reference or pointer.

[nico]

The concrete suggestion from https://github.com/SerenityOS/serenity/pull/19031#issuecomment-1562901677 in some more detail:

    MaybeOwned(MaybeOwned&& rhs) : m_handle(move(rhs.m_handle))
    {
        // A moved reference still points at the old location for refs to stack variables.
        // Don't use MaybeOwned's reference variant in a move ctor.
        VERIFY(!m_handle.template has<T*>());
    }

Maybe there's some code in serenity that doesn't like this (a ref to a heap object – but maybe that should use a weak ptr anyways?), but maybe it's at least worth a try.

It's kind of a general footgun, but MaybeOwned wraps the reference and makes it a bit harder to see. Also, it offers and extension point that maybe allows catching this.

[kleinesfilmroellchen]

    MaybeOwned(MaybeOwned&& rhs) : m_handle(move(rhs.m_handle))
    {
        // A moved reference still points at the old location for refs to stack variables.
        // Don't use MaybeOwned's reference variant in a move ctor.
        VERIFY(!m_handle.template has<T*>());
    }

Calling the move constructor on a non-owning MaybeOwned whose underlying object has not moved (e.g. a temporary MaybeOwned referencing a member of a long-lived heap-allocated object) is valid and sound, and this constructor would prevent it.

As @timschumi has elaborated, you are complaining about a general C++ footgun which MaybeOwned cannot detect. "Just program better lol" is unfortunately the best solution in lieu of proper lifetime tracking or garbage collection.

[timschumi]

Calling the move constructor on a non-owning MaybeOwned whose underlying object has not moved (e.g. a temporary MaybeOwned referencing a member of a long-lived heap-allocated object) is valid and sound, and this constructor would prevent it.

Note that this is what probably 99 to 100% of the reference-MaybeOwned do. They almost always construct a MaybeOwned with a reference inside it, and then move it to whatever location that needs it (usually a parameter of the next Stream's constructor).

The only thing that we could reasonably do to make it somewhat safe is to require users to pass in a NonNullOwnPtr<T>& or an existing MaybeOwned<T>& to 'prove' that they haven't just passed in a non-heap value without thinking. However, that still isn't really fool proof, and it would force us to heap-allocate AllocatingMemoryStream and friends, which are almost always short-lived and on the stack.

In the end, there is not even a slight correlation between "MaybeOwned has moved" and "underlying object has moved", to a point where if we try to disallow moving here we might as well disallow moving all other types of data structures that (can) carry a reference or pointer.

It's kind of a general footgun, but MaybeOwned wraps the reference and makes it a bit harder to see.

The last part was absolutely intentional. If something is using MaybeOwned<T>, it is saying "I don't care if I am supposed own this object or not, but make sure that I handle it correctly". This is somewhat of a middle-ground between just using a normal NonnullOwnPtr<T> ("I own this.") and a T& ("I don't care if you want me to own this, I'll just drop it if you don't hold onto it for me").

[stale[bot]]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions!