RefMutContainer is unsound

[CheaterCodes]

Since I was made aware of an unsoundness in this code base today (see #1485), I decided to have a bit of a look around.

I found that RefMutContainer erases lifetimes, resulting in more unsafe code.
Source location:

tokenizers/bindings/python/src/utils/mod.rs

Lines 44 to 66 in 14a07b0

	#[derive(Clone)]
	pub struct RefMutContainer<T> {
	inner: Arc<Mutex<Option<*mut T>>>,
	}
	impl<T> RefMutContainer<T> {
	pub fn new(content: &mut T) -> Self {
	Self {
	inner: Arc::new(Mutex::new(Some(content))),
	}
	}
	pub fn map<F: FnOnce(&T) -> U, U>(&self, f: F) -> Option<U> {
	let lock = self.inner.lock().unwrap();
	let ptr = lock.as_ref()?;
	Some(f(unsafe { ptr.as_ref().unwrap() }))
	}
	pub fn map_mut<F: FnOnce(&mut T) -> U, U>(&mut self, f: F) -> Option<U> {
	let lock = self.inner.lock().unwrap();
	let ptr = lock.as_ref()?;
	Some(f(unsafe { ptr.as_mut().unwrap() }))
	}
	}

Note that I didn't check if there is actually any unsoundness here. However, since the safety here can't be locally guaranteed, the methods should at least be marked as unsafe, but even better, fixed to be correct.

Here is an example code that shows undefined behavior when checked with Miri:

let container = {
    let mut content = String::from("foo");
    RefMutContainer::new(&mut content)
};

container.map(|text| {
  // Triggers UB
  println!("{text}");
});

Link to playground:
https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=fede2ac43a1fdb8b5e499b505e9ecdea

Error produced by miri:

error: Undefined Behavior: out-of-bounds pointer use: alloc1256 has been freed, so this pointer is dangling
   --> /playground/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/slice/raw.rs:138:9
    |
138 |         &*ptr::slice_from_raw_parts(data, len)
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ out-of-bounds pointer use: alloc1256 has been freed, so this pointer is dangling
    |
    = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
    = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information
help: alloc1256 was allocated here:
   --> src/main.rs:29:20
    |
29  |     let mut text = String::from("foo");
    |                    ^^^^^^^^^^^^^^^^^^^
help: alloc1256 was deallocated here:
   --> src/main.rs:31:5
    |
31  |     drop(text);
    |     ^^^^^^^^^^
    = note: BACKTRACE (of the first span):
    = note: inside `std::slice::from_raw_parts::<'_, u8>` at /playground/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/slice/raw.rs:138:9: 138:47
    = note: inside `<std::vec::Vec<u8> as std::ops::Deref>::deref` at /playground/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/alloc/src/vec/mod.rs:2831:18: 2831:64
    = note: inside `<std::string::String as std::ops::Deref>::deref` at /playground/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/alloc/src/string.rs:2487:43: 2487:52
    = note: inside `<std::string::String as std::fmt::Display>::fmt` at /playground/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/alloc/src/string.rs:2376:28: 2376:34
    = note: inside `<&std::string::String as std::fmt::Display>::fmt` at /playground/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/fmt/mod.rs:2377:62: 2377:82
    = note: inside `core::fmt::rt::Argument::<'_>::fmt` at /playground/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/fmt/rt.rs:173:76: 173:95
    = note: inside `std::fmt::write` at /playground/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/fmt/mod.rs:1178:21: 1178:44
    = note: inside `<std::io::StdoutLock<'_> as std::io::Write>::write_fmt` at /playground/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/io/mod.rs:1823:15: 1823:43
    = note: inside `<&std::io::Stdout as std::io::Write>::write_fmt` at /playground/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/io/stdio.rs:786:9: 786:36
    = note: inside `<std::io::Stdout as std::io::Write>::write_fmt` at /playground/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/io/stdio.rs:760:9: 760:33
    = note: inside `std::io::stdio::print_to::<std::io::Stdout>` at /playground/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/io/stdio.rs:1116:21: 1116:47
    = note: inside `std::io::_print` at /playground/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/io/stdio.rs:1226:5: 1226:37
note: inside closure
   --> src/main.rs:35:7
    |
35  |       println!("{text}");
    |       ^^^^^^^^^^^^^^^^^^
note: inside `RefMutContainer::<std::string::String>::map::<{closure@src/main.rs:33:19: 33:25}, ()>`
   --> src/main.rs:18:14
    |
18  |         Some(f(unsafe { ptr.as_ref().unwrap() }))
    |              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
note: inside `main`
   --> src/main.rs:33:5
    |
33  | /     container.map(|text| {
34  | |       // Triggers UB
35  | |       println!("{text}");
36  | |     });
    | |______^
    = note: this error originates in the macro `println` (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

Edit: Some updates for clarity

[MolotovCherry]

Similar reproduction as above, but just showing that it can easily be even more innocent than an explicit drop

let container = {
    let mut text = String::from("foo");
    RefMutContainer::new(&mut text)
};

[CheaterCodes]

Similar reproduction as above, but just showing that it can easily be even more innocent than an explicit drop

let container = {
    let mut text = String::from("foo");
    RefMutContainer::new(&mut text)
};

Haha, race condition!
I just updated it with your suggestion :)

[CheaterCodes]

Looking at this a bit closer, I can't think of any situation where this isn't

• horribly unsound or
• couldn't just use references internally.

This struct should either

• use raw pointers throughout and never dereference them
• store a &mut T internally instead of a *mut T