#246 [Coding Guideline]: Do not access memory using a pointer with an incorrect provenance#264
#246 [Coding Guideline]: Do not access memory using a pointer with an incorrect provenance#264manhatsu wants to merge 8 commits intorustfoundation:mainfrom
Conversation
❌ Deploy Preview for scrc-coding-guidelines failed.
|
|
@manhatsu This is going to continue to be a rule about provenance so it should definitely have provenance in the title, otherwise I won't be able to find it. For now I suggest the title be "Do not access memory using a pointer with incorrect provenance" |
Updated guidelines on pointer comparisons and memory access to clarify the importance of provenance and the implications of comparing pointers from different allocations.
|
@rcseacord It was my lack of understanding. I changed the title as you suggested. |
added new noncompliant / compliant solution
| Do not access memory using a pointer with an incorrect provenance. | ||
| Pointers, including values of reference type, have two components. | ||
| The pointer’s address identifies the memory location where the pointer is currently pointing. | ||
| The pointer’s provenance determines where and when the pointer is allowed to access memory. |
There was a problem hiding this comment.
The provenance also determines if the pointer is allowed to mutate the memory. See also the std docs for this: https://doc.rust-lang.org/std/ptr/index.html#provenance
This should maybe also be taken into account below when discussing when a memory access is UB.
| - Outcomes of pointer arithmetic across allocation boundaries | ||
|
|
||
| This rule ignores any metadata that may come with wide pointers; | ||
| it only pertains to thin pointers and the data part of a wide pointer. |
There was a problem hiding this comment.
| it only pertains to thin pointers and the data part of a wide pointer. | |
| it only pertains to thin pointers and the address part of a wide pointer. |
The additional data of a wide pointer is often called metadata (See the unstable function for reading it: https://doc.rust-lang.org/std/ptr/fn.metadata.html or the previous sentence). So calling the address "data" is confusing.
There was a problem hiding this comment.
Do you want to link to unstable docs? If yes it may be better to link to https://doc.rust-lang.org/std/ptr/trait.Pointee.html#pointer-metadata as here the current kinds of metadata are explained?
There was a problem hiding this comment.
@inkreasing OK, i added this link. do you see any other problems with this or does it look ready to merge?
There was a problem hiding this comment.
@rcseacord Oh sorry i am not in any position to decide wether this is ready to be merged. This was basically a drive-by review. Will make that clear from the beginning next time.
There was a problem hiding this comment.
no problem, i was just looking for your opinion.
link to unstable docs as here the current kinds of metadata are explained.
|
👋 Hey @iglesias! You've been assigned to review this coding guideline PR. Your Role as ReviewerAs outlined in our contribution guide, please:
Review Checklist
Bot CommandsIf you need to pass this review:
To assign someone else:
Other commands:
|
|
@iglesias one sec, we messed up and the thing has assigned you as reviewer to 4 PRs (it should only be able to assign you to 1) |
|
@iglesias okay, this one is the right one (according to the bot's queue). Sorry about the multiple pings! :) |
|
Hey @iglesias would you like to claim this review, or let someone else grab it |
|
Hey @iglesias, it's been more than 14 days since you were assigned to review this. Please take one of the following actions:
If no action is taken within 14 days, you may be transitioned from Producer to Observer status per our contribution guidelines. Life happens! If you're dealing with something, just let us know. |
|
@guidelines-bot /claim |
|
Hey @felix91gr @plaindocs sorry the delay :-) |
Co-authored-by: Fernando José <fernando.iglesiasg@gmail.com>
|
Hey @iglesias, it's been more than 14 days since you were assigned to review this. Please take one of the following actions:
If no action is taken within 14 days, you may be transitioned from Producer to Observer status per our contribution guidelines. Life happens! If you're dealing with something, just let us know. |
|
@guidelines-bot |
|
@guidelines-bot /commands |
|
Hey @felix91gr @plaindocs |
|
GAH we have to solve those conflicts, that's the issue. Damn, I forgot. I tried to do it on the web ui a couple times, but github wouldn't have it (something something large diff). And later I forgot to do it locally (I'm not even sure I know how to do that in a way github would accept). @iglesias your review is perfect as far as I can tell. Lemme see if we can get this solved with @plaindocs before the bot pings you again 🙇🏻 |
|
@felix91gr know anything about the two errors shown? |
|
@plaindocs I'm checking... one sec |
|
Ah. Okay, I see what's up. Hmm. I'll be back in a bit. The code examples need some help. This didn't trigger before because there were some merge conflicts that stopped CI jobs from running. |
|
Hey @iglesias, it's been more than 14 days since you were assigned to review this. Please take one of the following actions:
If no action is taken within 14 days, you may be transitioned from Producer to Observer status per our contribution guidelines. Life happens! If you're dealing with something, just let us know. |
3 similar comments
|
Hey @iglesias, it's been more than 14 days since you were assigned to review this. Please take one of the following actions:
If no action is taken within 14 days, you may be transitioned from Producer to Observer status per our contribution guidelines. Life happens! If you're dealing with something, just let us know. |
|
Hey @iglesias, it's been more than 14 days since you were assigned to review this. Please take one of the following actions:
If no action is taken within 14 days, you may be transitioned from Producer to Observer status per our contribution guidelines. Life happens! If you're dealing with something, just let us know. |
|
Hey @iglesias, it's been more than 14 days since you were assigned to review this. Please take one of the following actions:
If no action is taken within 14 days, you may be transitioned from Producer to Observer status per our contribution guidelines. Life happens! If you're dealing with something, just let us know. |
|
@PLeVasseur could you check the naggy review bot please |
|
Hey @iglesias, it's been more than 14 days since you were assigned to review this. Please take one of the following actions:
If no action is taken within 14 days, you may be transitioned from Producer to Observer status per our contribution guidelines. Life happens! If you're dealing with something, just let us know. |
|
Sorry for the nagging; working on it |
|
Thank you 🙇 |
Closes #246.