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

Fix doc for pointer's validity in zst operation #134912

Closed
wants to merge 1 commit into from

Conversation

DiuDiu777
Copy link
Contributor

PR Description

This pr addresses a discrepancy in the documentation regarding pointer validity for zero-sized types (ZSTs).

According to the documentation on core::ptr, it states that

For operations of size zero, every pointer is valid, including the null pointer.

However, this is contradicted by the documentation on core::ptr::read, which specifies that

even if T has size 0, the pointer must be non-null and properly aligned.

This creates a contradiction, as core::ptr::read explicitly states that pointers to ZSTs must be non-null, while the core::ptr documentation implies that null pointers are valid for ZSTs.

Suggestion

Besides, I would like to suggest a revision to the following sentence in the core::ptr documentation:

We say that a pointer is “dangling” if it is not valid for any non-zero-sized accesses. This means out-of-bounds pointers, pointers to freed memory, null pointers, and pointers created with NonNull::dangling are all dangling.

The term "dangling" traditionally refers to a pointer that still exists but points to memory that has been freed or is otherwise invalid. In contrast, a null pointer is not considered a "dangling" pointer, but rather an invalid pointer by definition.

So in this context, I think replacing "dangling" with "invalid" would be better?

@rustbot
Copy link
Collaborator

rustbot commented Dec 30, 2024

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Noratrieb (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

  • @rustbot author: the review is finished, PR author should check the comments and take action accordingly
  • @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

@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 Dec 30, 2024
Copy link
Member

@Noratrieb Noratrieb left a comment

Choose a reason for hiding this comment

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

This is the wrong way around, null is valid for zero-sized accesses. See Miri's output for this code (and ignore the lint).
You should fix the ptr::read docs instead

cc @RalfJung maybe you have opinions on how to best structure it

@rustbot rustbot 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-review Status: Awaiting review from the assignee but also interested parties. labels Dec 30, 2024
@RalfJung
Copy link
Member

RalfJung commented Dec 30, 2024

This has already been fixed as far as I can tell -- the docs at https://doc.rust-lang.org/nightly/std/ptr/fn.read.html do not contain the sentence you quote.

The term "dangling" traditionally refers to a pointer that still exists but points to memory that has been freed or is otherwise invalid. In contrast, a null pointer is not considered a "dangling" pointer, but rather an invalid pointer by definition.

"dangling" is used in different ways in different places, I don't think there is a standard definition. That's why we make our definition explicit. I don't see a reason to change the terminology here.

Also your PR doesn't even change anything here. Why are your PR description and the PR contents so disconnected?

@hxuhack
Copy link

hxuhack commented Dec 30, 2024

As @RalfJung mentioned, the issue with read has been resolved in the nightly version. However, several other APIs still require a valid non-null pointer for ZSTs, such as write_unaligned, read_unaligned, from_raw_parts,from_raw_parts_mut, and drop_in_place.
My question is: is it too strong to claim that all null pointers to ZSTs are valid in Rust?

@RalfJung
Copy link
Member

There are many different notions of "valid". Nobody claims that null pointers are always valid. The claim is that they are valid for reads and writes of size 0.

the _unaligned method docs should be fixed. The rest indeed does not allow null pointers.

@hxuhack
Copy link

hxuhack commented Dec 30, 2024

The doc of core::ptr states that For operations of size zero, every pointer is valid, including the null pointer.
From my understanding, the claim is equivalent to that all null pointers to ZSTs are valid in Rust.
Is it true? If not, what are the constraints for a null pointer to a ZST to be considered valid?

@RalfJung
Copy link
Member

RalfJung commented Dec 30, 2024

Context matters. You are taking that sentence out of context.

Many functions in this module take raw pointers as arguments and read from or write to them. For this to be safe, these pointers must be valid for the given access. Whether a pointer is valid depends on the operation it is used for (read or write), and the extent of the memory that is accessed (i.e., how many bytes are read/written) – it makes no sense to ask “is this pointer valid”; one has to ask “is this pointer valid for a given access”. Most functions use *mut T and *const T to access only a single value, in which case the documentation omits the size and implicitly assumes it to be size_of::() bytes.

Again, the question "is X valid" is meaningless. You have to ask "valid for what". The pointer ptr::dangling::<i32>() is perfectly valid in some sense (it can be created in safe code after all, and e.g. using it with == is entirely risk-free), but invalid for some uses (e.g. ptr::dangling::<i32>().read() is UB).

@hxuhack
Copy link

hxuhack commented Dec 31, 2024

@RalfJung Thank you for your responses. I am trying to understand why from_raw_parts and from_raw_parts_mut require non-null pointers for ZSTs. Additionally, I’m exploring the fundamental rules for determining whether a null pointer to a ZST type can be valid in a given context. Is this requirement because references to ZSTs (including empty slices), like all other references, must be non-null, as specified in the Nomicon's section on exotically-sized types?

@bors
Copy link
Contributor

bors commented Dec 31, 2024

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

@RalfJung
Copy link
Member

Is this requirement because references to ZSTs (including empty slices), like all other references, must be non-null

Yes.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Dec 31, 2024
Fix doc for read&write unaligned in zst operation

### PR Description
This PR addresses an inconsistency in the Rust documentation regarding `read_unaligned ` and `write_unaligned` on zero-sized types (ZSTs). The current documentation for [pointer validity](https://doc.rust-lang.org/nightly/std/ptr/index.html#safety) states that for zero-sized types (ZSTs), null pointers are valid:
> For zero-sized types (ZSTs), every pointer is valid, including the null pointer.

However, there is an inconsistency in the documentation for the unaligned read operation in the function [ptr::read_unaligned](https://doc.rust-lang.org/nightly/std/ptr/fn.read_unaligned.html)(as well as `write_unaligned`), which states:
> Note that even if T has size 0, the pointer must be non-null.

This change is also supported by [PR rust-lang#134912](rust-lang#134912)
> the _unaligned method docs should be fixed.
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Dec 31, 2024
Rollup merge of rust-lang#134953 - DiuDiu777:unaligned-doc, r=RalfJung

Fix doc for read&write unaligned in zst operation

### PR Description
This PR addresses an inconsistency in the Rust documentation regarding `read_unaligned ` and `write_unaligned` on zero-sized types (ZSTs). The current documentation for [pointer validity](https://doc.rust-lang.org/nightly/std/ptr/index.html#safety) states that for zero-sized types (ZSTs), null pointers are valid:
> For zero-sized types (ZSTs), every pointer is valid, including the null pointer.

However, there is an inconsistency in the documentation for the unaligned read operation in the function [ptr::read_unaligned](https://doc.rust-lang.org/nightly/std/ptr/fn.read_unaligned.html)(as well as `write_unaligned`), which states:
> Note that even if T has size 0, the pointer must be non-null.

This change is also supported by [PR rust-lang#134912](rust-lang#134912)
> the _unaligned method docs should be fixed.
@hxuhack
Copy link

hxuhack commented Jan 11, 2025

Thanks, @RalfJung. I noticed that the document mentions the requirement of non-null for pointer-to-reference conversion. I believe this might lead to confusion regarding whether null pointers for ZSTs are excluded.

Inspired by related issues, we started a project which aims to clarify all safety properties by defining them with primitive safety properties. We also plan to label unsafe APIs based on these "formally defined" properties. Our initial proposal for primitive safety properties can be found here: link. Do you have any comments or suggestions?

@RalfJung
Copy link
Member

I noticed that the document mentions the requirement of non-null for pointer-to-reference conversion. I believe this might lead to confusion regarding whether null pointers for ZSTs are excluded.

Pointers have to be non-null when converted to references. I don't see the potential for confusion here. You have to understand that different operations have different preconditions; that's why each operation documents its particular preconditions.

Inspired by related issues, we started a project which aims to clarify all safety properties by defining them with primitive safety properties. We also plan to label unsafe APIs based on these "formally defined" properties. Our initial proposal for primitive safety properties can be found here: link. Do you have any comments or suggestions?

Sorry, I won't have the time to review such an extensive document.

Regarding this PR, #134930 tweaked the wording a bit to clarify. I don't think any further changes are needed here. The PR as-is is definitely wrong, so I will close it. If you think the latest docs at https://doc.rust-lang.org/nightly/std/ptr/index.html are still unclear, please file an issue or a new PR explaining why and potentially suggesting concrete improvements.

@RalfJung RalfJung closed this Jan 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. 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