Possible soundness issue

[lunabunn]

someguynamedjosh/ouroboros#88

This crate, like ouroboros, uses a fake 'static to implement the self-referential Face. While I am not at all certain, my first-impression understanding based on the ouroboros issue linked above is that this is potentially unsound (as in, the compiler is free to generate UB) because if OwnedFace were to be passed into a function, the &'static [u8] (stored inside Face<'static>) would be invalidated at the end of function when the Vec is dropped.

[alexheretic]

Hello, what you describe is why self-ref cannot currently be done safely in rust. However, soundness is possible.

In this crate the trick is the 'static lifetime is encapsulated and never publicly leaked. So with an OwnedFace::as_face_ref you don't get a &Face<'static> you get &Face<'_> which has a lifetime inferred from the reference to the OwnedFace. This way standard lifetime guarantees ensure that it is impossible to have a valid reference to the underlying Face after OwnedFace has dropped.

[lunabunn]

Hello, what you describe is why self-ref cannot currently be done safely in rust. However, soundness is possible.

In this crate the trick is the 'static lifetime is encapsulated and never publicly leaked. So with an OwnedFace::as_face_ref you don't get a &Face<'static> you get &Face<'_> which has a lifetime inferred from the reference to the OwnedFace. This way standard lifetime guarantees ensure that it is impossible to have a valid reference to the underlying Face after OwnedFace has dropped.

I know, I (in my own code base) and ouroboros both happily relied on this behavior until recently.

However, the issue thread I linked to states that this is not enough; AFAICT, the mere fact that there is a &'static [u8] in there is a possible soundness issue. This is because a borrow that is passed into a function may not, even if it is encapsulated within a struct, dangle during the scope of said function.

[lunabunn]

I had to do some more checking around, but here's a more detailed explanation: when an OwnedFace is passed into a function, the internal &'static [u8] inside the RawFace inside the Face inside the OwnedFace may reasonably be retagged (my understanding is that this currently does not happen but may change at any time in the future; one should try running this through Miri with -Zmiri-retag-fields=all, but I am not currently able to do that, sorry). If it is retagged, it would then be protected equivalently to a reference being passed into the function directly, meaning it has to be valid when the function returns (at which point the OwnedFace and the Vec inside it have been dropped, invalidating the borrow).

See also: rust-lang/miri#2844

[alexheretic]

Thanks for expanding, I don't quite see how this applies to OwnedFace though. Maybe because the &'static is not passed itself into any function here.

I'm not familiar with using miri, but I ran MIRIFLAGS="-Zmiri-retag-fields=all" cargo +nightly miri test which was fine. Also added an explict drop to a test, also fine.

Perhaps we should add a miri run to the CI?

[Imberflur]

In my current understanding of things, because the SelfRefVecFace is placed in a Box before setting face, it will never encounter this issue of the &'static potentially being retagged (since SelfRefVecFace is never passed as a parameter while the face field is Some). ouroboros does not have this indirection through a Box.

However, while looking at the code I did notice that Face<&'static> is dropped after the data that it references (due to the order of the fields):

struct SelfRefVecFace {
    data: Vec<u8>,
    face: Option<ttf_parser::Face<'static>>,
    _pin: PhantomPinned,
}

IMO wrapping in ManuallyDrop and then explicitly dropping it to ensure that it is dropped first would be less error prone that reordering the fields and then relying on that. Presumably, Face currently does not access this data when it is dropped, so this doesn't currently cause any issues.

Alternatively, using MaybeUninit would allow explicitly controlling when the value is dropped like ManuallyDrop and should also prevent re-tagging. This avoids relying on the Box for soundness (I noticed a comment here #4 (comment) about potentially removing the Box). If the reliance on the box is kept, it would be useful to document it in the lifetime extension safety comment to help prevent accidentally removing it in the future.

[lunabunn]

In my current understanding of things, because the SelfRefVecFace is placed in a Box before setting face, it will never encounter this issue of the &'static potentially being retagged (since SelfRefVecFace is never passed as a parameter while the face field is Some). ouroboros does not have this indirection through a Box.

Yikes. I somehow completely glossed over this in my "code review" yesterday before I decided to open this issue. That's embarrassing, that should indeed be fine then. My apologies @ alexheretic, I should stop opening GitHub issues past 10 pm 😓 That being said...

IMO wrapping in ManuallyDrop and then explicitly dropping it to ensure that it is dropped first would be less error prone that reordering the fields and then relying on that. Presumably, Face currently does not access this data when it is dropped, so this doesn't currently cause any issues.

We don't actually need a MD because Face is already in an Option. We could just set that to None in Drop::drop to explicitly drop the face first before the storage gets dropped. Is a MD clearer? Maybe?

Alternatively, using MaybeUninit would allow explicitly controlling when the value is dropped like ManuallyDrop and should also prevent re-tagging. This avoids relying on the Box for soundness (I noticed a comment here #4 (comment) about potentially removing the Box). If the reliance on the box is kept, it would be useful to document it in the lifetime extension safety comment to help prevent accidentally removing it in the future.

I personally think it makes more sense for us to use an MU than a Box given the discussion in #4. However, perhaps we should hold off on this (or any other) change until we can get a clearer picture of what exactly we need to guarantee fields don't get retagged. See also someguynamedjosh/ouroboros#88 (comment)

[alexheretic]

Thanks for investigating. Let me know how you like #15 for ensuring drop order.