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

Add a lint against hidden reborrows in &mut -> *const casts #103652

Conversation

WaffleLapkin
Copy link
Member

This PR adds a lint called hidden_reborrow_in_ptr_casts that warn against code like this:

_ = &mut 0 as *const _;

Direct &mut -> *const casts go through an intermediate & reference, which strips write provenance from the pointer. Thus it's UB to write via a pointer derived from the resulting pointer.

This is a tricky thing that can easily be left unnoticed, so makes sense to warn users about it. Especially since there are plans to actually exploit this UB.

r? @RalfJung


Some more details:

  1. This lints both explicit (&mut 0 as *const _) and implicit (let _: *const _ = &mut 0) casts
  2. If the type has UnsafeCell somewhere (or, in other words it's !Freeze), then the warning is not issued, because it's ok to write to unsafe cell without write provenance
    • However we do warn if there are generic parameters in play
  3. The documentation and warn wording should probably be improved

P.S. this is rebased on #103625, because it was more convenient

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Oct 27, 2022
@rustbot
Copy link
Collaborator

rustbot commented Oct 27, 2022

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

The Miri subtree was changed

cc @rust-lang/miri

@oli-obk
Copy link
Contributor

oli-obk commented Oct 27, 2022

Very generic nit: lint names should read as a sentence if prefixed by allow/deny, so it should be hidden_reborrows_...

@RalfJung
Copy link
Member

This is a tricky thing that can easily be left unnoticed, so makes sense to warn users about it. Especially since there are plans to actually exploit this UB.

We are not emitting noalias for these casts, so pcwalton's work would not affect such code.

Also I'm sorry but I won't have time to review this.
r? compiler

@rustbot rustbot assigned oli-obk and unassigned RalfJung Oct 27, 2022
@RalfJung
Copy link
Member

RalfJung commented Oct 27, 2022

Oh also, the next aliasing model I am currently working on with an intern will hopefully make these casts just fine, and make the returned pointer writeable. So I am not even sure if we want this lint. I view this as a Stacked Borrows artifact, not something fundamental.

Direct &mut -> *const casts go through an intermediate & reference,

That's not true any more, is it? AFAIK these casts compile to addr_of!(*mutref). No & in sight.

@rust-log-analyzer
Copy link
Collaborator

The job x86_64-gnu-llvm-13 failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
   Compiling cc v1.0.73
   Compiling memchr v2.5.0
   Compiling std v0.0.0 (/checkout/library/std)
   Compiling unwind v0.0.0 (/checkout/library/unwind)
error: implicit reborrow results in a read-only pointer
    |
781 |         let a = ptr::read(x);
    |                           ^
    |
    |
    = note: cast of `&mut` reference to `*const` pointer causes an implicit reborrow, which converts the reference to `&`, stripping write provenance
    = note: it is UB to write through the resulting pointer, even after casting it to `*mut`
    = note: `-D hidden-reborrow-in-ptr-casts` implied by `-D warnings`
help: to save write provenance, cast to `*mut` pointer first
781 |         let a = ptr::read(x as *mut _);
    |                             +++++++++
    |                             +++++++++
help: to make reborrow explicit, add cast to a shared reference
    |
781 |         let a = ptr::read(x as &_);


error: implicit reborrow results in a read-only pointer
    |
782 |         let b = ptr::read(y);
    |                           ^
    |
    |
    = note: cast of `&mut` reference to `*const` pointer causes an implicit reborrow, which converts the reference to `&`, stripping write provenance
    = note: it is UB to write through the resulting pointer, even after casting it to `*mut`
help: to save write provenance, cast to `*mut` pointer first
    |
782 |         let b = ptr::read(y as *mut _);
    |                             +++++++++
help: to make reborrow explicit, add cast to a shared reference
    |
782 |         let b = ptr::read(y as &_);


error: implicit reborrow results in a read-only pointer
    |
917 |         let result = ptr::read(dest);
    |                                ^^^^
    |
    |
    = note: cast of `&mut` reference to `*const` pointer causes an implicit reborrow, which converts the reference to `&`, stripping write provenance
    = note: it is UB to write through the resulting pointer, even after casting it to `*mut`
help: to save write provenance, cast to `*mut` pointer first
917 |         let result = ptr::read(dest as *mut _);
    |                                     +++++++++
    |                                     +++++++++
help: to make reborrow explicit, add cast to a shared reference
917 |         let result = ptr::read(dest as &_);
    |                                     +++++


error: implicit reborrow results in a read-only pointer
    |
    |
506 |         let tmp = mem::ManuallyDrop::new(unsafe { ptr::read(pivot) });
    |
    |
    = note: cast of `&mut` reference to `*const` pointer causes an implicit reborrow, which converts the reference to `&`, stripping write provenance
    = note: it is UB to write through the resulting pointer, even after casting it to `*mut`
help: to save write provenance, cast to `*mut` pointer first
    |
506 |         let tmp = mem::ManuallyDrop::new(unsafe { ptr::read(pivot as *mut _) });
    |                                                                   +++++++++
help: to make reborrow explicit, add cast to a shared reference
    |
506 |         let tmp = mem::ManuallyDrop::new(unsafe { ptr::read(pivot as &_) });


error: implicit reborrow results in a read-only pointer
    |
    |
559 |     let tmp = mem::ManuallyDrop::new(unsafe { ptr::read(pivot) });
    |
    |
    = note: cast of `&mut` reference to `*const` pointer causes an implicit reborrow, which converts the reference to `&`, stripping write provenance
    = note: it is UB to write through the resulting pointer, even after casting it to `*mut`
help: to save write provenance, cast to `*mut` pointer first
    |
559 |     let tmp = mem::ManuallyDrop::new(unsafe { ptr::read(pivot as *mut _) });
    |                                                               +++++++++
help: to make reborrow explicit, add cast to a shared reference
    |
559 |     let tmp = mem::ManuallyDrop::new(unsafe { ptr::read(pivot as &_) });

error: could not compile `core` due to 5 previous errors
Build completed unsuccessfully in 0:03:26

@WaffleLapkin
Copy link
Member Author

So, from the @RalfJung's and folks' in community discord input it seems like this lint was misinformed and based on outdated view of the situation. The code that could be linted is currently UB (according to miri at least), but if it's not an inherent property of Rust, but more of a temporary result of SB, then we probably shouldn't lint it.

As such, I'm closing this ;-;

@WaffleLapkin WaffleLapkin deleted the forbids_writing,then_lints_about_that branch October 28, 2022 09:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants