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

Change lint message to be stronger for &T -> &mut T transmute #92704

Merged
merged 1 commit into from
Jan 20, 2022

Conversation

5225225
Copy link
Contributor

@5225225 5225225 commented Jan 9, 2022

The old message implied that it's only UB if you use the reference to mutate, which (as far as I know) is not true. As in, the following program has UB, and a &T -> &mut T transmute is effectively an unreachable_unchecked.

fn main() {
    #[allow(mutable_transmutes)]
    unsafe {
        let _ = std::mem::transmute::<&i32, &mut i32>(&0);
    }
}

In the future, it might be a good idea to use the edition system to make this a hard error, since I don't think it is ever defined behaviour? Unless we rule that &UnsafeCell<i32> -> &mut i32 is fine. (That, and you always could just use .get(), so you're not losing anything)

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jan 9, 2022
@rust-highfive
Copy link
Collaborator

r? @michaelwoerister

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 9, 2022
@chorman0773
Copy link
Contributor

I also wonder if this can't be promoted to a forbid-by-default, though perhaps that should be a separate PR that gets a crater run.

@rust-log-analyzer

This comment has been minimized.

@5225225 5225225 marked this pull request as ready for review January 9, 2022 23:31
@5225225 5225225 force-pushed the std_mem_transmute_ref_t_mut_t branch from d92fefb to 36a1141 Compare January 9, 2022 23:32
@michaelwoerister
Copy link
Member

Thanks for the PR, @5225225!

According to the "Producing an invalid value, even in private fields and locals." in the reference this sounds correct to me.

I'll ping @RalfJung to give his opinion before r+ing though.

@RalfJung
Copy link
Member

Under Stacked Borrows this transmute is indeed insta-UB, but future models might be able to fix rust-lang/unsafe-code-guidelines#133 and then the transmute could actually be okay, and only actual mutation would be problematic.

@michaelwoerister
Copy link
Member

Thanks for the info, @RalfJung!

Let's make the message stricter for now.

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Jan 20, 2022

📌 Commit 36a1141 has been approved by michaelwoerister

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 20, 2022
@RalfJung
Copy link
Member

Hm, not sure I agree with that decision -- so far we have avoided making any official statements based on Stacked Borrows.

The reality is that the rules of what exactly is and is not allowed here simply have not been decided yet. So this transmute is better avoided but the lint is a bit too definite in its statement IMO.

bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 20, 2022
…askrgr

Rollup of 13 pull requests

Successful merges:

 - rust-lang#89747 (Add MaybeUninit::(slice_)as_bytes(_mut))
 - rust-lang#89764 (Fix variant index / discriminant confusion in uninhabited enum branching)
 - rust-lang#91606 (Stabilize `-Z print-link-args` as `--print link-args`)
 - rust-lang#91694 (rustdoc: decouple stability and const-stability)
 - rust-lang#92183 (Point at correct argument when async fn output type lifetime disagrees with signature)
 - rust-lang#92582 (improve `_` constants in item signature handling)
 - rust-lang#92680 (intra-doc: Use the impl's assoc item where possible)
 - rust-lang#92704 (Change lint message to be stronger for &T -> &mut T transmute)
 - rust-lang#92861 (Rustdoc mobile: put out-of-band info on its own line)
 - rust-lang#92992 (Help optimize out backtraces when disabled)
 - rust-lang#93038 (Fix star handling in block doc comments)
 - rust-lang#93108 (:arrow_up: rust-analyzer)
 - rust-lang#93112 (Fix CVE-2022-21658)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 5c10dbd into rust-lang:master Jan 20, 2022
@rustbot rustbot added this to the 1.60.0 milestone Jan 20, 2022
@5225225 5225225 deleted the std_mem_transmute_ref_t_mut_t branch August 18, 2022 20:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants