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

Add small clarification around using pointers derived from references #103996

Merged
merged 1 commit into from
Nov 13, 2022

Conversation

SUPERCILEX
Copy link
Contributor

r? @RalfJung

One question about your example from rust-lang/libs-team#122: at what point does UB arise? If writing 0 does not cause UB and the reference x is never read or written to (explicitly or implicitly by being wrapped in another data structure) after the call to foo, does UB only arise when dropping the value? I don't really get that since I thought references were always supposed to point to valid data?

fn foo(x: &mut NonZeroI32)  {
  let ptr = x as *mut NonZeroI32;
  unsafe { ptr.cast::<i32>().write(0); } // no UB here
  // What now? x is considered garbage when?
}

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Nov 5, 2022
@rustbot
Copy link
Collaborator

rustbot commented Nov 5, 2022

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

  • Stabilizing library features
  • Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
  • Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
  • Changing public documentation in ways that create new stability guarantees
  • Changing observable runtime behavior of library APIs

@inquisitivecrystal inquisitivecrystal added the A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools label Nov 5, 2022
Comment on lines 38 to 39
//! access the same memory. That is, reference and pointer accesses cannot be
//! interleaved—they must follow stacked borrows.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stacked borrows is an experimental unofficial memory model, so the official docs cannot say that people "must follow stacked borrows". Maybe saying something along the lines of "The restrictions of using raw pointers in combination with references have not been decided yet, so it is advised to not interleave accesses using raw pointers and references.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, agreed. What question is this even answering? It seems unrelated to the other change.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yeah, sorry this should probably be in another PR. The current wording makes it sound like you cannot have a reference and pointer around at the same time. I'm trying to clarify that... maybe I can just remove "—they must follow stacked borrows"?

Copy link
Member

@RalfJung RalfJung Nov 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It already says that no reference can be "used to access" the same memory while the pointer is used. Not sure how "accesses cannot be interleaved" adds any new information here?

I guess I am fine with clarifying, but I really don't see how the current wording "makes it sound like you cannot have a reference and pointer around at the same time" in any way that the new wording would fix.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm, yeah this may not be the best clarification, though sometimes saying the same thing in different ways is still helpful. 🤷‍♂️ If I'm remembering correctly, I kind of got the feeling that the reference was "consumed" by casting it to a pointer when first reading this. So maybe saying this would be better? "That is, the reference may only be used when there are no more accesses through the pointer." Though I feel like interleaving expresses that idea more clearly?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess the "that is" already states that this sentence is a clarification, not a new rule.

So if you remove the mention of stacked borrows this is probably fine to land.

@RalfJung
Copy link
Member

RalfJung commented Nov 5, 2022

One question about your example from rust-lang/libs-team#122: at what point does UB arise? If writing 0 does not cause UB and the reference x is never read or written to (explicitly or implicitly by being wrapped in another data structure) after the call to foo, does UB only arise when dropping the value? I don't really get that since I thought references were always supposed to point to valid data?

That's an open question: rust-lang/unsafe-code-guidelines#84.

References must point to valid data whenever they are used as a value (passed to a function, assigned, anything like that). But when they are just sitting there, the situation is much less clear.

//! it is read, either through the raw pointer or the reference it originated from. This means a type
//! may freely transition between valid and invalid states when being written to by raw pointers.
//! Thus, when discussing safety, it may be useful to separately assert the validity of the pointer vs.
//! the validity of the data it points to.
Copy link
Member

@RalfJung RalfJung Nov 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This means a type may freely transition between valid and invalid states when being written to by raw pointers.

This has not really been decided yet either -- see rust-lang/unsafe-code-guidelines#84. I possibly used too definite language in rust-lang/libs-team#122.

I am also not sure what you mean by pointer and pointee invariant here. It is already the case that https://doc.rust-lang.org/nightly/std/ptr/fn.read.html says "src must point to a properly initialized value of type T". https://doc.rust-lang.org/nightly/std/ptr/fn.write.html does not say anything like that because it takes a T as argument, which already implicitly says that it must be a valid T.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, but large parts of unsafe Rust are simply still uncharted territory. :(

Haha yep, the deeper I dig the more that seems to be true. :)

pointee invariant here.

If we decide this should go through I'll clarify that.

which already implicitly says that it must be a valid T.

This is specifically about the as_bytes case, so maybe I should make the idea of breaking down a larger type into smaller pieces clearer. I want to be explicit that it is ok to write valid data that causes a type to become invalid so long as the type becomes valid again by the time it's read.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want to be explicit that it is ok to write valid data that causes a type to become invalid so long as the type becomes valid again by the time it's read.

And I'm saying I think we can't way this in the official docs yet because rust-lang/unsafe-code-guidelines#84 has not been officially answered yet.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotya, so unfortunately this whole paragraph is dead? I'll just keep the small clarification if so.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I am afraid so. :(

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@RalfJung
Copy link
Member

RalfJung commented Nov 5, 2022

I think there is general consensus that this is sound

fn foo(x: &mut NonZeroI32)  {
  let ptr = x as *mut NonZeroI32;
  unsafe { ptr.cast::<i32>().write(0); }
  unsafe { ptr.cast::<i32>().write(1); }
}

but even that would need lang-team FCP.

These are probably questions that we simply cannot answer for now and need to defer until the to-be-created operational semantics team gets around to them, and an RFC or FCP is passed on the relevant aspect of the semantics. Sorry, but large parts of unsafe Rust are simply still uncharted territory. :(

Signed-off-by: Alex Saveau <saveau.alexandre@gmail.com>
@SUPERCILEX SUPERCILEX changed the title Add clarifying docs on pointer read write invariants @SUPERCILEX Add small clarification around using pointers derived from references Nov 6, 2022
@SUPERCILEX SUPERCILEX changed the title @SUPERCILEX Add small clarification around using pointers derived from references Add small clarification around using pointers derived from references Nov 6, 2022
@RalfJung
Copy link
Member

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Nov 13, 2022

📌 Commit 28ea002 has been approved by RalfJung

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 Nov 13, 2022
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 13, 2022
…iaskrgr

Rollup of 5 pull requests

Successful merges:

 - rust-lang#103996 (Add small clarification around using pointers derived from references)
 - rust-lang#104315 (Improve spans with `use crate::{self}`)
 - rust-lang#104320 (Use `derive_const` and rm manual StructuralEq impl)
 - rust-lang#104357 (add is_sized method on Abi and Layout, and use it)
 - rust-lang#104365 (Add x tool to triagebot)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit a1b0702 into rust-lang:master Nov 13, 2022
@rustbot rustbot added this to the 1.67.0 milestone Nov 13, 2022
Aaron1011 pushed a commit to Aaron1011/rust that referenced this pull request Jan 6, 2023
Add small clarification around using pointers derived from references

r? `@RalfJung`

One question about your example from rust-lang/libs-team#122: at what point does UB arise? If writing 0 does not cause UB and the reference `x` is never read or written to (explicitly or implicitly by being wrapped in another data structure) after the call to `foo`, does UB only arise when dropping the value? I don't really get that since I thought references were always supposed to point to valid data?

```rust
fn foo(x: &mut NonZeroI32)  {
  let ptr = x as *mut NonZeroI32;
  unsafe { ptr.cast::<i32>().write(0); } // no UB here
  // What now? x is considered garbage when?
}
```
Aaron1011 pushed a commit to Aaron1011/rust that referenced this pull request Jan 6, 2023
…iaskrgr

Rollup of 5 pull requests

Successful merges:

 - rust-lang#103996 (Add small clarification around using pointers derived from references)
 - rust-lang#104315 (Improve spans with `use crate::{self}`)
 - rust-lang#104320 (Use `derive_const` and rm manual StructuralEq impl)
 - rust-lang#104357 (add is_sized method on Abi and Layout, and use it)
 - rust-lang#104365 (Add x tool to triagebot)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants