(soundness regression) `static mut` could be avoided.

[eddyb]

At least for inline_lazy, the Option<T> can be wrapped in Cell, needing only:

unsafe impl<T: Sync> Sync for Lazy<T> {}

Then we can just have a regular static and get can take &'static self safely.

EDIT: As noted below (https://github.com/rust-lang-nursery/lazy-static.rs/issues/117#issuecomment-415454944), the use of &'static mut Lazy<T> is unsound. Also, the impl above exists already, UB was introduced by regression at https://github.com/rust-lang-nursery/lazy-static.rs/commit/a308da1d1da7660a9577979c64bb30f0ed0700f7.

cc @anp @RalfJung

[RalfJung]

How is an unsafe impl Sync any better than a static mut?

[eddyb]

Because it's provably correct (it only depends on Lazy::get's body) and it makes uses safe.

Right now uses of Lazy are unsafe and have to be wrapped, which moves the hazards outside of the module. E.g. today for a reentrant Lazy::get you end up with the same &'static mut Lazy<T> twice in the call stack, and miri with a proper check would scream "UB".

[eddyb]

Just proves once more that static mut is an unusable mis-feature that creates UB left and right.

[eddyb]

Wait, it already has:

unsafe impl<T: Sync> Sync for Lazy<T> {}

Looks like it regressed in https://github.com/rust-lang-nursery/lazy-static.rs/commit/a308da1d1da7660a9577979c64bb30f0ed0700f7.

[RalfJung]

I do not understand the point about get. Global mutable state only works if every access cooperates, this includes readers. unsafe impl Sync can at best paper over soundness concerns.

However, I agree that this is unsound:

    pub fn get<F>(&'static mut self, f: F) -> &T
        where F: FnOnce() -> T
    {
        {
            let r = &mut self.0;
            self.1.call_once(|| {
                *r = Some(f());
            });
        }
        unsafe {
            match self.0 {
                Some(ref x) => x,
                None => std::intrinsics::unreachable(),
            }
        }

It doesn't even take reentrancy. self is alive all the time, but some other thread might be the one to win the race and initialize it.

[eddyb]

@RalfJung Right, I ignored multi-threading, because it's easier to reason about reentrance (ironically).

That is, f is arbitrary code, which means Lazy::get can be called from it, for the same self (since Lazy::get on a static mut is what lazy_static! provides a wrapper around).

Two Lazy::get on the same callstack with same self address is UB, right?

[RalfJung]

I find it easier to reason about concurrency :P

Two Lazy::get on the same callstack with same self address is UB, right?

The question is how long the references live. But given that the outer frame has a reference that's live before and after the reentrant call -- yes, this is UB.

[KodrAus]

Ok, so if I'm following this we'll avoid this problem by pushing mutability inside the Lazy through a Cell and relying on Once's thwarting of attempted re-entrancy to set the value and preventing multiple live mutable references?

[eddyb]

@KodrAus Yupp, ignoring the existence references, the current code's memory accesses are fine because of the Once, all we need to do is avoid &mut and use Cell instead.