Unsound interaction between `slab`-based registry and `Sticky`’s `Drop` implementation

[steffahn]

use std::{mem::ManuallyDrop, thread};

use fragile::{stack_token, Sticky};

fn main() {
    let dummy_sticky = thread::spawn(|| Sticky::new(())).join().unwrap();

    let sticky_string = ManuallyDrop::new(Sticky::new(String::from("Hello World")));
    stack_token!(t);

    let hello: &str = sticky_string.get(t);

    println!("now it exists: {hello}");
    drop(dummy_sticky);
    println!("now it's gone: {hello}");
}

now it exists: Hello World
now it's gone: -[L\�Jx

---

The problem is the code of Drop for Sticky looks as follows

impl<T> Drop for Sticky<T> {
    fn drop(&mut self) {
        if mem::needs_drop::<T>() {
            …
        } else if let Some(entry) = registry::try_remove(self.item_id, self.thread_id) {
            unsafe {
                (entry.drop)(entry.ptr);
            }
        }
    }
}

calling registry::try_remove from possibly the wrong thread in case of non-needs_drop types. On the other hand, the slab-based registry::try_remove simply does

pub fn try_remove(item_id: ItemId, thread_id: NonZeroUsize) -> Option<Entry> {
    let _ = thread_id;
    REGISTRY.with(|registry| unsafe { (*registry.get()).0.try_remove(item_id) })
}

completely ignoring thread_id, and thus possibly removing the wrong item if called from the wrong thread.

---

In the demonstration above, both dummy_sticky and sticky_string got an item_id of 0 (first index of the respective, previously empty, thread-local slab), so dropping dummy_sticky will prematurely drop the String belonging to sticky_string. The use of ManuallyDrop is just to suppress the panic from subsequently dropping the sticky_string, mostly for a nicer output - the UB has already happened by then, anyways.

[steffahn]

On that note, I’m wondering what role the thread_id as part of the map’s key is playing in the map_impl version of the registry. Wouldn’t all entries get the same identical thread_id? If so, it could perhaps be stored once, not copied for every entry.

Or maybe the slab based implementation is doing it all right, and thread_id should not be part of the registry interface in the first place; instead Sticky should make sure never to call any registry API from the wrong thread at all. Also, I’m not sure whether Sticky can do any reasonable optimizations for the “needs_drop::<T>() == false” case, anyways.

[mitsuhiko]

This is pretty bad and should be addressed. I will try to get to it when I find some time.