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

Safety comment in std::ptr::NonNull::dangling code was invalidated by a refactoring #132004

Open
tifv opened this issue Oct 21, 2024 · 2 comments
Labels
A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools Libs-Small Libs issues that are considered "small" or self-contained T-libs Relevant to the library team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@tifv
Copy link
Contributor

tifv commented Oct 21, 2024

The implementation of the std::ptr::NonNull::dangling function looked like this:

    pub const fn dangling() -> Self {
        // SAFETY: mem::align_of() returns a non-zero usize which is then casted
        // to a *mut T. Therefore, `ptr` is not null and the conditions for
        // calling new_unchecked() are respected.
        unsafe {
            let ptr = crate::ptr::invalid_mut::<T>(mem::align_of::<T>());
            NonNull::new_unchecked(ptr)
        }
    }

until a recent change (b58f647) made it into this:

    pub const fn dangling() -> Self {
        // SAFETY: mem::align_of() returns a non-zero usize which is then casted
        // to a *mut T. Therefore, `ptr` is not null and the conditions for
        // calling new_unchecked() are respected.
        unsafe {
            let ptr = crate::ptr::dangling_mut::<T>();
            NonNull::new_unchecked(ptr)
        }
    }

The code has changed, but the comment has not, and is now unrelated to the code.

Furthermore, it is unclear how to rewrite this comment correctly. The documentation of std::ptr::dangling_mut function only guarantees that it “Creates a new pointer that is dangling, but well-aligned”. However, the documentation of std::ptr module defines a dangling pointer with “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”. Since a dangling pointer can technically be null, the fact that std::ptr::dangling_mut returns a non-null pointer is an undocumented behaviour. This means that the safety of std::ptr::NonNull::dangling hinges on an undocumented behavior of another function.

@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Oct 21, 2024
@hanna-kruppe
Copy link
Contributor

This means that the safety of std::ptr::NonNull::dangling hinges on an undocumented behavior of another function.

The standard library can rely on its own undocumented behavior (unlike user code). It's also just a documentation oversight that ptr::dangling doesn't state that its result is non-null -- that is the entire reason why it exists. It would be good to fix the documentation and that should be uncontroversial. Updating the comment in NonNull::dangling doesn't have to be blocked on that, though.

@Noratrieb Noratrieb added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools T-libs Relevant to the library team, which will review and decide on the PR/issue. Libs-Small Libs issues that are considered "small" or self-contained and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Oct 21, 2024
@Noratrieb
Copy link
Member

Feel free to open a PR changing the SAFETY comment and docs.

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 Libs-Small Libs issues that are considered "small" or self-contained T-libs Relevant to the library team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

4 participants