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.

Sign up for GitHub

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

Jump to bottom

Taking a raw ref (&raw (const|mut)) of a deref of pointer (*ptr) is always safe #129248

Merged
merged 2 commits into from
Oct 24, 2024
Merged

Taking a raw ref (&raw (const|mut)) of a deref of pointer (*ptr) is always safe #129248

merged 2 commits into from
Oct 24, 2024
+46 −52

Conversation

compiler-errors
Copy link
Member

T-opsem decided in rust-lang/reference#1387 that *ptr is only unsafe if the place is accessed. This means that taking a raw ref of a deref expr is always safe, since it doesn't constitute a read.

This also relaxes the DEREF_NULLPTR lint to stop warning in the case of raw ref of a deref'd nullptr, and updates its docs to reflect that change in the UB specification.

This does not change the behavior of addr_of!((*ptr).field), since field projections still require the projection is in-bounds.

I'm on the fence whether this requires an FCP, since it's something that is guaranteed by the reference you could ostensibly call this a bugfix since we were counting truly safe operations as unsafe. Perhaps someone on opsem has a strong opinion? cc @rust-lang/opsem

@rustbot
Copy link
Collaborator

rustbot commented Aug 18, 2024

r? @nnethercote

rustbot has assigned @nnethercote.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@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 Aug 18, 2024
@RalfJung
Copy link
Member

Seems more like a lang question to me than an opsem question -- from an opsem side, the relevant question is whether there is UB, and the answer is "no". But from a lang perspective, it may seem odd to have unsafe rules that depend on the place-vs-value distinction, as many programmers do not properly understand this distinction. But maybe it's good that they will wonder " 🤔 why is this safe?" and then can be taught about said distinction?

Also note that due to #117288, sometimes we do have UB just from constructing a place. That's a MIR building bug but we should probably fix it before accepting such code as safe. (Sorry I didn't remember this when we spoke earlier.)

@compiler-errors
Copy link
Member Author

👍 yep, this could be blocked on #117288 then. I think we can probably T-lang-nominate this regardless for discussion.

@compiler-errors compiler-errors added T-lang Relevant to the language team, which will review and decide on the PR/issue. I-lang-nominated Nominated for discussion during a lang team meeting. and removed T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Aug 18, 2024
@nnethercote
Copy link
Contributor

The code changes seem fine, so r=me on them if/when lang/opsem are happy with this.

@scottmcm
Copy link
Member

Thinking about the DEREF_NULLPTR relaxation, I was pondering whether we should still lint -- but not say "this is UB" when it's not, of course -- on this as something that we might want you to do in a different way instead. @compiler-errors convinced me that rustc is not the correct place for that, however, and a "clippy suspicious" or similar would be the place to do that instead, if useful.

@traviscross
Copy link
Contributor

@rustbot labels -I-lang-nominated

We discussed this in lang triage today and agreed that this was both a good idea and one that's a natural consequence of our other recent decisions and so can go without FCP.

@bors r=nnethercode

@rustbot rustbot removed the I-lang-nominated Nominated for discussion during a lang team meeting. label Aug 21, 2024
@bors
Copy link
Contributor

bors commented Aug 21, 2024

📌 Commit 25d617f has been approved by nnethercode

It is now in the queue for this repository.

@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 Aug 21, 2024
@compiler-errors
Copy link
Member Author

@bors r-

This is still unsound!! #117288

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Aug 21, 2024
@compiler-errors compiler-errors added S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 21, 2024
@traviscross
Copy link
Contributor

Good catch. Thanks.

@RalfJung RalfJung mentioned this pull request Aug 21, 2024
Closed
@compiler-errors compiler-errors mentioned this pull request Aug 21, 2024
Closed
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@workingjubilee
Copy link
Member

warning: the feature raw_ref_op has been stable since 1.82.0-nightly and no longer requires an attribute to enable

Truly?!

@RalfJung
Copy link
Member

Yes, truly. :)

#127679

@RalfJung
Copy link
Member

So let _ = *raw_ptr; and match *raw_ptr { _ => ... } are not affected by this?

That does seem a bit inconsistent. It doesn't seem super useful to write code like that, but then it doesn't seem super useful to write &raw const *ptr either.

@compiler-errors
Copy link
Member Author

@RalfJung: No, that would need to have another special case in the THIR.

@workingjubilee workingjubilee mentioned this pull request Aug 30, 2024
Closed
4 tasks
@bors
Copy link
Contributor

bors commented Sep 15, 2024

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

@compiler-errors
Copy link
Member Author

That does seem a bit inconsistent. It doesn't seem super useful to write code like that, but then it doesn't seem super useful to write &raw const *ptr either.

🤷 I'd still like to fix it, regardless, I think, especially now that #117288 is closed. I'd also like if let _ = *raw_ptr; weren't unsafe, but that's far more difficult for unsafeck to implement.

@scottmcm
Copy link
Member

scottmcm commented Oct 7, 2024

I'd also like if let _ = *raw_ptr; weren't unsafe

I mean, we intentionally changed it to require being in an unsafe block...

@compiler-errors
Copy link
Member Author

compiler-errors commented Oct 7, 2024 •
edited
Loading

Because it was unsound? Could you provide some context?

@compiler-errors
Copy link
Member Author

also this is

@rustbot ready

:D

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-blocked Status: Blocked on something else such as an RFC or other implementation work. labels Oct 11, 2024
@nnethercote
Copy link
Contributor

@RalfJung, @scottmcm: any objections before I approve this?

@RalfJung
Copy link
Member

Procedurally it has been approved by t-lang. I'm not very familiar with THIR so it's hard for me to say whether the implementation matches the intent. We don't have a dedicated test for the cases that should be rejected, but tests/ui/lint/lint-deref-nullptr.rs and tests/ui/static/raw-ref-deref-without-unsafe.rs to cover the most important cases I can think of.

@nnethercote
Copy link
Contributor

I think this has waited long enough.

@bors r+

@bors
Copy link
Contributor

bors commented Oct 24, 2024

📌 Commit 367183b has been approved by nnethercote

It is now in the queue for this repository.

@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 Oct 24, 2024
@matthiaskrgr matthiaskrgr mentioned this pull request Oct 24, 2024
Merged
@RalfJung RalfJung added the relnotes Marks issues that should be documented in the release notes of the next release. label Oct 24, 2024
@rustbot rustbot mentioned this pull request Oct 24, 2024
Open
3 tasks
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 24, 2024
@bors
Auto merge of rust-lang#132099 - matthiaskrgr:rollup-myi94r8, r=matth…
5ae4d75 
…iaskrgr

Rollup of 5 pull requests

Successful merges:

 - rust-lang#129248 (Taking a raw ref (`&raw (const|mut)`) of a deref of pointer (`*ptr`) is always safe)
 - rust-lang#131906 (rustdoc: adjust spacing and typography in header)
 - rust-lang#132084 (Consider param-env candidates even if they have errors)
 - rust-lang#132096 (Replace an FTP link in comments with an equivalent HTTPS link)
 - rust-lang#132098 (rustc_feature::Features: explain what that 'Option<Symbol>' is about)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 93bf791 into rust-lang:master Oct 24, 2024
6 checks passed
@rustbot rustbot added this to the 1.84.0 milestone Oct 24, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Oct 24, 2024
@rust-timer
Unrolled build for rust-lang#129248
5a36bc0 
Rollup merge of rust-lang#129248 - compiler-errors:raw-ref-deref, r=nnethercote

Taking a raw ref (`&raw (const|mut)`) of a deref of pointer (`*ptr`) is always safe

T-opsem decided in rust-lang/reference#1387 that `*ptr` is only unsafe if the place is accessed. This means that taking a raw ref of a deref expr is always safe, since it doesn't constitute a read.

This also relaxes the `DEREF_NULLPTR` lint to stop warning in the case of raw ref of a deref'd nullptr, and updates its docs to reflect that change in the UB specification.

This does not change the behavior of `addr_of!((*ptr).field)`, since field projections still require the projection is in-bounds.

I'm on the fence whether this requires an FCP, since it's something that is guaranteed by the reference you could ostensibly call this a bugfix since we were counting truly safe operations as unsafe. Perhaps someone on opsem has a strong opinion? cc `@rust-lang/opsem`
@Veykril Veykril mentioned this pull request Dec 18, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@workingjubilee workingjubilee workingjubilee left review comments

Assignees

@nnethercote nnethercote

Labels
relnotes Marks issues that should be documented in the release notes of the next release. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.84.0
Development

Successfully merging this pull request may close these issues.
9 participants
@compiler-errors @rustbot @RalfJung @nnethercote @scottmcm @traviscross @bors @rust-log-analyzer @workingjubilee