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

constrain safety preconditions of layout_for_ptr functionality #117185

Closed
wants to merge 1 commit into from

Conversation

jswrenn
Copy link
Member

@jswrenn jswrenn commented Oct 25, 2023

This commit implements the recommendation of #69835 (comment) to make the safety preconditions of the raw pointer layout utilities more conservative, as to ease the path towards stabilization. In the future, we may (if we choose) remove some of these restrictions without breaking forwards compatibility.

@rustbot
Copy link
Collaborator

rustbot commented Oct 25, 2023

r? @thomcc

(rustbot has picked a reviewer for you, use r? to override)

@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 Oct 25, 2023
@thomcc
Copy link
Member

thomcc commented Oct 25, 2023

This feels like it's not a question for me.

r? @rust-lang/opsem

@rustbot rustbot assigned digama0 and unassigned thomcc Oct 25, 2023
@CAD97
Copy link
Contributor

CAD97 commented Oct 26, 2023

The noted comment suggests to

add a safety precondition that the pointer not result in size wrapping [to size_of_val_raw and friends]

but this is already the case, as the docs say that the size of the entire value must fit in isize. What this PR adds, that

the pointer plus the size must not overflow the address space

is a completely different requirement. (Plus, ptr.wrapping_byte_offset(size) is the one-past-the-end pointer; there's no benefit to mentioning it separately.)

See my reply in the tracking issue for more context.

This commit implements the recommendation of [1] to make the
safety preconditions of the raw pointer layout utilities more
conservative, to ease the path towards stabilization. In the
future, we may (if we choose) remove some of these restrictions
without breaking forwards compatibility.

[1]: rust-lang#69835 (comment)
@jswrenn
Copy link
Member Author

jswrenn commented Oct 26, 2023

@CAD97, ah, I misunderstood what you meant by 'wrap'. Yes, I agree, the size wrapping constraint is already covered. Still, it is possible to use ptr::slice_from_raw_parts to overflow the address space without overflowing the size, and addressing this possibility seems useful.

@RalfJung
Copy link
Member

See the discussion here -- I don't think we should do this. We should allow raw ptr size-of for non-dereferenceable pointers.

@jswrenn
Copy link
Member Author

jswrenn commented Nov 3, 2023

@RalfJung, as far as I can tell, the safety preconditions here do not generally require dereferenceability. The one exception is that the vtable part of a dyn pointer must point a valid vtable.

@CAD97
Copy link
Contributor

CAD97 commented Nov 3, 2023

Dereferencability specifically is more in respect to my proposed conservative wording in the tracking issue, but the sentiment holds the same for the change here: that there should be no restrictions placed by these *_raw methods on the address part of the pointer (for unsized tails of currently stable kinds).

I plan to make a PR sometime next week rewording/refining the current requirements and to propose T-lang FCP signoff on stabilizing that capability. I expect no lang concerns; I just haven't had the time/motivation to push for it yet.

As such, I'd suggest closing this PR as not planned. If T-lang requests a more conservative option in their FCP, we can reopen and/or otherwise pivot.

@jswrenn jswrenn closed this Nov 3, 2023
@jswrenn
Copy link
Member Author

jswrenn commented Nov 3, 2023

Happy to close, then. Let me know if there's anything @joshlf and I can do to help drive this forward. The stabilization of size_of_val_raw the major blocker for landing support of safer transmutation of DSTs into zerocopy.

@joshlf
Copy link
Contributor

joshlf commented Nov 3, 2023

Does that include that there should be no restrictions on the vtable pointer, or is the vtable pointer the one exception (ie, it will be required to point to a valid vtable entry, and perhaps one whose metadata is such that the pointer encodes a value whose size fits in isize)? If the vtable pointer can also be invalid, what is the behavior if called with an invalid vtable pointer?

@CAD97
Copy link
Contributor

CAD97 commented Nov 3, 2023

The current docs are the semantics I intend to FCP — the ptr.addr() part is irrelevant, but the ptr.meta() part is extremely important. IIRC, T-lang has FCPd vtable pointer validity as (at least) a safety invariant of fat pointers to dyn Trait tails, allowing trait upcasting raw pointers (feature(trait_upcast)). It's also a safety invariant of ptr::DynMetadata) (feature(ptr_metadata)). It will also be a safety precondition of layout_of_value_raw, though whether that's better spelled out or left implicit is a docs call I'm not quite worrying myself with yet (I'm focusing on just getting the semantics approved first). (Making it an explicit safety precondition might accidentally imply that it's not an implicit safety precondition to all other functions that don't explicitly remove the precondition.)

@RalfJung
Copy link
Member

RalfJung commented Nov 4, 2023

Does that include that there should be no restrictions on the vtable pointer, or is the vtable pointer the one exception (ie, it will be required to point to a valid vtable entry, and perhaps one whose metadata is such that the pointer encodes a value whose size fits in isize)?

Currently, the vtable pointer being valid is part of the validity invariant of dyn Trait-tailed raw pointers. So it is UB to even construct a raw ptr with a bad vtable. Therefore, the raw align/size-of methods can rely on the vtable pointer being valid.

This might be relaxed in the future to only be a safety invariant, but can't be relaxed further as @CAD97 noted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants