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

Documentation of wrapping_add/-_sub/-_offset need clarification... #80306

Closed
steffahn opened this issue Dec 22, 2020 · 1 comment · Fixed by #80383
Closed

Documentation of wrapping_add/-_sub/-_offset need clarification... #80306

steffahn opened this issue Dec 22, 2020 · 1 comment · Fixed by #80383
Labels
A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools C-enhancement Category: An issue proposing an enhancement or a PR with one. T-lang Relevant to the language 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

@steffahn
Copy link
Member

steffahn commented Dec 22, 2020

...about whether pointer arithmetic is allowed to - temporarily - leave the bounds of an object, then go back into bounds and afterwards dereference the pointer.

The question is if code like this:

fn foo(x: &u8) {
    let mut x = x as *const u8;
    x = x.wrapping_add(42);
    x = x.wrapping_sub(42);
    let _val = unsafe { *x };
    // use _val
}

is sound or unsound.

The current documentation says:

The resulting pointer does not need to be in bounds, but it is potentially hazardous to dereference (which requires unsafe).

In particular, the resulting pointer remains attached to the same allocated object that self points to. It may not be used to access a different allocated object. Note that in Rust, every (stack-allocated) variable is considered a separate allocated object.

Compared to add / sub / offset, this method basically delays the requirement of staying within the same allocated object: add / sub / offset is immediate Undefined Behavior when crossing object boundaries; wrapping_add / wrapping_sub / wrapping_offset produces a pointer but still leads to Undefined Behavior if that pointer is dereferenced. add / sub / offset can be optimized better and is thus preferable in performance-sensitive code.

So it specifies that the resulting pointer “remains attached to the same allocated object that self points to”, which suggests that after going back into bounds, dereferencing should be safe again.

It also says “this method basically delays the requirement of staying within the same allocated object; ” which sounds like, staying within the same allocated object is still a requirement, which could be interpreted as that whenever any intermediate value did not stay within bounds, that intermediate value has some delayed violation of requirements attached to it that triggers UB once the final (back in bounds) pointer is dereferenced.

The following comparison, “add / sub / offset is immediate Undefined Behavior when crossing object boundaries; wrapping_add / wrapping_sub / wrapping_offset produces a pointer but still leads to Undefined Behavior if that pointer is dereferenced” can easily be interpreted as confirming this interpretation, since object boundaries are crossed (twice, first going out of the object, then going back in) and the pointer is dereference afterwards (right after the second crossing of object boundaries).

After I was addressing this on IRLO, @RalfJung gave the answer

It is explicitly intended to be allowed to leave the object and then go back inbounds. Looks like we need to clarify the docs.

@rustbot modify labels: T-doc, T-libs, T-lang, C-enhancement
Feel free to remove a T- label if this doesn’t seem relevant for that team.

@rustbot rustbot added C-enhancement Category: An issue proposing an enhancement or a PR with one. A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools T-lang Relevant to the language 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. labels Dec 22, 2020
@RalfJung
Copy link
Member

It also says “this method basically delays the requirement of staying within the same allocated object; ” which sounds like, staying within the same allocated object is still a requirement, which could be interpreted as that whenever any intermediate value did not stay within bounds, that intermediate value has some delayed violation of requirements attached to it that triggers UB once the final (back in bounds) pointer is dereferenced.

Ah I see... yeah that is a misunderstanding. The delayed check means that when the (derived) ptr is dereferenced, that ptr needs to be in-bounds, but the intermediate ptrs do not have to be.

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 C-enhancement Category: An issue proposing an enhancement or a PR with one. T-lang Relevant to the language 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

Successfully merging a pull request may close this issue.

3 participants