matklad / once_cell

Rust library for single assignment cells and lazy statics without macros
Apache License 2.0
1.84k stars 110 forks source link

Add once_cell::race::OnceRef #213

Closed gh2o closed 1 year ago

gh2o commented 1 year ago

Like OnceBox, but stores a shared reference instead of a Box.

Mainly useful for #![no_std] environments where we can use it to store a &'static T.

matklad commented 1 year ago

Makes sense to have this, yeah! Could you express this in onterms of AtomicUsize though in the implementation?

pub struct OnceRef<'a, T> {
    inner: OnceNonZeroUsize,
    ghost: PhantomData<Option<&'a T>>,
}

We don't do that for OnceBox because it needs some custom drop code, but for OnceRef I think it should work!

I am also a tiny bit worried about phantom data and variance here. If you can a compile_fail test that we can't use this to somehow shorten the lifetime in an inapropriate way, that would be nice!

:thinking: actually, it does seem that we also need to do something about the lifetimes here, the current code seems to be unsound:

#[test]
fn lifetimes_test() {
    let x = 1;
    let l = OnceRef::new();
    if false {
        l.set(&x).unwrap();
    }
    {
        let y = 2;
        let r = OnceRef::new();
        r.set(&y).unwrap();
        swap(&l, &r);
    }
    eprintln!("uaf: {}", *l.get().unwrap());
    fn swap<'a>(l: &OnceRef<'a, i32>, r: &OnceRef<'a, i32>) {
        let lr = l.get();
        let rr = r.get();
        if let Some(rr) = rr {
            let _ = l.set(rr);
        }
        if let Some(lr) = lr {
            let _ = l.set(lr);
        }
    }
}
gh2o commented 1 year ago

Good catch on the lifetime issue. I've updated the code to use OnceNonZeroUsize, and changed ghost to PhantomData<UnsafeCell<&'a T>> so that your test now fails with error[E0597]: `y` does not live long enough.

matklad commented 1 year ago

Perfect, thanks!

could you also add an entry to change log, and bump a version in Cargo.toml, so that ci infra does a release on merge?

gh2o commented 1 year ago

could you also add an entry to change log, and bump a version in Cargo.toml, so that ci infra does a release on merge?

Done. Also updated set to return () on error similar to the other Once* types other than OnceBox (since &'a T is Copy), and made get_or_init and get_or_try_init return &'a T instead of &'self T.

matklad commented 1 year ago

bors r+

bors[bot] commented 1 year ago

Build succeeded: