Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New lint: _.as_ptr() as *mut _ is Undefined Behavior #4774

Open
Shnatsel opened this issue Nov 5, 2019 · 25 comments
Open

New lint: _.as_ptr() as *mut _ is Undefined Behavior #4774

Shnatsel opened this issue Nov 5, 2019 · 25 comments
Assignees

Comments

@Shnatsel
Copy link
Member

Shnatsel commented Nov 5, 2019

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
Copy link

Lokathor commented Nov 5, 2019

This is basically covered by #4771 and the discussion there

@scottmcm
Copy link
Member

scottmcm commented Nov 5, 2019

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
Copy link
Member Author

Shnatsel commented Nov 5, 2019

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

@Shnatsel
Copy link
Member Author

Shnatsel commented Nov 5, 2019

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
Copy link
Member

RalfJung commented Nov 5, 2019

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
Copy link
Member Author

Shnatsel commented Nov 6, 2019

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
Copy link
Contributor

CAD97 commented Dec 10, 2019

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
Copy link

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
Copy link
Member

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
Copy link

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
Copy link
Member

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
Copy link
Contributor

llogiq commented Dec 10, 2019

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?

@llogiq llogiq self-assigned this Dec 11, 2019
@Shnatsel Shnatsel changed the title New lint: *const T as *mut T is Undefined Behavior New lint: _.as_ptr() as *mut _ is Undefined Behavior Dec 13, 2019
@gnzlbg
Copy link
Contributor

gnzlbg commented Dec 13, 2019

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
Copy link
Contributor

llogiq commented Dec 13, 2019

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

@Shnatsel
Copy link
Member Author

@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
Copy link
Member Author

Shnatsel commented Dec 13, 2019

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
Copy link

No, the FFI case remains even after that.

@gnzlbg
Copy link
Contributor

gnzlbg commented Dec 14, 2019

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
Copy link

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
Copy link
Contributor

KamilaBorowska commented Dec 14, 2019

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
Copy link
Contributor

CAD97 commented Dec 14, 2019

@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
Copy link

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
Copy link
Contributor

CAD97 commented Dec 14, 2019

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
Copy link

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
Copy link
Contributor

CAD97 commented Dec 14, 2019

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

8 participants