Yoke vs strong protection

[SabrinaJewson]

The following code fails Miri when -Zmiri-retag-fields is enabled (which will become the default soon: rust-lang/miri#2985):

fn example(_: Yoke<&'static [u8], Vec<u8>>) {}
fn main() {
    example(Yoke::attach_to_cart(vec![0, 1, 2], |data| data));
}
use yoke::Yoke;

Miri error message

error: Undefined Behavior: not granting access to tag <1714> because that would remove [SharedReadOnly for <1769>] which is strongly protected because it is an argument of call 758
   --> ~/.local/share/rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ptr/mod.rs:497:1
    |
497 | pub unsafe fn drop_in_place<T: ?Sized>(to_drop: *mut T) {
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ not granting access to tag <1714> because that would remove [SharedReadOnly for <1769>] which is strongly protected because it is an argument of call 758
    |
    = help: this indicates a potential bug in the program: it performed an invalid operation, but the Stacked Borrows rules it violated are still experimental
    = help: see https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/stacked-borrows.md for further information
help: <1714> was created by a SharedReadWrite retag at offsets [0x0..0x3]
   --> src/main.rs:3:34
    |
3   |     example(Yoke::attach_to_cart(vec![0, 1, 2], |data| data));
    |                                  ^^^^^^^^^^^^^
help: <1769> is this argument
   --> src/main.rs:1:12
    |
1   | fn example(_: Yoke<&'static [u8], Vec<u8>>) {}
    |            ^
    = note: BACKTRACE (of the first span):
    = note: inside `std::ptr::drop_in_place::<[u8]> - shim(None)` at ~/.local/share/rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ptr/mod.rs:497:1: 497:56
    = note: inside `<std::vec::Vec<u8> as std::ops::Drop>::drop` at ~/.local/share/rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/alloc/src/vec/mod.rs:3013:13: 3013:91
    = note: inside `std::ptr::drop_in_place::<std::vec::Vec<u8>> - shim(Some(std::vec::Vec<u8>))` at ~/.local/share/rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ptr/mod.rs:497:1: 497:56
    = note: inside `std::ptr::drop_in_place::<yoke::Yoke<&[u8], std::vec::Vec<u8>>> - shim(Some(yoke::Yoke<&[u8], std::vec::Vec<u8>>))` at ~/.local/share/rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ptr/mod.rs:497:1: 497:56
note: inside `example`
   --> src/main.rs:1:46
    |
1   | fn example(_: Yoke<&'static [u8], Vec<u8>>) {}
    |                                              ^
note: inside `main`
   --> src/main.rs:3:5
    |
3   |     example(Yoke::attach_to_cart(vec![0, 1, 2], |data| data));
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    = note: this error originates in the macro `vec` (in Nightly builds, run with -Z macro-backtrace for more info)

note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace

Note that this is not the same as #2095. While #2095 deals with noalias, this deals with the different issue that the &'static [u8] passed into the function is expected to be valid for as long as the function runs, but the Vec is deallocated while the function runs.

See also:

• Fundamental Soundness Problem someguynamedjosh/ouroboros#88

[Manishearth]

Hmm, cc @RalfJung @saethlin

I was under the impression that the limited form of self-borrowing that Yoke does is sound, after comparing it with the soundness issues found in other self-borrow crates.

I was also under the impression that the cited lifetime in code is not relevant for Stacked Borrows (which lets us use this lifetime to mean "runtime lifetime"). Have I misunderstood something?

[Manishearth]

Is it sufficient to wrap the field in a ManuallyDrop and add a destructor?

[Manishearth]

Hm, no, that still causes the error

[Imberflur]

MaybeUninit should work for this, in the future ManuallyDrop may also work as well:

• Fundamental Soundness Problem someguynamedjosh/ouroboros#88 (comment)
• Fundamental Soundness Problem someguynamedjosh/ouroboros#88 (comment)

[SabrinaJewson]

I was also under the impression that the cited lifetime in code is not relevant for Stacked Borrows (which lets us use this lifetime to mean "runtime lifetime"). Have I misunderstood something?

So the lifetime itself isn’t relevant — the problem here is that SB now assumes that all passed-in references, no matter the lifetime, will be valid the entire function’s duration (LLVM’s dereferenceable), which includes after destructors have been run. The thing is that when you drop the backing Vec, this invalidates that assumption.

MaybeUninit should work for this, in the future ManuallyDrop may also work as well:

Specifically, you have to store [MaybeUninit<u8>; mem::size_of::<Y>] as well as a [Y; 0] for alignment. Since generic_const_exprs is unstable, it would require modification of implementations of Yoke to have the type Output associated parameter be some type that has this MaybeUninit array internally alongside the alignment array.

[RalfJung]

MaybeUninit<Y> should also do it?

I was also under the impression that the cited lifetime in code is not relevant for Stacked Borrows (which lets us use this lifetime to mean "runtime lifetime"). Have I misunderstood something?

The issue is that when Rust decides to pass a struct field-by-field, fields with reference type will get dereferenceable noalias, and hence deallocating the memory that reference points to while the function runs is UB. Currently Rust only passes structs field-by-field if they (recursively) have no more than 2 non-ZST fields, and that's why yoke happens to work -- Yoke<&'static [u8], Vec<u8>> has 5 fields (2 for the wide reference, 3 for the Vec). However there's no guarantee that the Rust ABI won't pass larger structs field-by-field in the future so we really should make this UB regardless of the number of fields, and that's where the problem comes from.

[Imberflur]

Afaik just MaybeUninit<Y> would be sufficient to prevent retagging.

[Manishearth]

Can confirm, MaybeUninit works (I quickly hacked it in such that it leaks). Now to actually write this nicely.

I'll probably write an ActuallyInit wrapper that behaves like MaybeUninit but has nice APIs. Or I guess that's literally just MaybeDangling and I could call it that?

[RalfJung]

Or I guess that's literally just MaybeDangling and I could call it that?

Not quite since MaybeDangling preserves niches and supports unsized types and drops the inner part on drop. But very close.

[Manishearth]

Yes, this would drop on Drop, I don't need it to avoid the Drop

AlmostMaybeDangling

[danielhenrymantilla]

FWIW, the first version of ::maybe_dangling has just been released, featuring both:

• a "fixed" ManuallyDrop with "maybe dangling" semantics (by virtue of just wrapping a MaybeUninit, and then mimicking ::core::mem::ManuallyDrop's API;
• the MaybeDangling wrapper, which is just this ManuallyDrop but with automatic drop glue slapped back onto it (it does require a nightly feature for the full #[may_dangle] finesse).

The idea is for it to act as a centralized placeholder until the RFC gets implemented and stabilized, much like with ::memoffset back in the day where we had no ptr::addr_of{,_mut}! in the standard library.

[Manishearth]

We try to be low dependency here unfortunately so I'd be reluctant to take that on but I'm really glad it exists!!

[RalfJung]

@Manishearth would you prefer if we delay landing rust-lang/miri#2985 until you have a fix shipped?

[Manishearth]

This just changes how complainy miri is, yes? (and does not change how likely it is the compiler will break things).

If so I don't mind that landing and would probably prefer it land so next time I miri yoke I don't have to remember to add in flags 😄

yoke is currently miri-clean but icu4x is not due to zerovec (stacked borrows issues that we haven't been able to clean up until we bumped our MSRV, which was very recent) and some transitive deps like t1ha, so personally I do not mind if yoke becomes miri-unclean temporarily.

We don't yet CI miri.

@sffc to verify (the impact on our clients of the change suggested by ralf is that ICU4X will be more likely to error on the UB-checking tool miri when clients use our code, but we already error on that tool and there is not yet a strong community expectation for deps to be miri clean because miri and the rules it applies are still in flux.

[RalfJung]

This just changes how complainy miri is, yes? (and does not change how likely it is the compiler will break things).

Yes.
There's still -Zmiri-retag-fields=none to opt-out if you want to check whether it is "otherwise clean" (or -Zmiri-retag-fields=scalar if you want to match current codegen semantics).

[sffc]

It seems fine to me if Miri adds more warnings, so long as it doesn't break the compilation of current Yoke / ICU4X versions.

[Manishearth]

PR in #3735 if people want to take a look, the KindaSortaDangling<T> type is relatively simple since we don't actually do much with this internal type.

[Manishearth]

Fixed in #3735