clarify that addr_of creates read-only pointers #129653

RalfJung · 2024-08-27T10:23:27Z

Stacked Borrows does make this UB, but Tree Borrows does not. This is tied up with #56604 and other UCG discussions. Also see this collection of links where rustc treats addr_of! as a "non-mutating use".

So, let's better be careful for now.

rustbot · 2024-08-27T10:23:36Z

r? @scottmcm

rustbot has assigned @scottmcm.
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

RalfJung · 2024-08-27T10:23:41Z

Cc @rust-lang/opsem

ojeda · 2024-08-27T10:30:48Z

This note is definitely useful when interpreting the docs of addr_of!. Thanks!

Darksonn · 2024-08-27T12:09:33Z

library/core/src/ptr/mod.rs

+/// It is still an open question whether writing through an `addr_of!`-created pointer is permitted
+/// or not. Until that is decided, the same rules as for shared references apply: it is UB to write
+/// through a pointer created with this operation, except for bytes located inside an `UnsafeCell`.


This wording sounds like taking a raw pointer raw_ptr and transforming it using addr_of!((*raw_ptr).field) results in a pointer that only has read-only permissions, even if raw_ptr has read/write permissions. Is that intended?

Hm... that is currently not the case, but given that this is all undecided, maybe we should say that indeed this makes a read-only pointer? If there's a good reason to be more fine-grained here I am open to that, the only concern here is that it makes it much harder to say what is and isn't read-only.

Uh ... I think that would be problematic. I would think that many people, like me, go around and assume that as long as you stay in raw pointer land, you never give up permissions on your pointers.

Yeah, addr_of! would be a bit of a pitfall, but given the wording I would have expected that.

Do we want an addr_of_const! instead, and tell people to use that unless you are very careful with addr_of!?

I'm not sure a third macro makes sense. I think addr_of! should guarantee that if both the input and output types are raw pointers, then permissions are preserved and the behavior is identical to using ptr::offset. It's very useful for teaching if I can say that the only thing that matters for the permissions of raw pointers is where it originally comes from. Adding exceptions to that general rule is counterproductive.

In the same vein, the usual motivation for not roundtripping through &T is that doing so gives up permissions to write. The entire point of addr_of! is that it does not roundtrip through &T.

When doing addr_of!(MY_STATIC) you are creating the raw pointer, so it is consistent with the above rule that the resulting pointer is read-only. In this case, the addr_of! is where the raw pointer originally came from, so this decides its permissions.

both the input and output types are raw pointers

The thing is, the input is a place, it's not raw or anything. So we have to distinguish on "what is the place based on". It's annoying. But more UB is also annoying so I'll give it a shot.

Darksonn · 2024-08-27T13:15:15Z

library/core/src/ptr/mod.rs

 /// It is still an open question whether writing through an `addr_of!`-created pointer is permitted
-/// or not. Until that is decided, the same rules as for shared references apply: it is UB to write
-/// through a pointer created with this operation, except for bytes located inside an `UnsafeCell`.
+/// or not. Specifically, if the place `expr` evaluates to is based on a raw pointer, then the
+/// result of `addr_of!` inherits all permissions from that raw pointer. However, if the place is
+/// based on a reference, local variable, or `static`, then until all details are decided, the same
+/// rules as for shared references apply: it is UB to write through a pointer created with this
+/// operation, except for bytes located inside an `UnsafeCell`.


Maybe it would make more sense to just state "the rules today are so and so, the pointer is always read-only unless the input is a raw pointer in which case permissions are preserved". And then say that these rules could be relaxed in the future.

Not sure what that would improve, sounds equivalent.

library/core/src/ptr/mod.rs

traviscross · 2024-08-28T07:07:05Z

cc @rust-lang/lang

traviscross · 2024-08-28T07:09:40Z

@rustbot labels +I-lang-nominated

Let's nominate to discuss. I went back and forth on whether to nominate, but it's probably worth us having a look, if nothing else so as to remind ourselves about the #56604 situation.

scottmcm

I think this makes good sense to me. Clarifying that this is already how things are working seems like an improvement to do now regardless of whether we want to change things in the future. (And those future changes could easily end up tied up in things like whether we should have a const/mut-agnostic pointer type that a addr_of_flexible!(MY_STATIC) could return.)

So I'll plan to r+ this unless there's opposition in the meeting on Wed -- I don't think a warning about current behaviour needs an FCP.

scottmcm · 2024-09-04T16:01:45Z

This was raised briefly in the lang triage meeting today, and nobody had objection to documenting the open question here.

Thanks for writing it!

@bors r+ rollup

bors · 2024-09-04T16:01:48Z

📌 Commit b5bd0fe has been approved by scottmcm

It is now in the queue for this repository.

…tmcm clarify that addr_of creates read-only pointers Stacked Borrows does make this UB, but Tree Borrows does not. This is tied up with rust-lang#56604 and other UCG discussions. Also see [this collection of links](Rust-for-Linux/linux#950 (comment)) where rustc treats `addr_of!` as a "non-mutating use". So, let's better be careful for now.

…iaskrgr Rollup of 12 pull requests Successful merges: - rust-lang#128919 (Add an internal lint that warns when accessing untracked data) - rust-lang#129471 ([rustdoc] Sort impl associated items by kinds and then by appearance) - rust-lang#129653 (clarify that addr_of creates read-only pointers) - rust-lang#129775 (bootstrap: Try to track down why `initial_libdir` sometimes fails) - rust-lang#129939 (explain why Rvalue::Len still exists) - rust-lang#129940 (s390x: Fix a regression related to backchain feature) - rust-lang#129942 (copy rustc rustlib artifacts from ci-rustc) - rust-lang#129943 (use the bootstrapped compiler for `test-float-parse` test) - rust-lang#129944 (Add compat note for trait solver change) - rust-lang#129947 (Add digit separators in `Duration` examples) - rust-lang#129955 (Temporarily remove fmease from the review rotation) - rust-lang#129957 (forward linker option to lint-docs) r? `@ghost` `@rustbot` modify labels: rollup

…tmcm clarify that addr_of creates read-only pointers Stacked Borrows does make this UB, but Tree Borrows does not. This is tied up with rust-lang#56604 and other UCG discussions. Also see [this collection of links](Rust-for-Linux/linux#950 (comment)) where rustc treats `addr_of!` as a "non-mutating use". So, let's better be careful for now.

…iaskrgr Rollup of 12 pull requests Successful merges: - rust-lang#128919 (Add an internal lint that warns when accessing untracked data) - rust-lang#129472 (fix ICE when `asm_const` and `const_refs_to_static` are combined) - rust-lang#129653 (clarify that addr_of creates read-only pointers) - rust-lang#129775 (bootstrap: Try to track down why `initial_libdir` sometimes fails) - rust-lang#129939 (explain why Rvalue::Len still exists) - rust-lang#129940 (s390x: Fix a regression related to backchain feature) - rust-lang#129942 (copy rustc rustlib artifacts from ci-rustc) - rust-lang#129943 (use the bootstrapped compiler for `test-float-parse` test) - rust-lang#129944 (Add compat note for trait solver change) - rust-lang#129947 (Add digit separators in `Duration` examples) - rust-lang#129955 (Temporarily remove fmease from the review rotation) - rust-lang#129957 (forward linker option to lint-docs) Failed merges: - rust-lang#129471 ([rustdoc] Sort impl associated items by kinds and then by appearance) r? `@ghost` `@rustbot` modify labels: rollup

…iaskrgr Rollup of 11 pull requests Successful merges: - rust-lang#128919 (Add an internal lint that warns when accessing untracked data) - rust-lang#129472 (fix ICE when `asm_const` and `const_refs_to_static` are combined) - rust-lang#129653 (clarify that addr_of creates read-only pointers) - rust-lang#129775 (bootstrap: Try to track down why `initial_libdir` sometimes fails) - rust-lang#129939 (explain why Rvalue::Len still exists) - rust-lang#129942 (copy rustc rustlib artifacts from ci-rustc) - rust-lang#129943 (use the bootstrapped compiler for `test-float-parse` test) - rust-lang#129944 (Add compat note for trait solver change) - rust-lang#129947 (Add digit separators in `Duration` examples) - rust-lang#129955 (Temporarily remove fmease from the review rotation) - rust-lang#129957 (forward linker option to lint-docs) r? `@ghost` `@rustbot` modify labels: rollup

…tmcm clarify that addr_of creates read-only pointers Stacked Borrows does make this UB, but Tree Borrows does not. This is tied up with rust-lang#56604 and other UCG discussions. Also see [this collection of links](Rust-for-Linux/linux#950 (comment)) where rustc treats `addr_of!` as a "non-mutating use". So, let's better be careful for now.

…iaskrgr Rollup of 11 pull requests Successful merges: - rust-lang#128919 (Add an internal lint that warns when accessing untracked data) - rust-lang#129472 (fix ICE when `asm_const` and `const_refs_to_static` are combined) - rust-lang#129653 (clarify that addr_of creates read-only pointers) - rust-lang#129775 (bootstrap: Try to track down why `initial_libdir` sometimes fails) - rust-lang#129939 (explain why Rvalue::Len still exists) - rust-lang#129942 (copy rustc rustlib artifacts from ci-rustc) - rust-lang#129943 (use the bootstrapped compiler for `test-float-parse` test) - rust-lang#129944 (Add compat note for trait solver change) - rust-lang#129947 (Add digit separators in `Duration` examples) - rust-lang#129955 (Temporarily remove fmease from the review rotation) - rust-lang#129957 (forward linker option to lint-docs) r? `@ghost` `@rustbot` modify labels: rollup

…kingjubilee Rollup of 14 pull requests Successful merges: - rust-lang#128919 (Add an internal lint that warns when accessing untracked data) - rust-lang#129021 (Check WF of source type's signature on fn pointer cast) - rust-lang#129472 (fix ICE when `asm_const` and `const_refs_to_static` are combined) - rust-lang#129653 (clarify that addr_of creates read-only pointers) - rust-lang#129775 (bootstrap: Try to track down why `initial_libdir` sometimes fails) - rust-lang#129781 (Make `./x.py <cmd> compiler/<crate>` aware of the crate's features) - rust-lang#129939 (explain why Rvalue::Len still exists) - rust-lang#129942 (copy rustc rustlib artifacts from ci-rustc) - rust-lang#129944 (Add compat note for trait solver change) - rust-lang#129947 (Add digit separators in `Duration` examples) - rust-lang#129955 (Temporarily remove fmease from the review rotation) - rust-lang#129957 (forward linker option to lint-docs) - rust-lang#129969 (Make `Ty::boxed_ty` return an `Option`) - rust-lang#129995 (Remove wasm32-wasip2's tier 2 status from release notes) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#129653 - RalfJung:addr-of-read-only, r=scottmcm clarify that addr_of creates read-only pointers Stacked Borrows does make this UB, but Tree Borrows does not. This is tied up with rust-lang#56604 and other UCG discussions. Also see [this collection of links](Rust-for-Linux/linux#950 (comment)) where rustc treats `addr_of!` as a "non-mutating use". So, let's better be careful for now.

clarify that addr_of creates read-only pointers

1ef4f5d

rustbot assigned scottmcm Aug 27, 2024

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 Aug 27, 2024

ojeda mentioned this pull request Aug 27, 2024

core wanted features & bugfixes Rust-for-Linux/linux#514

Open

13 tasks

Darksonn reviewed Aug 27, 2024

View reviewed changes

est31 reviewed Aug 27, 2024

View reviewed changes

library/core/src/ptr/mod.rs Outdated Show resolved Hide resolved

traviscross added the T-lang Relevant to the language team, which will review and decide on the PR/issue. label Aug 28, 2024

rustbot added the I-lang-nominated Nominated for discussion during a lang team meeting. label Aug 28, 2024

RalfJung force-pushed the addr-of-read-only branch from a14b1db to 5d995d5 Compare August 28, 2024 07:35

addr_of on places derived from raw pointers should preserve permissions

b5bd0fe

RalfJung force-pushed the addr-of-read-only branch from 5d995d5 to b5bd0fe Compare August 28, 2024 08:32

scottmcm approved these changes Aug 31, 2024

View reviewed changes

saethlin added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 31, 2024

scottmcm removed the S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). label Sep 4, 2024

bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Sep 4, 2024

scottmcm mentioned this pull request Sep 4, 2024

Coercing &mut to *const should not create a shared reference #56604

Open

scottmcm removed the I-lang-nominated Nominated for discussion during a lang team meeting. label Sep 4, 2024

joshlf mentioned this pull request Sep 4, 2024

Audit uses of addr_of! for read-only-ness google/zerocopy#1607

Closed

tgross35 mentioned this pull request Sep 5, 2024

Rollup of 7 pull requests #129976

Closed

matthiaskrgr mentioned this pull request Sep 5, 2024

Rollup of 13 pull requests #129977

Closed

matthiaskrgr mentioned this pull request Sep 5, 2024

Rollup of 12 pull requests #129979

Closed

matthiaskrgr mentioned this pull request Sep 5, 2024

Rollup of 12 pull requests #129984

Closed

matthiaskrgr mentioned this pull request Sep 5, 2024

Rollup of 11 pull requests #129999

Merged

workingjubilee mentioned this pull request Sep 6, 2024

Rollup of 15 pull requests #130007

Closed

workingjubilee mentioned this pull request Sep 6, 2024

Rollup of 14 pull requests #130010

Closed

bors merged commit 85d15d2 into rust-lang:master Sep 6, 2024
6 checks passed

rustbot added this to the 1.83.0 milestone Sep 6, 2024

RalfJung deleted the addr-of-read-only branch September 9, 2024 08:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

clarify that addr_of creates read-only pointers #129653

clarify that addr_of creates read-only pointers #129653

RalfJung commented Aug 27, 2024

rustbot commented Aug 27, 2024

RalfJung commented Aug 27, 2024

ojeda commented Aug 27, 2024

Darksonn Aug 27, 2024

RalfJung Aug 27, 2024

Darksonn Aug 27, 2024

ojeda Aug 27, 2024

Darksonn Aug 27, 2024

RalfJung Aug 27, 2024

Darksonn Aug 27, 2024

RalfJung Aug 27, 2024

traviscross commented Aug 28, 2024

traviscross commented Aug 28, 2024

scottmcm left a comment

scottmcm commented Sep 4, 2024

bors commented Sep 4, 2024

clarify that addr_of creates read-only pointers #129653

clarify that addr_of creates read-only pointers #129653

Conversation

RalfJung commented Aug 27, 2024

rustbot commented Aug 27, 2024

RalfJung commented Aug 27, 2024

ojeda commented Aug 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

traviscross commented Aug 28, 2024

traviscross commented Aug 28, 2024

scottmcm left a comment

Choose a reason for hiding this comment

scottmcm commented Sep 4, 2024

bors commented Sep 4, 2024