Clarify drop_in_place safety #108684

Manishearth · 2023-03-03T00:49:12Z

See discussion on zulip

This basically puts ptr::drop_in_place() on the same footing as ManuallyDrop::drop() when it comes to docs.

rustbot · 2023-03-03T00:49:19Z

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

rustbot · 2023-03-03T00:49:22Z

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

Stabilizing library features
Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
Changing public documentation in ways that create new stability guarantees
Changing observable runtime behavior of library APIs

JakobDegen · 2023-03-03T01:30:13Z

library/core/src/ptr/mod.rs

+///
+/// Having an `&` or `&mut` reference to the pointed-to value after calling [`drop_in_place()`]
+/// is still sound as long as it is not read from (in which case the soundness of the operation
+/// depends on the specific type).


I'm not so sure that we should say this. drop_in_place does not on its own deinitialize, but Drop::drop code might, and it's undecided if holding a & to uninit data is sound.

Drop::drop() also has an &mut reference, if Drop::drop() is allowed to deinit it then you are allowed to hold on to an &mut reference after drop_in_place. I do not think there is any situation where Drop::drop() may soundly invalidate the type in a world where holding &-to-uninit is unsound.

I think there definitely is. The issue is that "holding" is a really imprecise term. Specifically, consider:

struct S(u8); impl Drop for S { fn drop(&mut self) { let p = self as *mut S as *mut MaybeUninit<u8>; *p = MaybeUninit::uninit(); // In the world I'm talking about, this would be UB, if it was actually written: // let r = self; } } let mut s = S; let r = &mut s; drop_in_place(r); // But the move about was not actually included, and so no UB. However, we include it here: let r_immut = &*r; // or let r2 = r, either is possible // Now we have UB

This world isn't particularly likely, but it's definitely not unimaginable

It would be nice to just get a ruling on this because as it stands this leads to the API being super weird and annoying.

Note that ManuallyDrop already has this guarantee; I'm just importing it.

Actually! The guarantee on ManuallyDrop prevents the above drop impl from being considered safe, because such a drop impl would make ManuallyDrop::drop() leave the type in an uninit state.

Note that ManuallyDrop already has this guarantee;

The statement about & or &mut seems stronger than anything ManuallyDrop says, no?

From the docs of ManuallyDrop::drop():

This is exactly equivalent to calling ptr::drop_in_place with a pointer to the contained value

..

Other than changes made by the destructor itself, the memory is left unchanged, and so as far as the compiler is concerned still holds a bit-pattern which is valid for the type T.

Perhaps there's ambiguity on how the "other than changes made by the destructor itself" groups with the rest of the statement, but to me this is saying that one can rely on ManuallyDrop::drop() leaving T in a situation of being valid, so drop_in_place must as well.

Ah, ok, so it's an open question whether &T validity depends on valid T -- but either way, if we're supposing that drop does leave a valid T, then &T remains valid too.

Yeah; it's an open question but I think it's not actually needed to be answered to reach my conclusion.

I've opened rust-lang/unsafe-code-guidelines#394 and will change the docs to use the same squirrely wording that ManuallyDrop uses

Manishearth · 2023-03-04T19:12:30Z

@JakobDegen I've removed the bit about references. I'm still not sure if it's fine to say "As far as the compiler is concerned, the value will still contain a valid bit pattern for type T" since it's not quite clear as to what "as far as the compiler is concerned" means here.

JakobDegen · 2023-03-04T22:11:43Z

library/core/src/ptr/mod.rs

 /// foo` counts as a use because it will cause the value to be dropped
 /// again. [`write()`] can be used to overwrite data without causing it to be
-/// dropped.
+/// dropped. Read operations may be UB based on library invariants of that type,
+/// for example reading the value pointed to by a dropped `Box<T>` is a use-after-free.


This sentence seems useful, but can you clarify: Read operations on what? Ie, I have code like:

let p: *mut T = some_pointer_from_somewhere; drop_in_place(p);

I initially interpreted read operations as being about *p, but that doesn't seem to agree with the rest of the sentence, which seems to be about **p in the case where T is Box<S>.

JakobDegen · 2023-03-04T22:22:33Z

So about the

as far as the compiler is concerned still holds a bit-pattern which is valid for the type T

sentence: As far as I can tell, it doesn't make any new promises, so I wouldn't be super strongly opposed to including it. The reason I don't believe it makes any new promises is that 1) the sentence is already in the ManuallyDrop::drop documentation, and 2) I don't know what it's even supposed to mean.

That being said, I think we're better off not having it in at all. Quoting Manish from the UCG issue:

I wonder if we can make the sentence say something useful for the specific case (where you know the type's Drop impl) as opposed to the generic one.

That seems like a good idea. I'd suggest putting this sentence after the section on "Additionally, if T is not [Copy], using the pointed-to value after..." We can then make the whole paragraph say something like this (might need to work on the wording):

However, [drop_in_place()] does not modify the pointed-to value beyond any changes performed by [Drop::drop()]. This means that in cases where you know which type is being dropped and you know the behavior of the drop glue, calling drop_in_place multiple times on the same object may be fine.

It may be worth including an example

Mark-Simulacrum · 2023-03-04T22:53:28Z

library/core/src/ptr/mod.rs

@@ -430,6 +430,10 @@ mod mut_ptr;
 /// done automatically by the compiler. This means the fields of packed structs
 /// are not dropped in-place.
 ///
+/// [`drop_in_place()`] does not modify the pointed-to value beyond any changes
+/// performed by [`Drop::drop()`]. As far as the compiler is concerned, the value
+/// will still contain a valid bit pattern for type `T`.


Suggested change

/// will still contain a valid bit pattern for type `T`.

/// must still contain a valid bit pattern for type `T`.

I feel like "will" is a little odd here, this reads better to me -- but in general I find the sentence strange; typically we don't I think say things like "As far as the compiler ...", rather that something is UB or not.

You'd say that except that party of the wording was lifted straight from the MaybeUninit docs :)

As Jakob says it doesn't make much sense though, so we should probably tighten up the wording on both

You mean the ManuallyDrop docs, I assume. But yeah that wording doesn't make a ton of sense. I don't know what that sentence is even trying to achieve, can we just remove it?

Mark-Simulacrum · 2023-03-04T22:53:44Z

r? libs-api

dtolnay · 2023-03-14T23:12:39Z

Reassigning BurntSushi's reviews because based on git log -i --grep=r=burntsushi the last time they did rust-lang/rust reviews was over 3 years ago.

r? rust-lang/libs-api

dtolnay · 2023-03-14T23:46:01Z

On second thought, I'm not sure that libs-api would want anything to do with this. I'd be comfortable delegating this to @JakobDegen who's been engaging on the UCG issue.

r? JakobDegen

JakobDegen · 2023-03-15T00:22:18Z

This will need an opsem FCP, but also there are outstanding review comments

Manishearth · 2023-03-15T00:26:47Z

Yeah at the moment I think we still need to decide on what we want to do here

JakobDegen · 2023-03-15T02:47:00Z

I think this would be valuable to merge with the review comments addressed. I don't expect that we're gonna get a resolution to rust-lang/unsafe-code-guidelines#394 anytime soon, but specifically this part seems useful on its own:

drop_in_place() does not modify the pointed-to value beyond any changes performed by Drop::drop()

JohnCSimon · 2023-12-17T21:22:19Z

@Manishearth
ping from triage - can you post your status on this PR? There hasn't been an update in a few months. Thanks!

Manishearth · 2023-12-17T21:25:55Z

I don't think this is blocked on me

Amanieu · 2024-04-02T16:39:18Z

Removing libs-api, this is waiting on t-opsem.

RalfJung · 2024-05-29T19:45:13Z

Removing libs-api, this is waiting on t-opsem.

Ah, then maybe someone should ping them team. :) I just saw this by coincidence.
Cc @rust-lang/opsem

RalfJung · 2024-05-29T19:51:15Z

library/core/src/ptr/mod.rs

@@ -430,6 +430,10 @@ mod mut_ptr;
 /// done automatically by the compiler. This means the fields of packed structs
 /// are not dropped in-place.
 ///
+/// [`drop_in_place()`] does not modify the pointed-to value beyond any changes
+/// performed by [`Drop::drop()`]. As far as the compiler is concerned, the value


What is the point of this guarantee? drop can still mutate the value pretty much arbitrarily, right?
Or is it intended to say something about the auto-generated drop glue, in case I control the drop being called? That seems odd to state like this though, instead we should give a somewhat operational description of the auto-generated drop glue:

for a struct, it will first call the user-defined drop if it exists, and then call the drop glue of each field

for an enum, it will first call the user-defined drop if it exists, and then determine the active variant and call the drop glue for all fields of that variant

for an array, it will call the drop glue for each field

...

Also IMO we shouldn't duplicate the same thing between ManuallyDrop::drop and here. drop_in_place is probably the more canonical location, but then ManuallyDrop::drop should just reference this.

Or is it intended to say something about the auto-generated drop glue, in case I control the drop being called?

Yes.

Clarify drop_in_place safety

23d4d77

Manishearth requested a review from cuviper March 3, 2023 00:49

rustbot assigned Mark-Simulacrum Mar 3, 2023

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 Mar 3, 2023

JakobDegen reviewed Mar 3, 2023

View reviewed changes

Manishearth mentioned this pull request Mar 4, 2023

Can Drop::drop invalidate the value? (i.e. can drop_in_place rely on the value staying valid-but-unusable) rust-lang/unsafe-code-guidelines#394

Open

Remove note on references

f6747b0

Manishearth requested a review from JakobDegen March 4, 2023 19:11

JakobDegen reviewed Mar 4, 2023

View reviewed changes

Mark-Simulacrum reviewed Mar 4, 2023

View reviewed changes

rustbot assigned BurntSushi and unassigned Mark-Simulacrum Mar 4, 2023

Mark-Simulacrum added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. and removed T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Mar 4, 2023

rustbot assigned m-ou-se and unassigned BurntSushi Mar 14, 2023

rustbot assigned JakobDegen and unassigned m-ou-se Mar 14, 2023

JakobDegen added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-opsem Relevant to the opsem team and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 15, 2023

Dylan-DPC added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 21, 2024

Amanieu removed the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Apr 2, 2024

RalfJung reviewed May 29, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clarify drop_in_place safety #108684

Clarify drop_in_place safety #108684

Manishearth commented Mar 3, 2023

rustbot commented Mar 3, 2023

rustbot commented Mar 3, 2023

JakobDegen Mar 3, 2023

Manishearth Mar 3, 2023

JakobDegen Mar 3, 2023

Manishearth Mar 3, 2023

Manishearth Mar 3, 2023

cuviper Mar 3, 2023

Manishearth Mar 3, 2023

cuviper Mar 3, 2023

Manishearth Mar 3, 2023

Manishearth Mar 4, 2023

Manishearth commented Mar 4, 2023

JakobDegen Mar 4, 2023

JakobDegen commented Mar 4, 2023

Mark-Simulacrum Mar 4, 2023

Manishearth Mar 4, 2023

RalfJung May 29, 2024

Mark-Simulacrum commented Mar 4, 2023

dtolnay commented Mar 14, 2023

dtolnay commented Mar 14, 2023

JakobDegen commented Mar 15, 2023

Manishearth commented Mar 15, 2023

JakobDegen commented Mar 15, 2023 •

edited

Loading

JohnCSimon commented Dec 17, 2023

Manishearth commented Dec 17, 2023

Amanieu commented Apr 2, 2024

RalfJung commented May 29, 2024

RalfJung May 29, 2024

Manishearth May 29, 2024

	/// will still contain a valid bit pattern for type `T`.
	/// must still contain a valid bit pattern for type `T`.

Clarify drop_in_place safety #108684

Are you sure you want to change the base?

Clarify drop_in_place safety #108684

Conversation

Manishearth commented Mar 3, 2023

rustbot commented Mar 3, 2023

rustbot commented Mar 3, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Manishearth commented Mar 4, 2023

Choose a reason for hiding this comment

JakobDegen commented Mar 4, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mark-Simulacrum commented Mar 4, 2023

dtolnay commented Mar 14, 2023

dtolnay commented Mar 14, 2023

JakobDegen commented Mar 15, 2023

Manishearth commented Mar 15, 2023

JakobDegen commented Mar 15, 2023 • edited Loading

JohnCSimon commented Dec 17, 2023

Manishearth commented Dec 17, 2023

Amanieu commented Apr 2, 2024

RalfJung commented May 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JakobDegen commented Mar 15, 2023 •

edited

Loading