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

elaborate on slice wide pointer metadata #1499

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

RalfJung
Copy link
Member

* `dyn Trait` metadata is invalid if it is not a pointer to a vtable for
`Trait` that matches the actual dynamic trait the pointer or reference points to.
`Trait` that matches the actual dynamic type the pointer or reference points to.
Copy link
Member

Choose a reason for hiding this comment

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

Preexisting but how can this be a requirement for wide pointers, whose data portion is allowed to dangle, I assume?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm... yeah the actually implemented requirement is that the trait matches the trait given in the pointer type, i.e. a *const dyn Debug that points to a vtable for Display is UB.

@traviscross
Copy link
Contributor

@rust-lang/opsem / @RalfJung: In looking carefully at this on the rustdocs call, we realized it might be helpful, both to us and when the lang teams takes this up, if someone might be able annotate the changes here (e.g. using the GH review features), with some explanation for each of these (e.g. "this was incorrect because...", "this was already true because we had said...", "we're adding this guarantee here because...").

@RalfJung RalfJung force-pushed the wide-ptr-meta branch 2 times, most recently from 46bcc6c to 091b2f2 Compare June 19, 2024 06:36
* Invalid metadata in a wide reference, `Box<T>`, or raw pointer:
* A reference or `Box<T>` that is [dangling], misaligned, or points to an invalid value
(using the actual dynamic type of the pointee as determined by the vtable in
the metadata in case of dynamically sized types).
Copy link
Member Author

Choose a reason for hiding this comment

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

This does not change anything, it clarifies what the pointed-to value is in case of e.g. &dyn Trait.

(using the actual dynamic type of the pointee as determined by the vtable in
the metadata in case of dynamically sized types).
* Invalid metadata in a wide reference, `Box<T>`, or raw pointer. The requirement
for the metadata is determined by the type of the unsized tail:
Copy link
Member Author

Choose a reason for hiding this comment

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

Another piece of clarification, previously we were not very clear on what the metadata requirements are for types like (u32, [u32]): we spoke about "slice metadata" and the fact that this covered all types whose unsized tail is a slice was left implicit. Now it is explicit.

* A reference or `Box<T>` that is [dangling], misaligned, or points to an invalid value.
* Invalid metadata in a wide reference, `Box<T>`, or raw pointer:
* `dyn Trait` metadata is invalid if it is not a pointer to a vtable for
`Trait` that matches the actual dynamic trait the pointer or reference points to.
Copy link
Member Author

@RalfJung RalfJung Jun 19, 2024

Choose a reason for hiding this comment

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

"that matches the actual dynamic trait" was clearly nonsense. Also this is now moved to the "points to an invalid value" point since it's not about the metadata, it's about using the metadata to keep going recursively through the reference.

(i.e., it must not be read from uninitialized memory).
Furthermore, for wide references and `Box<T>`, slice metadata is invalid
if it makes the total size of the pointed-to value bigger than `isize::MAX`.
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the only actual change, decided in rust-lang/unsafe-code-guidelines#510.

@RalfJung
Copy link
Member Author

RalfJung commented Jun 19, 2024

Note that the PR has two commits; it may help to consider them separately.

Copy link
Contributor

@nikomatsakis nikomatsakis left a comment

Choose a reason for hiding this comment

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

+1

@scottmcm
Copy link
Member

scottmcm commented Jul 3, 2024

Starting a lang RFC so it's officially approved, and we can use that to update docs elsewhere following this.

@rfcbot fcp merge

@rfcbot
Copy link

rfcbot commented Jul 3, 2024

Team member @scottmcm has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns.
See this document for info about what commands tagged team members can give me.

@traviscross
Copy link
Contributor

@rfcbot reviewed

@rfcbot
Copy link

rfcbot commented Jul 3, 2024

🔔 This is now entering its final comment period, as per the review above. 🔔

psst @scottmcm, I wasn't able to add the final-comment-period label, please do so.

@traviscross
Copy link
Contributor

@rustbot labels -I-lang-nominated +final-comment-period

This is now in FCP so we can unnominate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants