Open Shnatsel opened 4 years ago
This is basically covered by https://github.com/rust-lang/rust-clippy/issues/4771 and the discussion there
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
.
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.
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. ;)
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".
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.
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.
In fact _.asptr() 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 intoraw() -> *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.
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
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.
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?
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.
@gnzlbg thank you, then I will only lint casts resulting in *mut
.
@llogiq you probably meant &mut
To sum up:
&T
to *mut _
is not UB in itself, but it's easy to trigger UB down the line&T
to *mut _
and then writing to it is UB&T
to &mut _
through any chain of casts is UBNote 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
.
Seemingly the last remaining valid use case for going from &T
to *mut
is going to be eliminated by https://github.com/rust-lang/rust/issues/47336
But it hasn't seen any progress in over a year
No, the FFI case remains even after that.
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.
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.
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
.
@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
.
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.
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.
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."
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
.
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.
in which case the proper solution is to rewrite it as
This also may happen when people try to mutate shared state through a
&
, in which case they need aCell
,RefCell
or anUnsafeCell
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:
Rust stdlib also had this bug: https://blog.rust-lang.org/2017/02/09/Rust-1.15.1.html