Stream: t-lang/wg-unsafe-code-guidelines

Topic: Pin destructor


gnzlbg (Oct 23 2019 at 09:49, on Zulip):

Can Pin::drop panic ? and if so, which invariants should that maintain ?

gnzlbg (Oct 23 2019 at 09:51, on Zulip):

https://doc.rust-lang.org/std/pin/#drop-guarantee and the section below only require that memory is not deallocated until drop gets called

gnzlbg (Oct 23 2019 at 09:52, on Zulip):

but if the drop call panics, the memory of the pin will be deallocated, e.g., via unwinding

gnzlbg (Oct 23 2019 at 09:53, on Zulip):

So AFAICT Pin::drop needs to make sure that, in the presence of a panic!, no references to the memory outlive the Pin

RalfJ (Nov 03 2019 at 12:22, on Zulip):

@gnzlbg good question... @boats any thoughts?

RalfJ (Nov 03 2019 at 12:23, on Zulip):

this seems worth opening an issue about -- not sure on which tracker though. if UCG = R-AM, then this is out of scope as its library UB. So likely the main rust issue tracker?

gnzlbg (Nov 04 2019 at 09:45, on Zulip):

@RalfJ probably in rust-lang/rust

gnzlbg (Nov 04 2019 at 09:45, on Zulip):

although it's unclear whether the docs of Pin need to mention this

boats (Nov 04 2019 at 10:28, on Zulip):

I guess the issue is that the destructor might panic before completing whatever fixups are necessary to put the system in a valid state?

boats (Nov 04 2019 at 10:29, on Zulip):

It seems like a code correctness issue in creating intrusive data structures

boats (Nov 04 2019 at 10:29, on Zulip):

We could mention it in the intrusive data structures section if there's a succinct way to explain the pitfall.

boats (Nov 04 2019 at 10:32, on Zulip):

(More abstractly, we've only guaranteed that drop gets called, that commitment has been met even if the drop panics, no? libraries are responsible for translating that guarantee into a safe abstraction for their actual purpose)

gnzlbg (Nov 04 2019 at 10:45, on Zulip):

I don't think we guarantee that drop gets called for Pin (we can't really guarantee this)

gnzlbg (Nov 04 2019 at 10:46, on Zulip):

Pin requires that drop gets called for safety

boats (Nov 04 2019 at 10:46, on Zulip):

I think I actually see the point

gnzlbg (Nov 04 2019 at 10:46, on Zulip):

The thing is Pin probably also requires that the drop of the T does not panic

boats (Nov 04 2019 at 10:46, on Zulip):

So we tell users who receive a Pin<P> that the destructor of P::Target will be called before P::Target's memory becomes invalid

gnzlbg (Nov 04 2019 at 10:47, on Zulip):

yes

boats (Nov 04 2019 at 10:47, on Zulip):

However, its possible if P's drop panics, that the destructor of P::Target won't be called, but the memory is invalid

boats (Nov 04 2019 at 10:47, on Zulip):

is that true / what you're talking about? (hard to reason about the drop glue and panics)

gnzlbg (Nov 04 2019 at 10:48, on Zulip):

I don't have a small reproducer of this, but I suspect one can write one

boats (Nov 04 2019 at 10:48, on Zulip):

Yes its true

boats (Nov 04 2019 at 10:48, on Zulip):

Or, let's say its true

boats (Nov 04 2019 at 10:48, on Zulip):

A similar example is if P implements Deref but doesn't actually involve pointer indirection, like ManuallyDrop does

boats (Nov 04 2019 at 10:49, on Zulip):

However, this is also why there's no generic safe constructor for a Pin<P>

boats (Nov 04 2019 at 10:49, on Zulip):

There are a few things we can say about this (though they are basically all libs issues, since its about how to correctly use this unsafe std API, and not language semantics)

gnzlbg (Nov 04 2019 at 10:50, on Zulip):

Yes, this would be another requirement of Pin for safety

boats (Nov 04 2019 at 10:50, on Zulip):

Well, I think there's one obvious correct one: if you create a safe constructor for Pin<P>, you must ensure that P's behavior upholds pin's drop guarantees

boats (Nov 04 2019 at 10:50, on Zulip):

so the incorrectness would be in the code that created a Pin<P> where Ps destructor panics

gnzlbg (Nov 04 2019 at 10:51, on Zulip):

The writer of P needs to make sure that if P::drop panics, deallocating P is safe.

boats (Nov 04 2019 at 10:51, on Zulip):

well, the writer of the code that makes a Pin<P>, P could come from another library that never thought about pinning at all

boats (Nov 04 2019 at 10:52, on Zulip):

I think a new section about defining safe constructors could be useful info, though it reaches the point where maybe some of this should go into the nomicon instead of the main pin docs

gnzlbg (Nov 04 2019 at 10:52, on Zulip):

Hm, I thought that the writer of P already needed to consider the possiblity of P being pinned when providing a drop impl for P

boats (Nov 04 2019 at 10:52, on Zulip):

Definitely not, because there's no safe way to pin an unknown pointer type

gnzlbg (Nov 04 2019 at 10:54, on Zulip):

But, e.g.,Box would need to consider this right ?

boats (Nov 04 2019 at 10:54, on Zulip):

In std, we needed to consider it when we added the Box::pin constructor

gnzlbg (Nov 04 2019 at 10:54, on Zulip):

That is, if you want to provide safe pinning for your type, then you need to consider what happens when your type destructor panics, and make sure that everything is still safe.

boats (Nov 04 2019 at 10:55, on Zulip):

Yes its true, I'm just saying that whoever decided to "provide safe pinning" is responsible, not whoever wrote the type (if they're separate libraries, they also need to make sure the guarantees they're relying on will be maintained in perpetuity)

boats (Nov 04 2019 at 10:55, on Zulip):

It's a distinction without a difference in practice

gnzlbg (Nov 04 2019 at 10:55, on Zulip):

Ah, sure, the distinction makes sense, thanks.

boats (Nov 04 2019 at 10:56, on Zulip):

So that's why I think this and the ManuallyDrop example could go together under a section on defining safe constructors for pinned pointers

gnzlbg (Nov 04 2019 at 10:56, on Zulip):

I think it would be better to have a minimum working example showing the problem before trying to come up with some docs for this

gnzlbg (Nov 04 2019 at 10:57, on Zulip):

Yes, that makes sense. With ManuallyDrop one can create this same issue, without having to explain how panicking in destructors works.

boats (Nov 04 2019 at 10:59, on Zulip):

one thing worth noting also is that because this is a requirement of creating a constructor, generic code like Pin<P> where P::Target = MyType is correct

boats (Nov 04 2019 at 10:59, on Zulip):

you can assume that that type upholds the pinning guarantee, even though the pointer type is abstract

gnzlbg (Nov 04 2019 at 11:02, on Zulip):

so I was trying to think of a practical situation where this would be an issue, but panic in destructors is weird

gnzlbg (Nov 04 2019 at 11:02, on Zulip):

it means something failed while dropping a type, so how can you then make sure that your type is ok to deallocate ?

RalfJ (Nov 05 2019 at 09:12, on Zulip):

However, its possible if P's drop panics, that the destructor of P::Target won't be called, but the memory is invalid

well the question is if this is legal, or if we require callers to not free memory in this case. but the latter seems unrealistic.

RalfJ (Nov 05 2019 at 09:12, on Zulip):

so, basically the safety requirement for drop of something pinned is that once drop returns or unwinds, the backing store must be safe to deallocate

RalfJ (Nov 05 2019 at 09:13, on Zulip):

"you must call drop before invalidating memory" is really just a different way of saying that (a) deallocating memory requires full ownership of that memory, and (b) for pinned stuff, you don't have that ownership -- but you can get it by calling drop

RalfJ (Nov 05 2019 at 09:14, on Zulip):

@boats ^

boats (Nov 06 2019 at 18:14, on Zulip):

That seems like a good way to explain the situation, though I'd always want to be clear that nothing at the language level is happening here.

RalfJ (Nov 16 2019 at 09:52, on Zulip):

@boats nothing at the R-AM level is happening here. if you consider the "meaning" of primitive Rust types to be part of the language, then that is involved here. There's a reason that supporting Pin in the RustBelt formalization of the type system is a global change that affects all types.

RalfJ (Nov 16 2019 at 09:53, on Zulip):

as for panics, should we try to somehow put this into the docs? it's a really subtle point but also I don't know where else to put it.

Last update: Nov 19 2019 at 18:05UTC