Navigation Menu

Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add once_cell::race::OnceRef #213

Merged
merged 1 commit into from Dec 29, 2022
Merged

Add once_cell::race::OnceRef #213

merged 1 commit into from Dec 29, 2022

Conversation

gh2o
Copy link
Contributor

@gh2o gh2o commented Dec 25, 2022

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
Copy link
Owner

matklad commented Dec 25, 2022

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!

🤔 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
Copy link
Contributor Author

gh2o commented Dec 28, 2022

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
Copy link
Owner

matklad commented Dec 28, 2022

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?

Like OnceBox, but stores a shared reference instead of a Box.
@gh2o
Copy link
Contributor Author

gh2o commented Dec 29, 2022

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
Copy link
Owner

matklad commented Dec 29, 2022

bors r+

@bors
Copy link
Contributor

bors bot commented Dec 29, 2022

Build succeeded:

@bors bors bot merged commit 85e372f into matklad:master Dec 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants