New lint: _.as_ptr() as *mut _ 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.

Update: turns out there are cases when this is not immediately trigger UB, but in those cases you shouldn't be doing these casts anyway.

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 passes Clippy as of version 0.0.212 but 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

Rust stdlib also had this bug: https://blog.rust-lang.org/2017/02/09/Rust-1.15.1.html

[Lokathor]

This is basically covered by #4771 and the discussion there

[scottmcm]

I think this would be better phrased as a lint for casting something when there's a method that just gives you what you want. The cast here isn't UB, but I agree that you shouldn't be doing .as_ptr() as *mut T -- but nor should you be doing .as_mut_ptr() as *const T.

[Shnatsel]

#4771 would be allow-by-default while this is more specific and can be deny-by-default

[Shnatsel]

Wow, Rust standard library itself had this bug: https://blog.rust-lang.org/2017/02/09/Rust-1.15.1.html

Also turns out this might not be instant UB depending on how the underlying slice was obtained, but it's still a really bad idea. I'll update the description with this info.

[RalfJung]

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.

This is not entirely correct... something like &mut foo as *mut T as *const T as *mut T is entirely harmless. 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.

Oh, looks like you found that yourself already. Might be good to update the initial paragraph then. ;)

[Shnatsel]

FWIW I've also opened a request for such lint in the compiler, but the resolution was "start out the lint in Clippy and then uplift it based on how the implementation looks".

[CAD97]

To be perfectly clear: (_: *const T) as *mut T is not UB. (The title of the issue should be adjusted for this fact.) All this does is change variance. In fact, this is perfectly safe code that doesn't require unsafe to be written, so it'd be unsound if this were UB.

What is UB is using a pointer whose provenance is a shared &_ borrow in a mutable manner; e.g. ptr::write(_, ...), &mut *_, or *_ =.

In fact _.as_ptr() as *mut _ occurs a lot, correctly, in a couple pointer-heavy libraries I wrote recently. The reason is for (A)Rc to ptr::NonNull conversion: they only stably expose into_raw() -> *const _ currently, and into_raw_non_null() -> ptr::NonNull<T> is not yet stable.

In conclusion, it would definitely be correct to warn-by-default use of _.as_ptr() as *mut _ and _.as_mut_ptr() as *const _ when both methods are present. This can be correct for both cases (suboptimal APIs exist and sometimes it is about variance), but these two are weird enough and likely-to-be-wrong enough to deserve acknowledgement.

This might be able to generically apply to any &_ -> *const _ function, because (|p: &*mut T| -> *const T { *p })(_) as *mut T is perfectly allowed but quite strange. Accessors typically should return the pointer type that communicates its provinence as well as possible.

[Lokathor]

Shocking that it hasn't been said yet: C FFI often needs to accept a *mut T that it's only going to read from. Because the C folks aren't as careful about const* vs * as we are in Rust.

[RalfJung]

In fact _.as_ptr() as *mut _ occurs a lot, correctly, in a couple pointer-heavy libraries I wrote recently. The reason is for (A)Rc to ptr::NonNull conversion: they only stably expose into_raw() -> *const _ currently, and into_raw_non_null() -> ptr::NonNull is not yet stable.

Notice that Rc::into_raw returns a ptr with "shared ref" provenance. So actually writing to that pointer is UB. (In fact, this likely makes Rc::from_raw(Rc::into_raw(rc)).get_mut() unsound .)

Shocking that it hasn't been said yet: C FFI often needs to accept a mut T that it's only going to read from. Because the C folks aren't as careful about const vs * as we are in Rust.

Where does a const-to-mut cast occur here? Is it when Rust code calls C code? But then one could also choose the C function signature in Rust to use *const if one is anyway sure that this is read-only.

[Lokathor]

You can PR winapi to offer something other than exactly and precisely what the official windows SDK headers declare, and we'll see how that goes ;3

[RalfJung]

Sure, if they choose a certain coding stale that might mean more lints that show up for them. I am just saying that there's nothing fundamental about FFI here.

[llogiq]

Do I get that right? We want to lint sequences of casts (with no other expressions in between) where the original expression is an immutable ref or ptr and the result is a mutable ref or ptr?

[gnzlbg]

Do I get that right? We want to lint sequences of casts (with no other expressions in between) where the original expression is an immutable ref or ptr and the result is a mutable ref or ptr?

If the original expression is an immutable ref or ptr, what you don't want is ending up in a mutable ref. AFAICT a mutable pointer is fine, and kind of required if you want to store a pointer derived from an immutable ref as a NonNull<T> which uses a *mut T internally.

[llogiq]

@gnzlbg thank you, then I will only lint casts resulting in *mut.

[Shnatsel]

@llogiq you probably meant &mut

To sum up:

• casting &T to *mut _ is not UB in itself, but it's easy to trigger UB down the line
• casting &T to *mut _ and then writing to it is UB
• casting &T to &mut _ through any chain of casts is UB

Note that &mut _ does not have to be constructed through an explicit cast: from_raw_parts and similar functions on mutable slices, vectors, etc also produce &mut.

[Shnatsel]

Seemingly the last remaining valid use case for going from &T to *mut is going to be eliminated by rust-lang/rust#47336

But it hasn't seen any progress in over a year

[Lokathor]

No, the FFI case remains even after that.

[gnzlbg]

To sum up:

Thanks @Shnatsel, that summary is correct AFAICT.

Note that &mut _ does not have to be constructed through an explicit cast: from_raw_parts and similar functions on mutable slices, vectors, etc also produce &mut.

Yes, @Shnatsel has an example in the OP that uses old_slice, but notice that due to auto-deref that can be an array, a vector, etc. For example (using transmute, but you can trigger this using from_raw_parts_mut, etc.):

let mut x = [1, 2, 3];
let y: &mut i32 = unsafe { transmute(x.as_ptr()) }; // ERROR
let y: &mut i32 = unsafe { transmute(x.as_mut_ptr()) }; // OK

The problem is that [i32; 3] auto-derefs into a slice, and [i32]::as_ptr(&self) creates a &self, so the chain you get is &self -> *const i32 -> &mut i32 which creates a &mut T from a &T, which is UB. Changing that code to x.as_mut_ptr() is ok because the chain you get is &mut self -> *mut i32 -> &mut i32, so you don't create any &mut T from a &T. It is very easy to cause this via auto-deref and methods taking &self.

Seemingly the last remaining valid use case for going from &T to *mut is going to be eliminated by rust-lang/rust#47336

I think there are a couple more valid use cases than that. @Lokathor mentioned the situation where FFI declarations for C functions use *mut T that are never read from, which is quite common, because in C people don't use const T* that often. One can maybe make the Rust bindings use *const T, but then one has other problems like automatic binding validation now fails for that API.

Some Rust APIs also expect a *mut T. For example, if you want to encode the property that you have a non-null pointer that you are only going to read from, your only tool is NonNull:

let x = [1, 2, 3];
let y = std::ptr::NonNull::<i32>::new(x.as_ptr() as *mut _)?;

but that creates a *mut i32 which is illegal to write to.

So we probably want to have multiple, independent lints.

• &T -> &mut T: always UB, should be enabled by default, and probably belongs in rustc at some point

• &T -> *mut T where there is a write to *mut T: always UB, should be enabled by default, and probably belongs in rustc as well at some point

• &T -> *mut T with "whitelist": clippy lint, not necessarily enabled by default, should not warn on common idioms like conversions of *const T/&T to *mut T when calling extern "C" functions, converting to NonNull<T>, etc.

• &T -> *mut T: an opt-in lint that always warns when this happens, so that people who are trying to find UB in crates can enable it, and go through all the warnings. Probably to noisy to be enabled by default, but we can decide where and when to enable it later.

[Lokathor]

I think the type 3 and 4 lints can be a a single lint, warn by defaults, and projects that know what they're doing can turn off the lint.

If you don't know enough of what you're doing to be confident in turning off the lint, you sure as ferris should not be doing it.

[KamilaBorowska]

I think already existing cast_ref_to_mut lint should be extended to handle cases where *mut T created from &T is passed into a function that isn't one of the following: any extern "C" function, std::ptr::NonNull::new, std::ptr::NonNull::new_unchecked.

[CAD97]

@Lokathor the problem with making it a single lint is that even if you have to use it for e.g. NonNull::new and you turn it off because of that, you're also turning it off for more questionable cases. And requiring every use of NonNull::new to opt-out of the lint is a lot.

Personally, I agree type three should be a warn-by-default clippy::correctness lint, and type four should be an allow-by-default clippy::pedantic lint.

Though, to be fair, &T -> NonNull<T> should be NonNull::from, not NonNull::new. It's only cases where APIs give you *const T (such as .into_raw() on shared data structures without .into_raw_non_null() equivalents) where you should need to cast a shared pointer into a *mut _ for NonNull.

[Lokathor]

What? No, if you need it in one specific constructor for a thing using NonNull then then just turn it off in that one constructor, not any other place.

[CAD97]

There are almost certainly multiple places that you need to do the transformation, rather than one localized one. I know that's the case for rc_borrow/rc_box if I hadn't used a trait and macro to get a generic into_raw_non_null, and instead just did the code for each of the types.

[Lokathor]

Between those two links I see one instance of NonNull::new with a quick Ctrl+F search.

I'm not saying that no one would ever hit the warning, I'm saying that warnings exist to be fixed or turned off. If there's a warning that you might be doing something fishy and you mark it as fine and turn if off, that's still a good. You've still helped everyone who looks at the code in the future. "What were they trying to do here, did they even consider this case? Oh, there's an allow, at least they did consider the case."

[CAD97]

Note that the impls may have to change to use NonNull::new more.

If you count erasable, there are currently two uses of NonNull::new(_unchecked), but each of those are macro-duplicated over (up to) five types.

If we know one case of the lint is much more prone to "questionable positives", and uses of that case tend to cluster, it makes sense to allow control of that separate part of the lint separately.

I personally am very happy to litter my code with very granular warning allows, because I'm quite pedantic about linting. But I'd be quite tempted to turn off a "cast *const _ to *mut _" lint globally, or at least module-wide, rather than annotate every use spot in a pointer heavy implementation.

It's perfectly fine as a pedantic lint, but a regular lint really shouldn't cover these known problem cases. At least in the FFI case, it'd almost certainly be clearer not write a wrapper to do the cast unless directly exposing a safe version of the API (prefer thinner wrappers, etc).

I'll admit I'm probably overestimating the impact the lint would have on pointer-heavy code, though, as the lint should only fire when the transform is &T -> *mut T and not just *const T -> *mut T.