Use UnsafeCell<MaybeUninit<T>> in AtomicCell

[taiki-e]

Fixes #833

Note: This contains two breaking changes:

• Unsized values are no longer allowed.
  This is because MaybeUninit doesn't allow it.
• AtomicCell now implements Drop.
  This is because MaybeUninit prevents T from being dropped, so we need to implement Drop for AtomicCell to avoid leaks of non-Copy types.

Breakages are allowed because this is a soundness bug fix. However, given the amount of breakage, we would not be able to yank the affected releases and would only create an advisory.

[taiki-e]

r? @jeehoonkang @RalfJung

[RalfJung]

Fixes #833

I am a bit surprised to see this fix on the crossbeam side. I thought this should be fixed on the rustc side by fixing rust-lang/rust#87341?

However, I think the same change is still needed in crossbeam, to fix the program that AtomicCell is unsound for types containing padding (#748).

[RalfJung]

Also, Miri still fails on CI, since this still uses AtomicI64 and friends under the hood -- the problem is just moved one layer down.

We need new language features to support atomic operations on MaybeUninit<$int>.

[taiki-e]

@RalfJung

I am a bit surprised to see this fix on the crossbeam side. I thought this should be fixed on the rustc side by fixing rust-lang/rust#87341?

I agree that this should be fixed on the rustc side. (However, AFAIK, until this PR was created, using MaybeUninit was misunderstood to be a sufficient way -- the issue was marked I-unsound a few hours after this PR was created)

However, I think the same change is still needed in crossbeam, to fix the program that AtomicCell is unsound for types containing padding (#748).

IIUC, that problem is about treating types containing uninitialized bytes as (initialized) integers, and what this PR does is to wrap types containing uninitialized bytes in MaybeUninit. So, I believe this PR is not sufficient to fix that (we need AtomicMaybeUninit).

Also, Miri still fails on CI, since this still uses AtomicI64 and friends under the hood -- the problem is just moved one layer down.

The test added in this PR uses AtomicCell<NonZeroU128>, so the AtomicCell uses global locks instead of atomic operations (128-bit atomics is not stabilized, so crossbeam doesn't use it). So, failure is probably due to just uses of detatched threads.

We need new language features to support atomic operations on MaybeUninit<$int>.

I'm working on implementing AtomicMaybeUninit at the library level (by using inline assembly). That said, providing it as a language feature would be ideal.

[dtolnay]

I am a bit surprised to see this fix on the crossbeam side. I thought this should be fixed on the rustc side by fixing rust-lang/rust#87341?

Even if fixed in rustc, this crate would remain prone to data races/UB on stable for the next 11 weeks and on older compilers permanently. I think it's worth considering a fix in this crate even if a fix in rustc is happening. This isn't a theoretical "hypothetically a future rustc could miscompile your code because there is theoretically UB here" — it's already observably doing the wrong thing on code that's reasonably typical of real-world usage of this crate (putting AtomicCell inside a struct inside an Option is not a rare situation).

The fact that this currently unavoidably breaks ?Sized support would be the only hesitation about doing a fix in crossbeam, but I think that discussion would only be about whether to release the workaround in a patch vs an incompatible semver version. How disruptive a patch for this would be depends on how much existing code involves AtomicCell<T> with T: ?Sized. I don't know of any, but I wouldn't necessarily know of any. The ?Sized has been there since #360 which doesn't seem like it was motivated by any particular use case.

Separately we should definitely fix rustc, and also fix the standard library to make MaybeUninit support T: ?Sized.

[RalfJung]

fix the standard library to make MaybeUninit support T: ?Sized.

That is very hard at best. See rust-lang/rust#80158.

[taiki-e]

I plan to release this in a patch version, considering that:

• Users using AtomicCell<!Sized> are either non-existent or very rare (I could not find it). I think this is not surprising considering that there is only one API left that is allows ?Sized and its use cases seem to be very limited.
• Adding Drop implementation is also a breaking change, but RFC1105 treats only the "fundamental" trait implementation as a major change and all others as minor changes (Drop is not fundamental trait).
• Breakages are allowed because this is a soundness bug fix. (Although it depends on the amount of breakage.)
• The fix does not exist yet in upstream.

bors r+

[bors]

Build succeeded:

• ci