Sticky's non-scoped APIs (get{,_mut}) are technically unsound

[danielhenrymantilla]

Basically the following signature:

fragile/src/sticky.rs

Line 138 in 3bdcfa1

pub fn get(&self) -> &T {

is technically unsound when the receiver is a &'static self (which can be achieved through Box::leak(Box::new()), a static : sync::OnceCell, etc.), since that yields a &'static T, which is technically incorrect given that T is stored in thread_local! storage (here, the registry), eventually deallocated and thus non-'static.

To illustrate, one can store the so-obtained &'static T in some other thread_local!'s drop glue (let's call it FOO), and then if the first thread_local! (the one backing the actual value the Sticky pointed to (the registry)) is reclaimed before FOO is, the &T in that remaining drop glue will be dangling (or at least pointing to an expired value).

This is exactly why the thread_local!s themselves (the LocalKeys) do not offer .get()-like APIs, and require this scoped one instead.

Sticky (and thus, SemiSticky) ought to offer .with{,_mut}(|it| { ... }) APIs instead 😬

[mitsuhiko]

The minimal repro is this:

#[test]
fn test_bad_sticky() {
    let (tx, rx) = std::sync::mpsc::channel();
    std::thread::spawn(move || {
        let sticky = Box::new(Sticky::new(Box::new(true)));
        let static_sticky = Box::leak(sticky);
        tx.send(static_sticky.get()).unwrap();
    })
    .join()
    .unwrap();

    let sticky = rx.recv().unwrap();
    dbg!(sticky);
}

A potential alternative solution to me appears to be to have a Handle/Ref like type returned from get() which derefs. For as long as this is not Send I believe that this should avoid the issue as well but it will be breaking too. I am kind of hoping to avoid the with() type API as it's incredibly annoying to work with.

[danielhenrymantilla]

Problem is leakage of the Handle/Ref type then 😅

Alas, besides the very annoying scoped API version (which is why I wish https://docs.rs/with_locals somehow got nicer language support), there is only one other API which can be fully sound: a macro one:

unsafe
fn get<'must_be_local> (r: &'must_be_local Sticky<T>, _local: &'must_be_local ())
  -> &'must_be_local T
…

macro_rules! safe_deref {(
    $sticky:expr => let $binding:pat $(,)?
) => (
    let local = ();
    let $binding = match &$sticky { sticky => {
        #[allow(unused_unsafe)] { unsafe {
            $crate::path::to::Sticky::get(sticky, &local)
        }}
    }};
)}

so that it can be used as:

let sticky = Sticky::new(42);
…
safe_deref!(sticky => let reference);
assert_eq!(reference, &42);

---

Idea

• EDIT: a discussion where this is actually tested

A slightly smoother API may be to factor this macro pattern into a helper type:

pub struct LocalVariable(());

impl LocalVariable {
    /// Have a macro offer this in a non-unsafe fashion:
    /// `create_local!(let token);` (token would then be a `&'fn LocalVariable`)
    pub unsafe fn new() -> Self {
        Self(())
    }

    /// we can still offer a scoped API as well to remain flexible.
    pub fn with_new<R>(scope: impl FnOnce(&Self) -> R) -> R {
        scope(&Self(()))
    }
}

and then have get be:

fn get<'local>(self: &'local Sticky<T>, _proof: &'local LocalVariable) -> &'local T

/// Notice how we don't need the mut on the proof, since we just care about its non-static lifetime.
fn get_mut<'local>(self: &'local mut Sticky<T>, _proof: &'local LocalVariable) -> &'local mut T

[mitsuhiko]

You are right. Since the handle can also become 'static there really does not seem to be a good non-macro way around this. I will ponder about this a bit but I want to get this fixed. Worst case a with API it is :(

[mitsuhiko]

This is resolved in 2.0.0. I went with passing a proof token in which I think has a better migration experience.