Signature of enter is not sound

[steffahn]

Neat, another self-referencing structs crate I hadn’t known of before (but now I do because of your recent-ish blog post). 🎉

You might have seen me before on the issue trackers of many other self-referencing structs crates.

---

Anyways, here’s the issue: The signature of enter says

G: for<'a> FnOnce(&'a mut <T as Family<'a>>::Family) -> Output,

which features a &'a mut Foo<'a>-style type, an infamous anti-pattern in Rust generally. These types are almost impossible to produce in safe code since they promise that Foo<'a> is borrowed mutably for its entire existence, essentially. So unsafe code producing them results in unsoundness really easily, more on that below.

The full signature in question is

pub fn enter<'borrow, Output: 'borrow, G>(&'borrow mut self, f: G) -> Output
where
    G: for<'a> FnOnce(&'a mut <T as Family<'a>>::Family) -> Output,

If you want to follow the precedent of ouroboros or self_cell, e.g. documented here:¹

fn with_dependent_mut<'outer_fn, Ret>(
   &'outer_fn mut self,
   func: impl for<'a> ::core::ops::FnOnce(&'a $Owner, &'outer_fn mut $Dependent<'a>) -> Ret
) -> Ret

then your correct signature must be the following instead:

pub fn enter<'borrow, Output: 'borrow, G>(&'borrow mut self, f: G) -> Output
where
    G: for<'a> FnOnce(&'borrow mut <T as Family<'a>>::Family) -> Output,
//                     ^^^^^^^

also, Output: 'borrow is unnecessarily restrictive, as far as I can tell, so you could consider

pub fn enter<'borrow, Output, G>(&'borrow mut self, f: G) -> Output
where
    G: for<'a> FnOnce(&'borrow mut <T as Family<'a>>::Family) -> Output,

---

So how is this unsound? Two ways, really. For one, producing &'a mut Foo<'a> at all is impossible to do soundly if Foo is a custom type that does implement Drop. On the other hand, for types without drop glue, producing more than one &'a mut Foo<'a> reference to the same target ever is also unsound. (This explains the Drop situation, too, as Drop receives &mut Self.)

Exploiting this is as easy as storing the &'a mut Foo<'a> (or some projection of it) inside of Foo<'a> itself:

use nolife::{BoxScope, Family, TimeCapsule};

struct Foo<'a> {
   s: String,
   r: Option<&'a mut String>,
}

struct FooFamily;

impl<'a> Family<'a> for FooFamily {
   type Family = Foo<'a>;
}

fn main() {
   {
       let mut scope = BoxScope::new(|mut time_capsule: TimeCapsule<FooFamily>| async move {
           let mut f = Foo {
               s: String::from("Hello World!"),
               r: None,
           };
           time_capsule.freeze_forever(&mut f).await
       });

       scope.enter(|foo| {
           foo.r = Some(&mut foo.s);
       });
       scope.enter(|foo| {
           let alias1: &mut String = &mut foo.s;
           let alias2: &mut String = foo.r.as_deref_mut().unwrap(); // miri will complain here already
           // two aliasing mutable references!!

           let s: &str = alias1;
           let owner: String = std::mem::take(alias2);

           println!("Now it exists: {s}");
           drop(owner);
           println!("Now it's gone: {s}");
       })
   }
}

example output

Now it exists: Hello World!
Now it's gone: b%�E[

miri output

error: Undefined Behavior: trying to retag from <2342> for Unique permission at alloc763[0x10], but that tag does not exist in the borrow stack for this location
   --> /home/frank/.rustup/toolchains/nightly-aarch64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ops/deref.rs:274:9
    |
274 |         *self
    |         ^^^^^
    |         |
    |         trying to retag from <2342> for Unique permission at alloc763[0x10], but that tag does not exist in the borrow stack for this location
    |         this error occurs as part of retag at alloc763[0x10..0x28]
    |
    = 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: <2342> was created by a Unique retag at offsets [0x10..0x28]
   --> src/main.rs:25:21
    |
25  |             foo.r = Some(&mut foo.s);
    |                     ^^^^^^^^^^^^^^^^
help: <2342> was later invalidated at offsets [0x10..0x30] by a Unique function-entry retag inside this call
   --> src/main.rs:21:49
    |
21  |             time_capsule.freeze_forever(&mut f).await
    |                                                 ^^^^^
    = note: BACKTRACE (of the first span):
    = note: inside `<&mut std::string::String as std::ops::DerefMut>::deref_mut` at /home/frank/.rustup/toolchains/nightly-aarch64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ops/deref.rs:274:9: 274:14
    = note: inside `std::option::Option::<&mut std::string::String>::as_deref_mut` at /home/frank/.rustup/toolchains/nightly-aarch64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/option.rs:1288:29: 1288:42
note: inside closure
   --> src/main.rs:29:39
    |
29  |             let alias2: &mut String = foo.r.as_deref_mut().unwrap();
    |                                       ^^^^^^^^^^^^^^^^^^^^
    = note: inside `nolife::scope::Scope::<FooFamily, {async block@src/main.rs:16:82: 22:10}>::enter::<'_, (), {closure@src/main.rs:27:21: 27:26}>` at /home/frank/.cargo/registry/src/index.crates.io-6f17d22bba15001f/nolife-0.3.3/src/scope.rs:166:22: 166:30
    = note: inside `nolife::BoxScope::<FooFamily, {async block@src/main.rs:16:82: 22:10}>::enter::<'_, (), {closure@src/main.rs:27:21: 27:26}>` at /home/frank/.cargo/registry/src/index.crates.io-6f17d22bba15001f/nolife-0.3.3/src/box_scope.rs:117:18: 117:41
note: inside `main`
   --> src/main.rs:27:9
    |
27  | /         scope.enter(|foo| {
28  | |             let alias1: &mut String = &mut foo.s;
29  | |             let alias2: &mut String = foo.r.as_deref_mut().unwrap();
30  | |             // two aliasing mutable references!!
...   |
37  | |             println!("Now it's gone: {s}");
38  | |         })
    | |__________^

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

error: aborting due to 1 previous error

---

As far as I can tell, this soundness issue is present in all existing versions of nolife.

Footnotes

1. for comparison, yoke has a bit of a weird, different signature, that also appeared sound last time I looked at it, but I should consider opening an issue there, too, because the ouroboros/self_cell one is just a lot better (less restrictive) for the user. ↩

[dureuill]

Thank you, I opened #9 to apply the suggested enter signature.

also, Output: 'borrow is unnecessarily restrictive, as far as I can tell, so you could consider

I can't remember off the top of my head why I added the restriction. I spent a lot of time fiddling the signature of enter, so I'm not too keen on relinquishing the restriction without giving it more thoughts.

I'll keep the suggestion on my TODO as #10, to reevaluate someday where my head is fresher.

[steffahn]

I can't remember off the top of my head why I added the restriction. I spent a lot of time fiddling the signature of enter, so I'm not too keen on relinquishing the restriction without giving it more thoughts.

Possibly the thought process was that Output: 'borrow would prevent the FnOnce here to return anything that involves the lifetime 'a. However, this is already sufficiently prevented by the mere fact that it’s a HRTB, and Output is a single type that’s the same for all choices of 'a.

AFAICT, the only thing that Output: 'borrow now prevents is that Output contains any lifetimes from stuff that the FnOnce captured; unless such lifetimes are longer than (or equal to) 'borrow. Which is often not a problem, in case you don’t want to keep 'borrow all that long. But I suppose, this could prevent niche – but sound – use-cases where it’d return … say … Result<&'borrow Foo, &'x Bar>, where the &'x Bar is some captured reference, and then in the Ok case, you’d keep the &'borrow Foo around for longer while already dropping the Bar early.

Also… I’m just realizing that even with Output = (), you can “simulate” output by capturing a &mut Option<ActualOutput> in the closure, and populating it inside. This approach results in no limitation of ActualOutput: 'borrow at all.¹ Which is quite a strong argument against keeping Output: 'borrow: because it’s trivial to write a wrapper that lifts this bound, anyways. (Feel free to double-check whether or not one really can wrap the API like this, of if I’m missing something.)

Footnotes

1. Unless you were to go ahead and add a restriction of G: 'borrow to prevent such a thing. ↩

[dureuill]

Alright that's pretty convincing, I might consider doing this soon :-).

If that's OK with you, I'm adding you as a co-author of the commits in #9. Given I applied your suggested fix this seems only fair to me.

I'll do the same once I'm done ironing out the quirks of #11.

[steffahn]

If that's OK with you, I'm adding you as a co-author of the commits in #9. Given I applied your suggested fix this seems only fair to me.

Sure, that's okay ^^