clarify interaction of pin drop guarantee and panics

[RalfJung]

Cc rust-lang/unsafe-code-guidelines#232
@Diggsey would this have helped?

[rust-highfive]

r? @Mark-Simulacrum

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

[RalfJung]

Cc @rust-lang/lang for the pin drop guarantee

[Diggsey]

Yep, LGTM

[RalfJung]

Maybe this should go through T-lang FCP, not sure.

[RalfJung]

@rust-lang/lang the question for nomination is: do you all agree with what the docs say in this PR -- that it is okay to deallocate the memory backing pinned data after calling its drop even if that drop panicked?

At least for stack pinning, this is pretty much the only possible option: when something is pinned in a stack slot, unwinding will continue if the destructor panics, and thus the memory will get deallocated. Without the semantics I am proposing, Stack pinning would have to somehow abort-on-panic-in-drop to be sound.

However, I do not know how well existing unsafe code that interacts with the pin drop guarantee handles panics occurring in its drop impl. In particular this means that pinned "containers" (generic types that have pinned fields, including Future combinators) need to make sure that they do not skip dropping pinned fields when there is a panic. The rustc-generated drop glue takes care of this, but when using ManuallyDrop or raw pointers, the code has to set up appropriate drop guards.

[Diggsey]

@RalfJung the only sane alternative I can think of would be to disallow panicking from destructors of pinned data entirely (ie. make it UB or an immediate abort).

[RalfJung]

ie. make it UB or an immediate abort

I don't think UB is a reasonable option at all, but we could consider abort, yes.

That is still an option even if we adopt this now -- my proposal is future-compatible with aborting.

[joshtriplett]

ping @cramertj

[joshtriplett]

Based on some discussion in the lang team triage meeting:

• Freeing pinned stack memory, without running the whole drop (because some drop panicked), could result in some un-canceled operation scribbling over subsequently reused stack memory.
• We discussed how we might be able to transition to "always abort if any drop panics", rather than special-casing pinned memory.

[RalfJung]

Freeing pinned stack memory, without running the whole drop (because some drop panicked), could result in some un-canceled operation scribbling over subsequently reused stack memory.

I do not know with which intent you state this fact here.

This just means that when one field's drop panics, the next field's drop needs to be called (like the native drop glue does for structs and arrays etc, and like properly exception-safe destructors should everywhere).

So, this is not in contradiction with saying "if you call drop and it panics, you can free the memory". If you want to say that there is a problem here or that this means we cannot adopt the PR, then please explain, as I do not think that is the case.

[nikomatsakis]

Argh, I had an in-progress comment here in ... some tab. Probably I'll find it later. I wanted to ask @RalfJung a clarifying question.

It seems to me that, as you say, we already have the expectation that -- if a drop panics -- the memory might be freed anyway, so the Drop is responsible for aborting in that case. I'm not very happy about this, and I wonder if we could get away with changing it, but it's certainly true now.

Given that, I'm not really sure what alternative there is to what is documented here -- it would have to be, I believe, that somehow the drop machinery will abort, right? But it's not very clear to me if there is any way for us to make the drop machinery catch the panic and abort that specifically targets pinned memory and not other memory.

So my question is: Are there any proposals for such a mechanism? Zlso, I guess, does the rest of the logic in this comment sound correct to you.

[RalfJung]

Yeah, there's not really any alternative I can think of. But I also felt like I shouldn't land a docs change like this without consulting T-lang, to at least make sure y'all are aware of the situation here. Also maybe one of you would have come up with an alternative.

Maybe I was overcautious.

[nikomatsakis]

Well, the main alternative I have in my mind is..maybe we can just make all drops abort on panic. I think that was definitely a mistake not to do so -- or at least, change the default (but allow an opt-out), or something like that. I imagine that some people are relying on the current behavior, though. I feel like we've had this conversation enough times I should have the "patterns" at the tip of my tongue, but I don't presently.

[nikomatsakis]

But that seems orthogonal to this documentation PR =)

[Diggsey]

@nikomatsakis I suppose you could add the abort automatically to the compiler-generated drop glue for types which don't implement Unpin. Since almost all types implement Unpin it would greatly reduce the amount of breakage.

[RalfJung]

But that seems orthogonal to this documentation PR =)

Agreed. :D
So, if the consensus was basically that this is the only possible options... does that mean green light for the PR?

[joshtriplett]

Freeing pinned stack memory, without running the whole drop (because some drop panicked), could result in some un-canceled operation scribbling over subsequently reused stack memory.

I do not know with which intent you state this fact here.

This just means that when one field's drop panics, the next field's drop needs to be called (like the native drop glue does for structs and arrays etc, and like properly exception-safe destructors should everywhere).

So, this is not in contradiction with saying "if you call drop and it panics, you can free the memory". If you want to say that there is a problem here or that this means we cannot adopt the PR, then please explain, as I do not think that is the case.

Here's the scenario I was concerned about. If this is possible, I think it's an argument that we need to fix this somehow rather than just documenting it:

Suppose I have a pinned buffer on the stack, and some other component has a pointer to that buffer which it will use as the target of a write, and the drop for that pinned buffer (call it A) arranges with that other component to cancel the write before the memory gets freed. Now, suppose that a different drop (call it B) panics. In the course of unwinding, we free the stack memory without calling drop A, and we start to unwind the stack. We then call another drop (call it C), which uses (directly or indirectly) enough stack to overlap where that buffer used to be. Then the other component writes through the pointer, having not been cancelled. The stack is now scribbled-on and corrupted. Now drop C tries to return, using the corrupted stack, which best-case crashes and worst-case runs arbitrary code. (If the buffer being written into was arbitrary network data, this would be a remotely exploitable security hole.)

With that scenario in mind: are you saying that even if one drop panics, we'll still call every other drop that doesn't directly depend on that one, so the only way we can free memory without calling its corresponding drop is if that drop call panics? That would be a little more reasonable; it still means the drop has to do a catch_unwind and abort, but that's doable.

Is there any circumstance under which the above scenario could happen?

(I still think we should try to find a way to transition to "panic in drop is automatically abort", but that might be a difficult transition. I wonder if any code actually relies on this behavior, though.)

[RalfJung]

@joshtriplett

Now, suppose that a different drop (call it B) panics. In the course of unwinding, we free the stack memory without calling drop A, and we start to unwind the stack.

Why would it skip the destructor of A? rustc-generated drop glue always calls the remaining destructors when one of them panics (except for #47949 which is a bug).

With that scenario in mind: are you saying that even if one drop panics, we'll still call every other drop that doesn't directly depend on that one

Yes. As far as I know, that's how recursive dropping should work.

so the only way we can free memory without calling its corresponding drop is if that drop call panics? That would be a little more reasonable; it still means the drop has to do a catch_unwind and abort, but that's doable.

Indeed the drop of a pinned element (that relies on this guarantee) would have to ensure it doesn't panic -- in your example that would be the destructor of A. It can do that either by not doing anything that could panic, or indeed by turning panics into aborts.

Is there any circumstance under which the above scenario could happen?

Not without a soundness bug elsewhere, I don't think. For example, it used to be the case that VecDeque would not call drop on all its elements when another element destructor panics. Of course it would later still deallocate the backing buffer of all elements. (That might or might not have been fixed in the recent round of leak-on-panic fixes in libstd.) That is fine as long as it is impossible to get a pinned pointer to an element of VecDeque -- which is indeed impossible unless VecDeque opts-in to that by providing a "pin projection" like fn(Pin<&mut VecDeque<T>>, usize) -> Pin<&mut T>. If VecDeque had such a projection, then its old, broken destructor would violate the already existing clause "you must not deallocate the memory of pinned data without calling its destructor first".

Does this answer your question?

[nikomatsakis]

So @RalfJung I believe the answer is that we are clear to land this PR, yes.

[withoutboats]

I think the doc comment clarification is fine and I think there's no particular interaction between pin and drop panics, except maybe pinning highlights that a destructor which panics can be quite error-prone. Any change to the behavior of panic in drop should be made across the board and have nothing to do with whether values are pinned or not.

(My understanding is that this is the consensus viewpoint.)

[joshtriplett]

@RalfJung Thank you for the detailed explanation; that fully addresses my concerns.

[nikomatsakis]

@bors r+ rollup

[bors]

📌 Commit 33541d5 has been approved by nikomatsakis