Warn that *const T as *mut T is Undefined Behavior

[Shnatsel]

Casting a *const T to *mut T may lead to memory corruption since it allows mutation of shared state. Even if the *const T happened to be unique, it is still undefined behavior and the optimizer may break such code in interesting ways. In a nutshell, this is as bad as transmuting a & into &mut. The compiler should warn against doing this.

Update: as pointed out below, there are cases when that does not immediately trigger UB, but in those cases there is no reason to do this in the first place.

This often occurs when people try to consume a data structure and create a new one from it, e.g.

let new_slice = core::slice::from_raw_parts_mut(old_slice.as_ptr() as *mut B, len)

in which case the proper solution is to rewrite it as

let new_slice = core::slice::from_raw_parts_mut(old_slice.as_mut_ptr(), len)

This also may happen when people try to mutate shared state through a &, in which case they need a Cell, RefCell or an UnsafeCell instead.

Playground with a real-world snippet that fails MIRI: https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=b28a15e3d99616b03caafdd794550946

This pattern seems to be quite widespread - quoting @RalfJung on Zulip:

I have seen at least 2 or 3 cases over the last few weeks for a const-to-mut raw ptr cast being the give-away for mutation of shared data

I have already requested a Clippy lint for this, but this looks important enough to warn against by default, without relying on optional tooling to catch this.

[scottmcm]

Note that it's not UB to do this cast. This cast can be a contributing factor to code that's unsound (like https://blog.rust-lang.org/2017/02/09/Rust-1.15.1.html), but it's important not to say that the cast by itself is UB.

[Shnatsel]

Would it be correct to say that it becomes UB as soon as the pointer is actually used? Or as soon as a value is constructed from it?

[hanna-kruppe]

No, it all depends on how the pointer was actually made. For example, you can use slice.as_mut_ptr() to get a *mut T and then cast to *const T and back as often as you like and that won't make writing to that memory UB.

[Shnatsel]

Hmm, indeed you are right - this passes MIRI: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=efe35f2a1422ef308c66236ee3d88135

So while it's not instant UB by itself, there is absolutely no reason to do this.

[RalfJung]

Others already raised this, here's the way I put it:

What is relevant is the initial cast, when a reference is turned to a raw pointer. I think of the pointer as "crossing into another domain", that of uncontrolled raw accesses. If that initial transition is a *const, then the entire "domain" gets marked as read-only (modulo UnsafeCell). The raw ptrs basically "remember" the way that the first raw ptr got created.

(It's kind of confusing to have the same discussion both here and at rust-lang/rust-clippy#4774... maybe both issues shouldn't have been opened with almost the same text at the same time.^^)

[Shnatsel]

Good point, I will refrain from doing so in the future. I've updated description based on the discussion.

[Centril]

I think we could start out the lint in Clippy and then uplift it based on how the implementation looks and when we have a more concrete proposal.

[petertodd]

One nuisance with this is code that uses NonNull<T> for a *const T rather than the *mut T that the API assumes.

Though maybe there's a better way to do that?

[pitdicker]

This seems like a good place to ask, as it concerns a cast from *const to *mut.

Suppose I have a shared reference to an atomic. Would transmuting it to a mutable pointer for FFI be a problem?

let atomic = AtomicI32::new(1);
let ptr = &atomic as *const AtomicI32 as *mut i32;
unsafe {
    ffi(ptr);
}

Or would a dance with UnsafeCell be better?

let atomic = AtomicI32::new(1);
unsafe {
    let ptr = (&*(&atomic as *const AtomicI32 as *const UnsafeCell<i32>)).get();
    ffi(ptr);
}

[RalfJung]

Right now I think the official ruling is that you must go through UnsafeCell::get.

Stacked Borrows says that &atomic as *const AtomicI32 as *mut i32 is fine. The "&T-to-*const T" cast has a special exception for UnsafeCell (in fact, that is under the hood what makes UnsafeCell::get work). But Stacked Borrows is an experiment and not normative.

[pitdicker]

@RalfJung Thank you, that makes sense.

I wonder if it would be useful to add a small as_mut_ptr method to the Atomic* types to do just this, as I see this pattern at least in the standard library, parking_lot, and an experimental crate I am working on. They all go the direct route without the UnsafeCell at the moment. I'll make a PR and see how it turns out.

[KamilaBorowska]

About the proposed lint, I once added cast_ref_to_mut lint to Clippy (about a year ago), which was very minimal, it only handled obviously incorrect cases. In particular, if you look at https://github.com/rust-lang/rust-clippy/blob/master/tests/ui/cast_ref_to_mut.rs, one of things it intentionally didn't warn about is this (I made this decision to make the lint less annoying, essentially it only goes off if it knows for sure the code is wrong):

extern "C" {
    // N.B., mutability can be easily incorrect in FFI calls -- as
    // in C, the default is mutable pointers.
    fn ffi(c: *mut u8);
    fn int_ffi(c: *mut i32);
}
ffi(a.as_ptr() as *mut _);
int_ffi(num as *const _ as *mut _);
int_ffi(&3 as *const _ as *mut _);

However, it probably should be changed to recognize methods from standard library accepting *mut pointers as having correct mutability.