rust-lang / unsafe-code-guidelines

Forum for discussion about what unsafe code can and can't do
https://rust-lang.github.io/unsafe-code-guidelines
Apache License 2.0
651 stars 57 forks source link

Should a mutable reference which is only used for reads become invalid on reads through other ptrs? #351

Open RalfJung opened 2 years ago

RalfJung commented 2 years ago

Brought up by @saethlin around here: https://github.com/rust-lang/rust/pull/92092 fixes a standard library issue where

there is a mutable reference which is used only for reads becomes invalid as a result of interior mutation of the referent, where a shared reference does not become invalid.

So, this is a case where using a shared ref rather than a mutable ref was more correct, in the face of mutation inside an UnsafeCell.

I have to say I consider this fully intended, the promise of a mutable reference is uniqueness and UnsafeCell deliberately has no effect on &mut. But it is worth tracking whether violating this rule is a common issue, and whether it might be worth somehow allowing this. The most extreme version of this would in general allow reads through other pointers when using an &mut (perhaps making the &mut read-only in the process? that would be a new mechanism in the aliasing model though). But that will severely impact our even entirely remove our ability to reorder writes to mutable references.

saethlin commented 2 years ago

I'm very mixed on whether this is important or not. On the one hand, the code that the linked PR fixes is "obviously" silly. But it was checked in to the standard library, which I would hope gets some of the best code review out there, and is generally exposed to a lot of eyeballs (seems like every day I skim the list of open PRs on rust-lang/rust I see a few fixes for typos). So this is code that sort of obviously could have been cleaned up for reasons other than soundness... but it wasn't.

But on the other hand, writing Rust code which is both generic and uses unsafe is already very difficult because of the complexity of dealing with unwinding panics from any user-provided code such as in safe traits. So now with this situation, I feel like we need to teach library authors to make sure that their tests include both panics from generic code (notably panicking drops) but also tests under Miri where the generic type exercises interior mutability in strange place (like a PartialEq impl).

Ultimately though, I don't know how often this situation comes up. I feel like the code pattern that makes this UB is both bizarre yet easily enough written, so I don't know if my lack of examples is more an indication that it's not a problem or an indication that people are simply not testing for it and generic code is full of land mines.

digama0 commented 2 years ago

Note that one motivation for making this legal is to allow all &mut borrows to be "two-phase", using a special borrow stack element to encode this (see can &mut just always be two-phase). Under that proposal, a &mut which is only ever used for reading is essentially the same as a shared borrow in terms of the legal programs it permits.

RalfJung commented 2 years ago

Under that proposal, a &mut which is only ever used for reading is essentially the same as a shared borrow in terms of the legal programs it permits.

Note that a &mut you receive as a function argument would be already "activated" though (or else we again lose the mentioned optimizations), so this is only the case within a function.

comex commented 2 years ago

Prior art: LLVM's noalias is documented as follows (emphasis added):

This indicates that memory locations accessed via pointer values based on the argument or return value are not also accessed, during the execution of the function, via pointer values not based on the argument or return value. This guarantee only holds for memory locations that are modified, by any means, during the execution of the function.

saethlin commented 2 years ago

Isn't LLVM's noalias designed to model C's restrict (which was at one point called noalias)? I am wary of defining too much behavior and ruling out more sophisticated optimizations.

RalfJung commented 2 years ago

Yes indeed, Stacked Borrows quite deliberately is more restrictive than noalias/restrict, and allows more optimizations.

RalfJung commented 1 year ago

The most extreme version of this would in general allow reads through other pointers when using an &mut (perhaps making the &mut read-only in the process? that would be a new mechanism in the aliasing model though).

FWIW this is exactly what happens in Tree Borrows (for other reasons: to allow reordering reads). A mutable reference that is only read from accepts as much "interference" from other references as a shared reference does.