Fix unsync unsound aliasing of Option<T>

[danielhenrymantilla]

As per https://github.com/matklad/once_cell/issues/1, I have looked at unsync::OnceCell code, and found that ::once_cell::unsync::OnceCell::set is unsound when combined with ::once_cell::unsync::OnceCell::get:

• for a brief moment, before Err-oring, we have a unique reference to the inner Option<T>, despite it being potentially aliased by a previous .get() borrow.

For instance, miri rejects the following code:

fn main ()
{
    let x = ::once_cell::unsync::OnceCell::new();
    x.set(42).unwrap();
    let at_x = x.get().unwrap(); // --- (shared) borrow of inner `Option<T>` --+
    let _ = x.set(27); // <-- temporary (unique) borrow of inner `Option<T>`   |
    println!("{}", at_x); // <------- up until here ---------------------------+
}

Changing the kind of borrow with which the discriminant of Option<T> is read to be shared, before getting a unique borrow only on the non-Err-oring path fixes the issue, as done in this PR.

[matklad]

This is tricky, but makes sense. I wonder if there's some kind of more direct way to get this "upgradable read" behavior: in the current code, doing the second "load" of slot is basically a no-op anyway, but I can imagine situations where fetching a refernce again causes overhead...

Did you manage to run the test suite under miri somehow? If I try to do this locally, I get Miri evaluation error: miri does not support dynamically loading libraries

bors r+

Thanks!

[bors[bot]]

Build succeeded

• continuous-integration/travis-ci/push

[danielhenrymantilla]

Did you manage to run the test suite under miri somehow? If I try to do this locally, I get Miri evaluation error: miri does not support dynamically loading libraries

Same for me: the sync / multithreading / pthread-based stuff seems to use something that is not supported by miri.

[Shnatsel]

Can this lead to memory corruption or concurrency issues in practice?

If so, please file a security advisory at https://github.com/RustSec/advisory-db so that dependents of this crate can check if they're running a vulnerable version and upgrade.

[matklad]

I don’t think so: this violates stacked borrows model, but compiler most likely does’t exploit this yet.

In other words, this is unsound according to a memory model which is not formalized yet, and no known miscompilations exist.

[matklad]

@Shnatsel not that advisory will be filed due to https://github.com/matklad/once_cell/issues/46