Soundness issue in Drop for SyncOnceCell

[m-ou-se]

#75648 added #[may_dangle] to T in the Drop implementation of SyncOnceCell. This is correct for simple types like T = &str, but when T's Drop implementation accesses borrowed data, this might lead to accessing already dropped data:

#![feature(once_cell)]

use std::lazy::SyncOnceCell;

struct A<'a>(&'a str);

impl<'a> Drop for A<'a> {
    fn drop(&mut self) {
        dbg!(self.0);
    }
}

fn main() {
        let cell = SyncOnceCell::new();
        {
            let s = String::from("hello world");
            let _ = cell.set(A(&s));
        }
}

[src/main.rs:9] self.0 = "\u{0}\u{0}\u{0}\u{0}\u{0}\u{0}\u{0}\u{0}\u{10}thread 'main' panicked at 'byte index 9 is not a char boundary; it is inside '\u{10}' (bytes 8..9) of `À`', library/core/src/fmt/mod.rs:2043:22
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

[m-ou-se]

Vec and Box also use #[may_dangle] in their Drop impl. But I can't reproduce this problem with either of those instead of SyncOnceCell. Not sure why only SyncOnceCell seems to have this problem.

Edit: Probably because SyncOnceCell contains T in a MaybeUninit, which doesn't implement Drop. Adding a PhantomData<T> should fix that. (Testing that now.)

[jonas-schievink]

I think the problem is that set puts no lifetime constraint on T, so you can write any arbitrarily short-lived T to the cell. It would have to at least outlive the &self reference.

[Mark-Simulacrum]

Cc @matklad, likely this affects upstream once_cell as well?

[m-ou-se]

I think the problem is that set puts no lifetime constraint on T, so you can write any arbitrarily short-lived T to the cell. It would have to at least outlive the &self reference.

That's also the case for a Vec:

fn main() {
        let mut vec = Vec::new();
        {
            let s = String::from("hello world");
            vec.push(&s);
        }
}

This compiles fine. (As it should.)

Replacing &s with A(&s) from above here does give an error though, like it should. Looks like dropck does understand that Vec's drop is going to drop Ts.

Looks like PhantomData<T> is what made that work properly for Vec and Box:

rust/library/core/src/ptr/unique.rs

Lines 40 to 45 in 02fe309

	// NOTE: this marker has no consequences for variance, but is necessary
	// for dropck to understand that we logically own a `T`.
	//
	// For details, see:
	// https://github.com/rust-lang/rfcs/blob/master/text/0769-sound-generic-drop.md#phantom-data
	_marker: PhantomData<T>,

[jonas-schievink]

Oh, yeah, you're right

[m-ou-se]

likely this affects upstream once_cell as well?

Doesn't look like it: the once_cell crate uses Option<T> instead of MaybeUninit<T>:

    // FIXME: switch to `std::mem::MaybeUninit` once we are ready to bump MSRV
    // that far. It was stabilized in 1.36.0, so, if you are reading this and
    // it's higher than 1.46.0 outside, please send a PR! ;) (and do the same
    // for `Lazy`, while we are at it).
    value: UnsafeCell<Option<T>>,

https://github.com/matklad/once_cell/blob/af70a46a856d1ff14b9d74cb5a4b14e448a902b7/src/imp_std.rs#L21-L25
https://github.com/matklad/once_cell/blob/af70a46a856d1ff14b9d74cb5a4b14e448a902b7/src/imp_pl.rs#L13