dangling pointer from temp cleanup #148490
Conversation
rustbot
added
S-waiting-on-review
| .note = pointers do not have a lifetime; when calling `{$callee}` the `{$ty}` will be deallocated at the end of the statement because nothing is referencing it as far as the type system is concerned | ||
| .help_bind = you must make sure that the variable you bind the `{$ty}` to lives at least as long as the pointer returned by the call to `{$callee}` | ||
| .help_returned = in particular, if this pointer is returned from the current function, binding the `{$ty}` inside the function will not suffice | ||
| lint_dangling_pointers_from_temporaries = expression creates a dangling pointer to dropped temporary `{$ty}` |
There was a problem hiding this comment.
I'm not sure that's the a good description of the issue, I feel like it's a bit misleading, the temporary is not yet dropped (otherwise this would insta UB), it's dropped only after creating the pointer, and it's the dropping of the temporary which makes the pointer dangling.
The previously wording was clearer IMO on this point. Not sure how to best improve that.
There was a problem hiding this comment.
Hmm, I thought that detail did not matter. Do you have a reference for your claim that that would be insta UB?
There was a problem hiding this comment.
Do you have a reference for your claim that that would be insta UB?
For String::new().DROPPED.as_ptr() it's apparently a grey area rust-lang/unsafe-code-guidelines#188 where use-after-dropped and use-after-free are different regarding opsem. I was told that it should probably be UB and is heavily discouraged.
But for allocation like "".to_string().DROPPED.as_ptr() Miri flags it as UB1 on the as_ptr() call, so 🤷
Footnotes
There was a problem hiding this comment.
I created rust-lang/miri#4668 to hopefully clarify this...
There was a problem hiding this comment.
Right, so this is UB because of a dangling reference (which miri calls a pointer confusingly). I think the existence of https://doc.rust-lang.org/nightly/std/ptr/fn.dangling.html proves that just a dangling pointer is not UB.
I also think that means that it does not really matter whether a now-dangling pointer was temporarily valid at its point of creation.
There was a problem hiding this comment.
My un-easeness is not on whenever the pointer was temporarily valid, it is, but as you said it's not relevant. What I'm having problem with that your proposed message makes it ambiguous as to whenever or not the pointer was created from an invalid temporary, which rust-lang/miri#4668 made it clear that it's UB (lang or libs).
It would be IMO better if the message made it clear / obvious that the pointer is dangling precisely because it points to a temporary that's dropped too early, which the previous message was clearer about.
Maybe we could say something like this:
dropped temporary makes the pointer to
{$ty}dangling
There was a problem hiding this comment.
Or maybe something like this:
warning: this pointer becomes dangling as the temporary {$ty} is dropped at statement end
After reflection I think I like this one the best, with the caveat that we should change the primary span to the as_ptr() part
| .help_bind = you must make sure that the variable you bind the `{$ty}` to lives at least as long as the pointer returned by the call to `{$callee}` | ||
| .help_returned = in particular, if this pointer is returned from the current function, binding the `{$ty}` inside the function will not suffice | ||
| lint_dangling_pointers_from_temporaries = expression creates a dangling pointer to dropped temporary `{$ty}` | ||
| .label_ptr = dangling pointer created here |
There was a problem hiding this comment.
This also is misleading IMO, it's not yet dangling but it will be.
There was a problem hiding this comment.
| .label_ptr = dangling pointer created here | |
| .label_ptr = pointer created here |
Urgau
added
S-waiting-on-author
|
BTW help vs note: help should be used to show changes the user can possibly make to fix the problem. note should be used for everything else, such as other context, information and facts, online resources to read, etc. -- https://rustc-dev-guide.rust-lang.org/diagnostics.html#diagnostic-levels Should probably change a lot of these helps to notes... |
rust-cloud-vms
bot
force-pushed
the
dangling-ptr-lint-2
branch
from
0ef89d7 to
09188da
Compare
|
@rustbot ready |
rustbot
added
S-waiting-on-review
4 participants
Continuation of #148348
r? @Urgau