Unsafe audit

[matklad]

This crate uses unsafe quite a bit for both sync and unsync variants of OnceCell. I feel rather confident about unsync usages of unsafe: the implementation is the same as in the lazycell. However the sync version, which is inspired by lazy_static, has quite a bit more of my own invention.

It would be cool if knowledgeable people in the area reviewed the code for safety issues!

[matklad]

At least on issue was pointed on reddit: https://www.reddit.com/r/rust/comments/9406rl/once_cell_a_lazy_static_without_macros_and_more/e3hbt3k/

It should be fixed by https://github.com/matklad/once_cell/commit/0af68fbfc11d66bca61eacf2eff4b2803a50c455.

However, I was unable to write a test that actually exposes the problem in the original code. It would be cool if someone could make the code as of this commit to actually fail.

[kohensu]

I had the same concern as SlipperyFrob on reddit but it seems that it was in fact safe due to the implementations of call_once():

• for std: the call to set_inner() is surrounded by state.load(Ordering::SeqCst) and state.swap(COMPLETE, Ordering::SeqCst)
• for parking_lot it is surrounded by self.0.load(Ordering::Relaxed) and self.0.swap(DONE_BIT, Ordering::Release)

So the write of the ptr can not be visible by another thread without the write of the data.

[matklad]

@kohensu I initially thought just like that, but now I am thinking that SlipperyFrob is, in fact, 100% correct. Here's a manually forced reordering which I think would be correct under Relaxed, and which actually makes the test fail:

https://github.com/matklad/once_cell/blob/0c7c8e7194e312d0d45664f340bb9da57b0e3094/src/lib.rs#L504-L513

The problem is, the thread that calls get does not call set_inner, so the call_once synchronization has no effect on it.

EDIT: here's the corresponding stresstest:

https://github.com/matklad/once_cell/blob/0af68fbfc11d66bca61eacf2eff4b2803a50c455/tests/test.rs#L180-L202

EDIT: specifically, this is an instance of "Semi-synchronization is fine" pitfall: https://shipilev.net/blog/2016/close-encounters-of-jmm-kind/#pitfall-semi-sync

[kohensu]

@matklad, my bad, I was focusing on the get_or_init() part. Indeed there was a race between the get() and the set(). By the way thank you for taking the time to correct me.

[matklad]

I've significantly reworked sync version internals. Now, for parking lot case it leverages Once::state method for synchronization. For std version, we have to maintain is_initialized flag ourselves unfortunately.

[matklad]

I think this isn't actionable at this point: we are already deployed throughout the ecosystem :)

And, well, we now have some extensive miri tests, which inspire at least some confidence!