Unsound issue in `inner::drop`

[CXWorks]

Hi, thanks for your time to read this issue.

Our static analyzer find a potential unsound issue in the drop of Inner, where it doesn't provide enough check to ensure the soundness.

wgp/src/inner.rs

Lines 71 to 87 in 90753e1

	fn drop(&mut self) {
	#[inline(never)]
	unsafe fn drop_slow(this: *mut Inner, old_refcount: usize) {
	match old_refcount {
	2 => (*this).waker.wake(),
	1 => drop(Box::from_raw(this)),
	_ => {}
	}
	}
	let old_refcount = self.deref().refcount.fetch_sub(1, Ordering::Release);
	if old_refcount > 2 {
	return;
	}
	self.deref().refcount.load(Ordering::Acquire);
	unsafe { drop_slow(self.0.as_ptr(), old_refcount) }
	}

There is no synchronization that ensures only one thread proceeds to the drop(Box::from_raw(...)). The fetch_sub with Ordering::Release and the later load(Ordering::Acquire) do not form a full acquire–release synchronization that would ensure mutual exclusion or visibility of destruction.

A quick fix is turn the fetch_sub into acq-rel :

let old_refcount = self.deref().refcount.fetch_sub(1, Ordering::AcqRel);
if old_refcount == 1 {
    unsafe { drop(Box::from_raw(self.0.as_ptr())) }
} else if old_refcount == 2 {
    unsafe { (*self.0.as_ptr()).waker.wake(); }
}

Thanks again for your time.

[Nugine]

Memory ordering is always a headache.

The drop logic is from triomphe::Arc. Does triomphe::Arc have the same problem?

https://github.com/Manishearth/triomphe/blob/6ff0527036632be33a6ed45e5e8a0fb6243dee47/src/arc.rs#L627-L659

I'm also curious about your static analyzer. Is it open source?

[CXWorks]

Aha, thanks for your time and interest. Our static analyzer is actually a specilized LLM trained on Rust for code audting(safe/unsafe). I am still doing the evaluation and will release it once it's stable. There are two problems here:

1. Use a acq load as memory fense
2. You drop_slow logic

For problem 1, both your code and the triomphe::Arc have the small issue, you can check the std impl of Arc:

https://github.com/rust-lang/rust/blob/243c5a35e18b2634892fe7091d5ee888a18f77f5/library/alloc/src/sync.rs#L2612-L2689

The reason to use atomic::fence(Acquire) instead of a load acquire is because the compiler may optimize out this unused load(although it's unlikely to happen in practice). You can check following discussions for details:

• https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2015/n4455.html
• Use a load rather than a fence when dropping the contents of an Arc. rust-lang/rust#41714

In a nutshell, load acq is 99.99% correct but a fence is 100%. We will also report this issue to triomphe::Arc.

For problem 2, the code tries to wake something in the refcount=2 and then free the memory. Here you don't have a strong fence to ensure the wake function is called happens before the free. A quick understanding is in other implementations they ensure only one thread calles to drop_slow but here there will be two threads, these two threads need to be synced. And unfrotunately our suggested patch is also wrong in this case. A stronger sync is needed here for soundness or simply mark the constructor as unsafe are two possible solutions.

Thanks again for your time.

[Nugine]

Ok. I'll look into this later as I'm busy these days. (Re-learning memory orders again)

This issue is quite a good case showing LLM can spot hard bugs in the codebase.