Possible UB from Box<T> aliasing

[nico-abram]

While watching yesterday's live stream Box shallow copying was mentioned, and me and a couple viewers asked about aliasing

I asked in the UFC zulip stream about it

Quoting RalfJ:

RalfJ: it's more that Box is an exclusive ptr and even reads 
        through aliases are disallowed when a ptr is exclusive

RalfJ: (this grants extra optimization power because it means 
        nobody else can even observe the current value stored behind the ptr)

Example which triggers UB according to MIRI (As provided by bjorn3 in the zulip topic)

Even just making a shared reference instead of actually reading the Copy value also triggers UB according to MIRI

Changing it to make the shallow copy the same way as the Box<T> shallow copy impl seems to change nothing: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=6e7b08eb28d6032fbdb41028225f4dde

[nico-abram]

Related comment #73 (comment)

[jonhoo]

Thanks! Just for reference I'll add my reply to that to remind myself in the future:

My guess is that the best way to fix this is to move to NonNull everywhere I store a Box now.

It does have pretty unfortunate implications for ShallowCopy though, because it suggests that you can't ever ShallowCopy something that holds a Box either. That would be extremely unfortunate.

[danielhenrymantilla]

It does have pretty unfortunate implications for ShallowCopy though, because it suggests that you can't ever ShallowCopy something that holds a Box either. That would be extremely unfortunate.

What do you mean? Once you use a ptr::NonNull or any other kind of raw pointer, the aliasing guarantees on the pointee are inert, only being asserted on dereference.

And since having &Box<_>es is allowed in safe-code, having ptr::NonNull<Box<_>>s that are only &-dereferenced is fine too.

[jonhoo]

@danielhenrymantilla My concern specifically is that it makes it very unergonomic to work with boxed values in evmap, since you cannot simply use a value that contains Box — you'd need to wrap it in some other type and then deal with the resulting unsafety in your own code. NonNull<Box<_>>, besides being a pointer to a pointer, is not very ergonomic, and requires unsafe annotations every time you use it even though those particular safety variants are all upheld by the library itself. The wrapper is just there to "nullify" the aliasing requirement of Box.

[jonhoo]

Quick update: I believe that the use in left-right is now sound, since it no longer keeps aliased Boxes around as of d1b0477. I still haven't figured out what to do in evmap around ShallowCopy though. See #81 (comment) for an in-depth exploration of the problem.

[danielhenrymantilla]

@jonhoo I think I misunderstood your

something that holds a Box either

I interpreted it as "Box<T> -> ptr::NonNull<T> is not enough in case T = Box<U>" (double indirection already present; and in that case it is fine).

But if you are worried about a general P -> MaybeAliased<P> transformation, where P may be Box<T> or &mut T, etc., then I think MaybeUninit is the most appropriate wrapper for that right now, but I'm not 100% sure. I have commented on the associated UCG issue to discuss about that point.

[jonhoo]

@danielhenrymantilla Yes, I agree with you, I think MaybeUninit is the way to go. Take a look at #83, and in particular https://github.com/jonhoo/rust-evmap/blob/4e83da000cfee94810eb790f25c1dc159def1b12/evmap/src/aliasing.rs!