mutable_transmutes lint should catch transmutes from a type without interior mutability to one with

[glandium]

I'm filing this as a regression, although the summary is worded in a feature request-ish way.

So, the regression itself is this: code that transmutes from a type without interior mutability to one with interior mutability leads to really bad outcomes after the bump to LLVM 16, because of the changes in llvm/llvm-project@01859da. This is a pattern that happens in the wild, probably mostly around FFI. At least, that's how it happens in the Firefox codebase.

Code

Here is a reduced testcase using a similar pattern to what Firefox is using:

pub struct Foo(std::cell::UnsafeCell<usize>);

pub struct Bar([u8; 0]);

pub fn foo(f: &Bar) {
    unsafe {
        let f = std::mem::transmute::<&Bar, &Foo>(f);
        *(f.0.get()) += 1;
    }
}

With rustc up to 1.69.0 in --release mode, this produces the following IR:

define void @_ZN10playground3foo17hc0e352349f95f9ecE(ptr noalias nocapture noundef nonnull readonly align 1 %f) unnamed_addr #0 {
start:
  %0 = load i64, ptr %f, align 8, !noundef !2
  %1 = add i64 %0, 1
  store i64 %1, ptr %f, align 8
  ret void
}

With rustc 1.70.0-beta.2 in --release mode, this produces the following IR:

define void @_ZN10playground3foo17h2141d3a0b5fe8d73E(ptr noalias nocapture noundef nonnull readonly align 1 %f) unnamed_addr #0 {
start:
  ret void
}

Note how everything is gone because the input pointer is marked as noalias readonly, and thus the code is not expected to change what it points to, so it's all removed. This is the typical example of undefined behavior leading to the optimizer doing unexpected things. I'm not arguing that there isn't undefined behavior. The undefined behavior existed before. But with LLVM 16, now the undefined behavior is actively dangerous.

Now, to come back to the feature-request-y summary I wrote, the following code does not compile:

pub struct Bar([u8; 0]);

pub fn foo(f: &Bar) {
    unsafe {
        let _ = std::mem::transmute::<&Bar, &mut Bar>(f);
    }
}

The produced error is:

error: transmuting &T to &mut T is undefined behavior, even if the reference is unused, consider instead using an UnsafeCell

I would argue that the code that is now compiled to nothing should also produce a similar error, and that error should be shipped in 1.70.0.

Cc: @pcwalton, @nikic

[apiraino]

WG-prioritization assigning priority (Zulip discussion).

@rustbot label -I-prioritize +P-high +T-compiler +A-LLVM +regression-from-stable-to-beta

[ChayimFriedman2]

@rustbot claim

[glandium]

FWIW, we've hit yet another case of such a transmute that a lint would have made obvious in the first place...

[glandium]

While trying to find workarounds, I also figured out that the same footgun can be triggered with "manual" transmutes too... e.g.

let f = std::ptr::NonNull::from(f).cast::<Foo>().as_ref();

or

let f = (f as *const Bar as *const Foo).as_ref().unwrap();

I don't know if they exist in the wild, but a lint would sure be useful for those too.

[emilio]

This lint should be quite a big priority IMO. Code like this is busted right now if Base doesn't have an UnsafeCell:

#[repr(C)]
struct Derived {
    base: Base,
    something: RefCell<...>,
}

impl Base {
    fn as_derived(&self) -> Option<&Derived> {
        if !self.is_derived() { // Assume some sort of bit check here.
            return None;
        }
        Some(unsafe { std::mem::transmute(self) })
    }
}

It's totally non-obvious that this code is broken, and totally non-obvious that LLVM will mis-compile your code unless Base has an UnsafeCell or some other interior mutability. This is code that is reasonable (IMO) and has been working forever (Servo has used this kind of pattern for a long time, see https://github.com/servo/servo/blob/c86faae371f1319d136425e2ffcd80def48132fd/components/script/dom/bindings/inheritance.rs#L16)

[glandium]

an UnsafeCell or some other interior mutability

AIUI this actually boils down to UnsafeCell. Any other interior mutability that counts for this problem will have an UnsafeCell hidden somewhere.

[glandium]

BTW, PhantomData<UnsafeCell<...>> doesn't work around the problem either. (maybe it should?)

[sylvestre]

@samueltardieu do you think it could be added as a clippy checker?

[sagudev]

@ChayimFriedman2 any progress on this?

[ChayimFriedman2]

@sagudev I'm working on it (slowly). It is not as simple, because to handle the example in the OP also requires looking for nested fields, which the current code does not do.

[sagudev]

@sagudev I'm working on it (slowly). It is not as simple, because to handle the example in the OP also requires looking for nested fields, which the current code does not do.

Good luck.

Clippy has is_interior_mut_ty which might help.

[ChayimFriedman2]

Under Tree Borrows transmuting a & to &UnsafeCell works as long as there is no actual mutable access to the value, and there is even a test for that in Miri. What am I supposed to do with this lint? See also #118446.

[emilio]

@ChayimFriedman2 I think warn-by-default would be a better state than the current status quo.

[alexpyattaev]

There apparently is a way to go around existing lints around transmutation of shared references into mutable pointers by passing them into functions. See example here: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=fc92b7ea1550c45c3d978420b1036e91

Arguably, this sort of inconsistent warning behavior is even more dangerous then no warning at all... Hopefully you can address this at the same time. Thanks!

[ChayimFriedman2]

@alexpyattaev It is extremely hard to lint on such cases. It will probably require inter-procedural analysis, or at least an extensive system of metadata about functions that rustc is unlikely to ever have.

Arguably, this sort of inconsistent warning behavior is even more dangerous then no warning at all...

Lints are on a best-effort basis. We lint when we can, and don't lint when we cannot. This is way better than not linting at all, especially considering that many of the incorrect usages will get caught.

Also, since this is a new feature request, that should've been a new issue.

[alexpyattaev]

Apologies, I simply misunderstood how the lint was intended/expected to work. By no means do I expect the compiler to hold my hand when I type unsafe and do strange stuff. My concern was motivated by one of my students finding this way to bypass the lint and thinking his code no longer had UB (since the error went away). As this lint was not intended to be a "reliable idiot stopper", probably it would be nice to have something like "stop and think, do not try to work around this lint" somewhere in there to prevent people from "finding workaround" for it. Or you can safely ignore this and let them learn the hard way to not upset the compiler =)

[oriongonza]

@rustbot claim

[ChayimFriedman2]

@dev-ardi I already have a working PR for this (#128351), don't know why I was unassigned.

[jieyouxu]

My apologizes, I have no idea why I unassigned when I even did a try job on the PR (#128351)