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 0af68fb.

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:

once_cell/src/lib.rs

Lines 504 to 513 in 0c7c8e7

	unsafe fn set_inner(&self, value: T) {
	let bomb: T = mem::uninitialized();
	let ptr = Box::into_raw(Box::new(bomb));
	self.inner.store(ptr, Relaxed);
	println!("written_ptr");
	::std::thread::sleep_ms(100);
	println!("writing value");
	ptr::write(ptr, value);
	}
	}

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:

once_cell/tests/test.rs

Lines 180 to 202 in 0af68fb

	fn sync_once_cell_does_not_leak_partially_constructed_boxes() {
	let n_tries = 100;
	let n_readers = 10;
	let n_writers = 3;
	const MSG: &str = "Hello, World";
	for _ in 0..n_tries {
	let cell: sync::OnceCell<String> = sync::OnceCell::INIT;
	scope(|scope| {
	for _ in 0..n_readers {
	scope.spawn(|| loop {
	if let Some(msg) = cell.get() {
	assert_eq!(msg, MSG);
	break;
	}
	});
	}
	for _ in 0..n_writers {
	scope.spawn(|| cell.set(MSG.to_owned()));
	}
	})
	}
	}

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!