Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use UnsafeCell<MaybeUninit<T>> in AtomicCell #834

Merged
merged 4 commits into from Jun 21, 2022
Merged

Conversation

taiki-e
Copy link
Member

@taiki-e taiki-e commented May 22, 2022

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
Copy link
Member Author

taiki-e commented May 22, 2022

r? @jeehoonkang @RalfJung

@RalfJung
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Member Author

taiki-e commented May 23, 2022

@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
Copy link

dtolnay commented May 27, 2022

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
Copy link
Contributor

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

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

@taiki-e
Copy link
Member Author

taiki-e commented Jun 21, 2022

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

bors r+

@bors
Copy link
Contributor

bors bot commented Jun 21, 2022

Build succeeded:

@bors bors bot merged commit 9e9ff76 into master Jun 21, 2022
@bors bors bot deleted the taiki-e/atomic-cell branch June 21, 2022 01:50
bors bot added a commit that referenced this pull request Jun 23, 2022
853: Prepare for the next release r=taiki-e a=taiki-e

- crossbeam-utils 0.8.9 -> 0.8.10
  - Fix unsoundness of `AtomicCell` on types containing niches. (#834)
    This fix contains breaking changes, but they are allowed because this is a soundness bug fix. See #834 for more.


Co-authored-by: Taiki Endo <te316e89@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

AtomicCell is unsound on types containing niches
3 participants