Nothing Special   »   [go: up one dir, main page]

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

Lint against &T to &mut T and &T to &UnsafeCell<T> transmutes #128351

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ChayimFriedman2
Copy link
Contributor
@ChayimFriedman2 ChayimFriedman2 commented Jul 29, 2024

Conversion from & to &mut are and always were immediate UB, and we already lint against them, but until now the lint did not catch the case were the reference was in a field.

Conversion from & to &UnsafeCell is more nuanced: Stacked Borrows makes it immediate UB, but in Tree Borrows it is sound.

However, even in Tree Borrows it is UB to write into that reference (if the original value was Freeze). In all cases crater found where the lint triggered, the reference was written into.

Lints (mutable_transmutes existed before):

The mutable_transmutes lint catches transmuting from &T to &mut T because it is undefined behavior.

Example

unsafe {
    let y = std::mem::transmute::<&i32, &mut i32>(&5);
}

Produces:

error: transmuting &T to &mut T is undefined behavior, even if the reference is unused, consider instead using an UnsafeCell
 --> .\example.rs:5:17
  |
5 |         let y = std::mem::transmute::<&i32, &mut i32>(&5);
  |                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  |
  = note: transmute from `&i32` to `&mut i32`
  = note: `#[deny(mutable_transmutes)]` on by default

Explanation

Certain assumptions are made about aliasing of data, and this transmute
violates those assumptions. Consider using UnsafeCell instead.


The unsafe_cell_transmutes lint catches transmuting or casting from &T to &UnsafeCell<T>
because it dangerous and might be undefined behavior.

Example

use std::cell::Cell;

unsafe {
    let x = 5_i32;
    let y = std::mem::transmute::<&i32, &Cell<i32>>(&x);
    y.set(6);

    let z = &*(&x as *const i32 as *const Cell<i32>);
    z.set(7);
}

Produces:

error: transmuting &T to &UnsafeCell<T> is error-prone, rarely intentional and may cause undefined behavior
 --> .\example.rs:6:17
  |
6 |         let y = std::mem::transmute::<&i32, &Cell<i32>>(&x);
  |                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  |
  = note: transmute from `*&i32` to `(*&Cell<i32>).value`
  = note: `#[deny(unsafe_cell_transmutes)]` on by default

error: transmuting &T to &UnsafeCell<T> is error-prone, rarely intentional and may cause undefined behavior
 --> .\example.rs:9:17
  |
9 |         let z = &*(&x as *const i32 as *const Cell<i32>);
  |                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  |
  = note: transmute from `i32` to `Cell<i32>.value`

Explanation

Conversion from &T to &UnsafeCell<T> might be immediate undefined behavior, depending on
unspecified details of the aliasing model.

Even if it is not, writing to it will be undefined behavior if there was no UnsafeCell in
the original T, and even if there was, it might be undefined behavior (again, depending
on unspecified details of the aliasing model).

It is also highly dangerous and error-prone, and unlikely to be useful.

Crater summary is below.

r? compiler

@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. labels Jul 29, 2024
@rustbot
Copy link
Collaborator
rustbot commented Jul 29, 2024

The Miri subtree was changed

cc @rust-lang/miri

@oli-obk

This comment was marked as resolved.

@oli-obk
Copy link
Contributor
oli-obk commented Jul 29, 2024

@bors try

bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 29, 2024
@bors
Copy link
Contributor
bors commented Jul 29, 2024

⌛ Testing commit 5c1d66e with merge 94971c2...

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor
bors commented Jul 29, 2024

💔 Test failed - checks-actions

@rust-log-analyzer

This comment has been minimized.

@oli-obk
Copy link
Contributor
oli-obk commented Jul 29, 2024

@bors try

bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 29, 2024
…e-cell, r=<try>

[crater] Lint against &T to &mut T and &T to &UnsafeCell<T> transmutes

Needs a (check-only) crater run as per https://rust-lang.zulipchat.com/#narrow/stream/213817-t-lang/topic/Lint.20against.20.60.26.60-.3E.60.26UnsafeCell.60.20transmutes/near/454868964.

r? ghost
@bors
Copy link
Contributor
bors commented Jul 29, 2024

⌛ Trying commit 59c22c3 with merge be92fb7...

@rust-log-analyzer

This comment has been minimized.

@ChayimFriedman2

This comment was marked as resolved.

@tgross35
Copy link
Contributor

@bors try

bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 30, 2024
…e-cell, r=<try>

[crater] Lint against &T to &mut T and &T to &UnsafeCell<T> transmutes

Needs a (check-only) crater run as per https://rust-lang.zulipchat.com/#narrow/stream/213817-t-lang/topic/Lint.20against.20.60.26.60-.3E.60.26UnsafeCell.60.20transmutes/near/454868964.

r? ghost
@bors
Copy link
Contributor
bors commented Jul 30, 2024

⌛ Trying commit cc72a92 with merge 0914651...

@bors
Copy link
Contributor
bors commented Jul 30, 2024

☀️ Try build successful - checks-actions
Build commit: 0914651 (09146510cdb33d15474a35fde290187a88b1cf35)

@oli-obk
Copy link
Contributor
oli-obk commented Jul 30, 2024

@craterbot check

@craterbot
Copy link
Collaborator

👌 Experiment pr-128351 created and queued.
🤖 Automatically detected try build 0914651
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 30, 2024
@craterbot
Copy link
Collaborator

🚧 Experiment pr-128351 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@bors
Copy link
Contributor
bors commented Aug 28, 2024

☔ The latest upstream changes (presumably #129665) made this pull request unmergeable. Please resolve the merge conflicts.

@traviscross traviscross added the T-lang Relevant to the language team, which will review and decide on the PR/issue. label Sep 25, 2024
@traviscross traviscross changed the title Lint against &T to &mut T and &T to &UnsafeCell<T> transmutes Lint against &T to &mut T and &T to &UnsafeCell<T> transmutes Oct 2, 2024
@compiler-errors compiler-errors removed the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Oct 16, 2024
@nikomatsakis
Copy link
Contributor

@rfcbot fcp merge

We discussed this in the lang team. No FCP is needed for the first part (a bug fix) but the second lint is new. We are in favor, however, so starting off an FCP.

@rfcbot
Copy link
rfcbot commented Oct 16, 2024

Team member @nikomatsakis has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns.
See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Oct 16, 2024
@traviscross
Copy link
Contributor

@rfcbot reviewed

@rfcbot rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Oct 16, 2024
@rfcbot
Copy link
rfcbot commented Oct 16, 2024

🔔 This is now entering its final comment period, as per the review above. 🔔

@RalfJung
Copy link
Member

Conversion from & to &mut are and always were immediate UB, and we already lint against them, but until now the lint did not catch the case were the reference was in a field.

Not quite -- under SB, this is only UB if the shared reference points to a Freeze type.

Under TB, it's actually only UB once you write to the &mut. However, it still seems fine to lint against the Freeze case here, this is almost never correct. If we eventually decide we want to allow this, we can relax the lint.

@RalfJung
Copy link
Member

Can you add a test that transmuting &Cell<i32> to &mut is allowed? The reference is writeable so it seems odd to me to lint against that.

@traviscross traviscross removed the I-lang-nominated Nominated for discussion during a lang team meeting. label Oct 16, 2024
@ChayimFriedman2
Copy link
Contributor Author

@RalfJung

Can you add a test that transmuting &Cell<i32> to &mut is allowed? The reference is writeable so it seems odd to me to lint against that.

We already lint in this case. I can change that, but given that, is that fine to not do this in this PR?

@rust-log-analyzer

This comment has been minimized.

@RalfJung
Copy link
Member
RalfJung commented Oct 17, 2024 via email

This adds the lint against `&T`->`&UnsafeCell<T>` transmutes, and also check in struct fields, and reference casts (`&*(&a as *const u8 as *const UnsafeCell<u8>)`).

The code is quite complex; I've tried my best to simplify and comment it.

This is missing one parts: array transmutes. When transmuting an array, this only consider the first element. The reason for that is that the code is already quite complex, and I didn't want to complicate it more.
This catches the most common pattern of transmuting an array into an array of the same length with type of the same size; more complex cases are likely not properly handled.
We could take a bigger sample, for example the first and last elements to increase the chance that the lint will catch mistakes, but then the runtime complexity becomes exponential with the nesting of the arrays (`[[[[[T; 2]; 2]; 2]; 2]; 2]` has complexity of O(2**5), for instance).
@tmandry
Copy link
Member
tmandry commented Oct 24, 2024

@rfcbot reviewed

@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. to-announce Announce this issue on triage meeting and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Oct 26, 2024
@rfcbot
Copy link
rfcbot commented Oct 26, 2024

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

@jieyouxu jieyouxu added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Nov 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. 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-lang Relevant to the language team, which will review and decide on the PR/issue. to-announce Announce this issue on triage meeting
Projects
None yet
Development

Successfully merging this pull request may close these issues.