invalid_reference_casting lint catches some patterns that aren't UB

[LegionMammal978]

When reading the 1.73.0 release notes, I noticed that the invalid_reference_casting lint was made deny-by-default in #112431, under the premise that casting &T to &mut T via raw pointers is always UB under Stacked Borrows. However, the lint appears a bit overly strict, since such a cast does not cause UB when T is a zero-sized type:

#![allow(invalid_reference_casting)]

fn main() {
    let r1: &() = &();
    let r2: &mut () = unsafe { &mut *(r1 as *const _ as *mut _) };
    *r2 = ();
}

Without #![allow(invalid_reference_casting)], this cast triggers the lint in 1.75.0-nightly (187b813 2023-10-03):

error: casting `&T` to `&mut T` is undefined behavior, even if the reference is unused, consider instead using an `UnsafeCell`
 --> src/main.rs:5:32
  |
5 |     let r2: &mut () = unsafe { &mut *(r1 as *const _ as *mut _) };
  |                                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  |
  = note: for more information, visit <https://doc.rust-lang.org/book/ch15-05-interior-mutability.html>
  = note: `#[deny(invalid_reference_casting)]` on by default

However, when the lint is disabled, Miri runs the program successfully, under both Stacked Borrows and Tree Borrows. This makes sense looking at the rules: the reference doesn't point to any bytes, so it doesn't need to perform any access or be granted any permissions.

This pattern looks a bit dubious when expressed as a reference cast, but it is not UB. Therefore, I would suggest downgrading or turning off the lint when the destination type is a ZST.

Note: An earlier version of this issue also considered directly casting from &UnsafeCell<i32> to &mut i32, which is legal under Stacked Borrows and Tree Borrows, but explicitly called out as UB in the UnsafeCell documentation. It was decided that linting on this case, as well as the related case of casting from &UnsafeCell<i32> to &mut UnsafeCell<i32> which isn't explicitly mentioned, is permissible as an enforcement of library UB until the aliasing rules are ultimately finalized. This was clarified in the diagnostic by #116421.

[Urgau]

There is a difference between library UB and language UB and this is an example of one.

The documentation of UnsafeCell specifically says:

Note that the only valid way to obtain a *mut T pointer to the contents of a shared UnsafeCell is through .get() or .raw_get().

And even calls out against what your describing:

Even though T and UnsafeCell<T> have the same memory layout, the following is not allowed and undefined behavior:

unsafe fn not_allowed<T>(ptr: &UnsafeCell<T>) -> &mut T {
   let t = ptr as *const UnsafeCell<T> as *mut T;
   // This is undefined behavior, because the `*mut T` pointer
   // was not obtained through `.get()` nor `.raw_get()`:
   unsafe { &mut *t }
}

@RalfJung said here that he felt that linting here was justified. That still the case?

To me the lint is working as intended, this isn't a bug.

@rustbot label -needs-triage +A-lint

[RalfJung]

Yes, that was deliberate. But of course if people find it confusing we can always reconsider.

[LegionMammal978]

There is a difference between library UB and language UB and this is an example of one.

The documentation of UnsafeCell specifically says:

Note that the only valid way to obtain a *mut T pointer to the contents of a shared UnsafeCell is through .get() or .raw_get().

And even calls out against what your describing:

Even though T and UnsafeCell<T> have the same memory layout, the following is not allowed and undefined behavior:

unsafe fn not_allowed<T>(ptr: &UnsafeCell<T>) -> &mut T {
   let t = ptr as *const UnsafeCell<T> as *mut T;
   // This is undefined behavior, because the `*mut T` pointer
   // was not obtained through `.get()` nor `.raw_get()`:
   unsafe { &mut *t }
}

To me the lint is working as intended, this isn't a bug.

I see how the first cast causes library UB according to the UnsafeCell documentation. However, a modified version of the first cast is still forbidden by the lint, even though it correctly uses .get() to "obtain a *mut T pointer to the contents of a shared UnsafeCell<T>":

let r1: &UnsafeCell<i32> = &UnsafeCell::new(0);
let r2: &mut UnsafeCell<i32> = unsafe { &mut *(r1 as *const _ as *mut _) };
unsafe { *r2.get() = 42 };

Also, even without UnsafeCell, there's still the case where the destination type is a ZST and requires no permission, but the lint still catches it. It's generally permitted to create a &mut ZST reference from any (non-null aligned) address at all, except for those of dangling allocations and locals, and creating it from a shared reference seems like just another case of that. Unless we mean to have some secret unwritten language UB that prevents &ZST-to-&mut ZST casts in particular?

[Urgau]

let r1: &UnsafeCell<i32> = &UnsafeCell::new(0);
let r2: &mut UnsafeCell<i32> = unsafe { &mut *(r1 as *const _ as *mut _) };
unsafe { *r2.get() = 42 };

r2 is doing exactly what the documentation is saying to be undefined behavior. You want the .get() to be done on the creation of r2, like this:

let r1: &UnsafeCell<i32> = &UnsafeCell::new(0);
let r2: &mut UnsafeCell<i32> = unsafe { &mut *(r1.get() as *mut _) };

Otherwise it's still undefined behaviour according to the documentation.

As for the ZST concern I will let someone more knowledgeable address it.

[LegionMammal978]

let r1: &UnsafeCell<i32> = &UnsafeCell::new(0);
let r2: &mut UnsafeCell<i32> = unsafe { &mut *(r1 as *const _ as *mut _) };
unsafe { *r2.get() = 42 };

r2 is doing exactly what the documentation is saying to be undefined behavior. You want the .get() to be done on the creation of r2, like this:

let r1: &UnsafeCell<i32> = &UnsafeCell::new(0);
let r2: &mut UnsafeCell<i32> = unsafe { &mut *(r1.get() as *mut _) };

Otherwise it's still undefined behaviour according to the documentation.

Where in the documentation does it talk "exactly" about creating a &mut UnsafeCell<T> from an &UnsafeCell<T>? My point is that the quoted text is talking only about creating a pointer to the contents of the UnsafeCell<T>, and says nothing about creating one to the cell itself.

---

If we go through the literal text of the UnsafeCell documentation regarding &mut references, it says:

If you have a reference &T, then normally in Rust the compiler performs optimizations based on the knowledge that &T points to immutable data. Mutating that data, for example through an alias or by transmuting an &T into an &mut T, is considered undefined behavior.

This is specifically referring to the case where T does not have interior mutability, and &T is truly immutable. (However, it is still missing the caveat of ZST references, unless we stretch "normally" to exclude that.)

There is no legal way to obtain aliasing &mut, not even with UnsafeCell<T>.

r2 is not aliased with any other &mut references.

If you create a safe reference with lifetime 'a (either a &T or &mut T reference), then you must not access the data in any way that contradicts that reference for the remainder of 'a.

A write access doesn't contradict an &UnsafeCell<T> reference.

A &mut T reference may be released to safe code provided neither other &mut T nor &T co-exist with it. A &mut T must always be unique.

r2 remains unique for its lifetime, and indeed it could be safely passed into a callback (as long as that callback can't cause a read access on r1).

Note that whilst mutating the contents of an &UnsafeCell<T> (even while other &UnsafeCell<T> references alias the cell) is ok (provided you enforce the above invariants some other way), it is still undefined behavior to have multiple &mut UnsafeCell<T> aliases. That is, UnsafeCell is a wrapper designed to have a special interaction with shared accesses (i.e., through an &UnsafeCell<_> reference); there is no magic whatsoever when dealing with exclusive accesses (e.g., through an &mut UnsafeCell<_>): neither the cell nor the wrapped value may be aliased for the duration of that &mut borrow.

Once again, r2 is not aliased with any other &mut UnsafeCell<i32> references. Perhaps this wording could be stretched as the cast "losing" the magic and making r2.get() no longer work, but that would be a very big stretch in the absence of explicit clarification.

Note that the only valid way to obtain a *mut T pointer to the contents of a shared UnsafeCell<T> is through .get() or .raw_get(). A &mut T reference can be obtained by either dereferencing this pointer or by calling .get_mut() on an exclusive UnsafeCell<T>.

This is the main relevant passage here. Again, this is very specifically talking about "obtain[ing] a *mut T pointer to the contents". When I convert r1 to r2, I am not obtaining a mutable reference to the i32 contained in the cell, but to the UnsafeCell<i32> itself. Then, I convert that to a shared reference and call .get() to validly obtain a *mut i32 pointer to the contents. At no point do I create a mutable pointer to the contents without calling .get() or .raw_get().

[Urgau]

Sorry, I didn't realise your point was about &UnsafeCell<T> -> &mut UnsafeCell<T> regardless of the inner T.

That's an interesting point.

The reference seems to agree with your interpretation:

Behavior considered undefined:
Mutating immutable data. All data inside a const item is immutable. Moreover, all data reached through a shared reference or data owned by an immutable binding is immutable, unless that data is contained within an UnsafeCell.

That would means that:

• &UnsafeCell<T> -> &mut UnsafeCell<T> is fine
• &T -> &mut T is only fine, if T has interior mutability
• &UnsafeCell<T> -> &mut T is not fine and should use UnsafeCell::get or UnsafeCell::raw_get

Does this make sense?

EDIT: No, it doesn't.

This would means that for &A -> &mut B to be considered UB with casts:

• A should not have interior mutability
• OR if A has interior mutability then B should not have interior mutability (couldn't call UnsafeCell::{raw_,}get)

Another way to say it would be to say that for a given transformation &A -> &mut B only done with casts to not be consider lang and library UB A and B should have interior mutability.

[RalfJung]

&UnsafeCell -> &mut UnsafeCell is fine

Eh what? &T -> &mut T is basically never fine, for any T. This has nothing to do with UnsafeCell.

[Urgau]

&UnsafeCell -> &mut UnsafeCell is fine

Eh what? &T -> &mut T is basically never fine, for any T. This has nothing to do with UnsafeCell.

My bad, I mixed-up some things in my head, opsem is really not my strong suite. Sorry for the noise.

[LegionMammal978]

&UnsafeCell -> &mut UnsafeCell is fine

Eh what? &T -> &mut T is basically never fine, for any T. This has nothing to do with UnsafeCell.

The thing is, we never actually say anywhere that &UnsafeCell<T> → &mut UnsafeCell<T> is not fine. The documentation for UnsafeCell specifically says that &T → &mut T is not fine when &T is a reference to immutable data, and that &UnsafeCell<T> is the exception to this rule, since it allows the data to be mutable.

Then, later, it expressly forbids converting &UnsafeCell<T> to &mut T directly, saying that the only valid way to mutably access the contents is through .get() or .raw_get(). However, the paragraph forbidding that says absolutely nothing about converting &UnsafeCell<T> to &mut UnsafeCell<T>. And nothing outside of that paragraph forbids that conversion either: the original &UnsafeCell<T> is valid for write accesses in general, and there's nothing violating the uniqueness of the &mut UnsafeCell<T> derived from it.

[RalfJung]

When it comes to library docs, everything that is not allowed is forbidden. You cannot make any assumptions about the internals of a library type except for those that are explicitly documented.

[RalfJung]

the original &UnsafeCell is valid for write accesses in general,

No, it isn't. At least we haven't documented that. The only thing we have documented is that you can call get on it and write to the pointer that it returns. For all you know, &UnsafeCell is a read-only pointer that only becomes read-write in get.

That's not how Stacked Borrows or Tree Borrows work, but it is possible (not likely, but possible) that we end up with an aliasing model that works like that.

[LegionMammal978]

When it comes to library docs, everything that is not allowed is forbidden. You cannot make any assumptions about the internals of a library type except for those that are explicitly documented.

Isn't there general agreement that certain assumptions are allowed, even when the standard library docs don't explicitly permit them? In other words, the set of permitted unsafe operations on arbitrary types is strictly larger than "operations that can be performed with safe code and the standard library". The archetypical example here is take_mut, which you consider to be sound, and so do I, even though it has observable effects that cannot be generated safely. But meanwhile, the related operation of swapping equally-sized DSTs is not considered sound. Until we have a grand unified theory of safety invariants, there is a gray area wherever the docs don't explicitly permit nor forbid a particular operation.

[RalfJung]

We're grandfathering in patterns like take_mut, but this approach is in general not tenable -- it is not hard to end up in situations where two such extensions are mutually incompatible.

Also, this issue here is about an opsem (aliasing model) question more than it is about a safety invariant question.

[LegionMammal978]

Also, this issue here is about an opsem (aliasing model) question more than it is about a safety invariant question.

I'd say the UnsafeCell case is a safety invariant question in that it is not UB by the standards of our current aliasing models, but it is UB by virtue of its library docs explicitly saying that it is UB. (You were specifically saying above that this is about "the internals of a library type".) Right now, the current aliasing models are treated as a "source of truth" for the ultimate aliasing model, in that if a certain pattern gets through every configuration of Miri, and doesn't make assumptions about library types, it's generally considered acceptable; the alternative would be to ban all programs doing anything mildly peculiar with the aliasing rules until we finally decide on a model. UnsafeCell is only an exception here due to the additional rule in its documentation.

Also, how does #116421 solve the case of &ZST-to-&mut ZST casts, to which the current aliasing models also do not ascribe any language UB, and to which no library docs apply? This issue is not just about UnsafeCell. I am not aware of a precedent of preliminarily deciding that we have "secret unwritten language UB" that can be enforced by the compiler with a deny-by-default lint, even when the pattern is valid today.

[RalfJung]

For gray area we generally do reserve the right to deny, I think, in particular if there are good alternatives. (I don't know if there are good alternatives for this case.)

But yes for ZST the argument could be made that we're unlikely to make this actually UB in the future so maybe we shouldn't lint against this. I don't have a strong opinion either way, just a technical concern -- it can be hard to find out if the type is a ZST. We could possibly silence the lint if we're sure it's a ZST but still lint for generic types that might be ZST. The only concern with that is that the fact that the lint disappears on ZST could be interpreted as a promise that this is fine...

[LegionMammal978]

But yes for ZST the argument could be made that we're unlikely to make this actually UB in the future so maybe we shouldn't lint against this. I don't have a strong opinion either way, just a technical concern -- it can be hard to find out if the type is a ZST. We could possibly silence the lint if we're sure it's a ZST but still lint for generic types that might be ZST. The only concern with that is that the fact that the lint disappears on ZST could be interpreted as a promise that this is fine...

Sure, I have no particular qualms with adding extra restrictions on "private ZST" types and whatnot, akin to the restrictions on ZST fields for repr(transparent). My main concern is that deny-by-default lints (except for future-incompat lints) should only be used for patterns that can't and won't ever do what they look like they're doing, and I don't think that performing odd but legal aliasing operations according to the current models meet this bar. (After all, what if we want to write programs specifically to demonstrate the minutiae of the aliasing models? It would be a pain to keep working around the compiler extraneously saying "no, this way is Wrong, you mustn't do it".)

If implicit promises are a real concern, then perhaps it might be useful to have something less forceful than a deny-by-default lint for patterns that could be dubious under alternative conceptions of the aliasing rules. But I personally don't see it as that big of an issue, given how compiler UB detection has always explicitly been on a "best-effort" basis rather than a hard guarantee.

Regardless, can this issue be re-opened until the ZST question is resolved?

[RalfJung]

odd but legal aliasing operations according according to the current models

We don't have a "current model", we just have a bunch of non-normative experiments.

But anyway, I reopened. Can you update the issue description to reflect that this is only about the ZST case now?

[LegionMammal978]

I have updated the issue description.