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

Unsoundness: Patterns in function parameters are not checked for union access #130528

Closed
ChayimFriedman2 opened this issue Sep 18, 2024 · 5 comments · Fixed by #130531
Closed
Assignees
Labels
C-bug Category: This is a bug. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness P-critical Critical priority regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@ChayimFriedman2
Copy link
Contributor

ChayimFriedman2 commented Sep 18, 2024

I tried this code:

union U {
    a: &'static i32,
    b: usize,
}

fn foo(U { a }: U) {
    dbg!(*a);
}

fn main() {
    foo(U { b: 0 });
}

Playground.

I expected to see this happen: rustc rejecting this code. We are transmuting arbitrary types without unsafe, this is clearly unsound.

Instead, this happened: rustc accepted this code, and it SIGSEGVs at runtime.

@ChayimFriedman2 ChayimFriedman2 added the C-bug Category: This is a bug. label Sep 18, 2024
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Sep 18, 2024
@Urgau Urgau added regression-from-stable-to-stable Performance or correctness regression from one stable version to another. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness labels Sep 18, 2024
@rustbot rustbot added I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Sep 18, 2024
@Urgau
Copy link
Member

Urgau commented Sep 18, 2024

Seems to have regressed between 1.76 and 1.77.

In 1.76 (and below) it gave:

error[E0133]: access to union field is unsafe and requires unsafe function or block
 --> <source>:6:12
  |
6 | fn foo(U { a }: U) {
  |            ^ access to union field
  |
  = note: the field may not be properly initialized: using uninitialized data will cause undefined behavior

It's probably related to the THIR unsafeck which was stabilized in that release.

@compiler-errors
Copy link
Member

I'll take a look at this. I think I may have found out what's going on.

@rustbot claim

@saethlin saethlin added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Sep 18, 2024
workingjubilee added a commit to workingjubilee/rustc that referenced this issue Sep 19, 2024
…m, r=Urgau

Check params for unsafety in THIR

Self-explanatory. I'm not surprised this was overlooked, given the way that THIR visitors work. Perhaps we should provide a better entrypoint.

Fixes rust-lang#130528
workingjubilee added a commit to workingjubilee/rustc that referenced this issue Sep 19, 2024
…m, r=Urgau

Check params for unsafety in THIR

Self-explanatory. I'm not surprised this was overlooked, given the way that THIR visitors work. Perhaps we should provide a better entrypoint.

Fixes rust-lang#130528
workingjubilee added a commit to workingjubilee/rustc that referenced this issue Sep 19, 2024
…, r=Nadrieril

Never patterns constitute a read for unsafety

This code is otherwise unsound if we don't emit an unsafety error here. Noticed when fixing rust-lang#130528, but it's totally unrelated.

r? `@Nadrieril`
@apiraino
Copy link
Contributor

WG-prioritization assigning priority (Zulip discussion).

@rustbot label -I-prioritize +P-critical

@rustbot rustbot added P-critical Critical priority and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Sep 19, 2024
@bors bors closed this as completed in 944df8e Sep 19, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Sep 19, 2024
Rollup merge of rust-lang#130533 - compiler-errors:never-pat-unsafeck, r=Nadrieril

Never patterns constitute a read for unsafety

This code is otherwise unsound if we don't emit an unsafety error here. Noticed when fixing rust-lang#130528, but it's totally unrelated.

r? `@Nadrieril`
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Sep 19, 2024
Rollup merge of rust-lang#130531 - compiler-errors:thir-unsafeck-param, r=Urgau

Check params for unsafety in THIR

Self-explanatory. I'm not surprised this was overlooked, given the way that THIR visitors work. Perhaps we should provide a better entrypoint.

Fixes rust-lang#130528
@compiler-errors
Copy link
Member

Reopening for beta nomination.

@wesleywiser
Copy link
Member

Fix backported via #130531

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness P-critical Critical priority regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants