-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
base: master
Are you sure you want to change the base?
Lint against &T
to &mut T
and &T
to &UnsafeCell<T>
transmutes
#128351
Conversation
The Miri subtree was changed cc @rust-lang/miri |
This comment was marked as resolved.
This comment was marked as resolved.
f27dba0
to
d20e89c
Compare
@bors try |
d20e89c
to
5c1d66e
Compare
…e-cell, r= [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
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
💔 Test failed - checks-actions |
5c1d66e
to
b62dc4c
Compare
This comment has been minimized.
This comment has been minimized.
b62dc4c
to
59c22c3
Compare
@bors try |
…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
This comment has been minimized.
This comment has been minimized.
59c22c3
to
cc72a92
Compare
This comment was marked as resolved.
This comment was marked as resolved.
@bors try |
…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
☀️ Try build successful - checks-actions |
@craterbot check |
👌 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
🚧 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
☔ The latest upstream changes (presumably #129665) made this pull request unmergeable. Please resolve the merge conflicts. |
&T
to &mut T
and &T
to &UnsafeCell<T>
transmutes
@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. |
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. |
@rfcbot reviewed |
🔔 This is now entering its final comment period, as per the review above. 🔔 |
Not quite -- under SB, this is only UB if the shared reference points to a Under TB, it's actually only UB once you write to the |
Can you add a test that transmuting |
We already lint in this case. I can change that, but given that, is that fine to not do this in this PR? |
d19a506
to
8c137f7
Compare
This comment has been minimized.
This comment has been minimized.
Hm, just makes me wonder if there are any other wrong assumptions encoded in the lint...
|
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).
8c137f7
to
573ee3f
Compare
@rfcbot reviewed |
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. |
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):Crater summary is below.
r? compiler