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

What are the validity invariants for AtomicBool? #208

Closed
asajeffrey opened this issue Sep 30, 2019 · 13 comments
Closed

What are the validity invariants for AtomicBool? #208

asajeffrey opened this issue Sep 30, 2019 · 13 comments
Labels
A-validity Topic: Related to validity invariants

Comments

@asajeffrey
Copy link

The validity invariant for bool is that it must be 0x01 or 0x00. Does AtomicBool have the same requirement, or is it allowed to contain any u8? Under the hood, it's an AtomicU8, so the current implementation doesn't have the requirement of being a valid bool, but is this part of its contract?

@nox
Copy link

nox commented Oct 4, 2019

Are there any public logs for the Zulip group?

@Lokathor
Copy link
Contributor

Lokathor commented Oct 4, 2019

You can log in to zulip with a GitHub account.

@bjorn3
Copy link
Member

bjorn3 commented Oct 4, 2019

It does expose you email address to others though.

@elichai
Copy link

elichai commented Oct 4, 2019

Well if you put anything that's not 0/1 there then this will be UB: https://doc.rust-lang.org/src/core/sync/atomic.rs.html#351

@asajeffrey
Copy link
Author

@elichai Yes, the application I'm looking at doesn't provide mutable access, just &AtomicBool. AFAIK this is safe in the current implementation, but this isn't documented as being safe in future versions.

@RalfJung RalfJung added C-active-discussion-topic A-validity Topic: Related to validity invariants labels Oct 9, 2019
@RalfJung
Copy link
Member

RalfJung commented Oct 9, 2019

Right now, AtomicBool is defined as UnsafeCell<u8> and thus the validity invariant is the same as u8. But that might be considered an implementation detail.

But what is the motivation here? Why not just use AtomicU8 if you intend to violate that invariant?

@Lokathor
Copy link
Contributor

Lokathor commented Oct 9, 2019

Possibly the same answer as with a lot of code: "I didn't write that part over there, I'm just trying to interface with it."

Or possibly, "I might expose this to C and I need to know if C will be able to make my data explode"

@asajeffrey
Copy link
Author

I have a trait that says it's okay to take a &T even though the reference is to shared memory. The question is whether I can implement this trait for AtomicBool (assuming for the moment that I can for AtomicU8).

@nox
Copy link

nox commented Oct 10, 2019

I just woke up so maybe I'm missing some critical information, but if you can take an &AtomicU8, then you can also take an &u8, and you can probably do so both at the same time, which means it's also UB for AtomicU8 or any other type with interior mutability.

@RalfJung
Copy link
Member

@asajeffrey well it's certainly not safe to have an AtomicBool pointing to shared memory, as safe code could load it and that's UB if there's not truly a bool there.

Right now, in terms of language UB AtomicBool and AtomicU8 are equivalent. But I'd expect that it's library-UB to have an AtomicBool that's not a valid bool.

@gnzlbg
Copy link
Contributor

gnzlbg commented Oct 10, 2019

But I'd expect that it's library-UB to have an AtomicBool that's not a valid bool.

To expand on what "library-UB" means: the standard library makes no guarantees about how AtomicBool is implemented, in particular, it does not guarantee that it is implemented using UnsafeCell<u8>, i.e., the implementation can change to use UnsafeCell<bool> any time and that probably wouldn't be a breaking change (cc @rust-lang/libs).

So yes, for a particular toolchain version, you can breach the standard library privacy boundary, inspects how AtomicBool is implemented, and uses the knowledge that it currently uses an UnsafeCell<u8> under the hood to temporarily break the safety invariant of AtomicBool without having UB on the abstract machine due to a violation of the validity invariant. Whether that code exhibits undefined behavior or not then depends on an assumption that can be silently invalidated any time by the toolchain.

I think this status quo is fine. AtomicBool is just an user-defined aggregate whose rules follow from the aggregate rules. If the standard library wanted to guarantee that you can store a value different from true and false on an AtomicBool, then they can just do that on the AtomicBool docs.

@RalfJung
Copy link
Member

I agree with @gnzlbg. Closing the issue then, unless someone thinks this should be added to a FAQ like I suggested here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-validity Topic: Related to validity invariants
Projects
None yet
Development

No branches or pull requests

7 participants