Stream: general

Topic: #60822


simulacrum (Dec 23 2019 at 12:49, on Zulip):

@gnzlbg I think a sync chat might be helpful -- I'm trying to understand your viewpoint on what exactly we should document.

You say: "In the context of this discussion, the question that needs to be properly answered somewhere in the reference is: If a call to Drop::drop fails, was the value dropped? If the answer is "no", then calling Drop::drop again isn't a "double-drop" because the value has not been dropped."

To me that's not really something we need to clarify, since any function which takes ownership can only be called once, even if it panics, right?

simulacrum (Dec 23 2019 at 12:49, on Zulip):

Like, there's nothing specific to Drop here.

simulacrum (Dec 23 2019 at 12:50, on Zulip):

Would you expect us to clarify this for all functions? Essentially we'd be saying "panic does not undo transfers of ownership"

simulacrum (Dec 23 2019 at 12:50, on Zulip):

but that seems like an odd statement to make (at least to me) because that seems somewhat... obvious, I guess?

gnzlbg (Dec 23 2019 at 12:51, on Zulip):

Notice that drop_in_place drops without taking ownership

simulacrum (Dec 23 2019 at 12:51, on Zulip):

no, that's not true

simulacrum (Dec 23 2019 at 12:52, on Zulip):

drop_in_place explicitly notes: "using the pointed-to value after calling drop_in_place can cause undefined behavior"

any Drop is a transfer of ownership

gnzlbg (Dec 23 2019 at 12:52, on Zulip):

"Can"

gnzlbg (Dec 23 2019 at 12:52, on Zulip):

any Drop that succeeds, is definetely a transfer of ownership

gnzlbg (Dec 23 2019 at 12:53, on Zulip):

but we are talking about, e.g., a call to ptr::drop_in_place that fails

simulacrum (Dec 23 2019 at 12:53, on Zulip):

You would agree that std::mem::drop cannot be called more than once, even if it panics, right?

simulacrum (Dec 23 2019 at 12:53, on Zulip):

(i.e., there's nothing to clarify there)

gnzlbg (Dec 23 2019 at 12:53, on Zulip):

I'm not sure how you would be able to do that

simulacrum (Dec 23 2019 at 12:53, on Zulip):

well, I mean, obviously it's possible

gnzlbg (Dec 23 2019 at 12:53, on Zulip):

the only way to run into these issues, is using ptr::drop_in_place

gnzlbg (Dec 23 2019 at 12:54, on Zulip):

the borrow checker doesn't let you call mem::drop twice

gnzlbg (Dec 23 2019 at 12:54, on Zulip):

not even in unsafe code

simulacrum (Dec 23 2019 at 12:54, on Zulip):

That's just because it's easier to misuse

simulacrum (Dec 23 2019 at 12:54, on Zulip):

i.e., it only semantically takes ownership

gnzlbg (Dec 23 2019 at 12:54, on Zulip):

no, i mean that it is literally impossible to call mem::drop on a value twice in Rust

simulacrum (Dec 23 2019 at 12:54, on Zulip):

Its docs even say "This is semantically equivalent to calling ptr::read and discarding the result"

simulacrum (Dec 23 2019 at 12:54, on Zulip):

okay, sure

simulacrum (Dec 23 2019 at 12:54, on Zulip):

but you can obviously resynthesize the value and re-pass it in if you wanted to

simulacrum (Dec 23 2019 at 12:55, on Zulip):

e.g., by ptr::reading again

gnzlbg (Dec 23 2019 at 12:55, on Zulip):

sure, but a ptr::read + forget doesn't do anything

gnzlbg (Dec 23 2019 at 12:55, on Zulip):

as in, if I have some memory on the heap, and I call ptr::read and forget the result, the value on the heap "is still alive"

simulacrum (Dec 23 2019 at 12:55, on Zulip):

Okay, so I see that "discarding" as "std::mem::drop"

gnzlbg (Dec 23 2019 at 12:55, on Zulip):

yes

simulacrum (Dec 23 2019 at 12:56, on Zulip):

i.e., drop_in_place is conceptually std::mem::drop(ptr::read(ptr))

gnzlbg (Dec 23 2019 at 12:56, on Zulip):

for me the clarification that should happen somewhere, is that when you try to drop something, it doesn't matter whether that succeeds or fails, that drops the value

simulacrum (Dec 23 2019 at 12:56, on Zulip):

and with that definition it of course can't be called twice, for the same reason that std::mem::drop can't be

simulacrum (Dec 23 2019 at 12:56, on Zulip):

but how is that different from std::mem::drop(ptr::read(ptr)) panicking?

gnzlbg (Dec 23 2019 at 12:56, on Zulip):

well, you are assuming that the mem::drop succeeds

bjorn3 (Dec 23 2019 at 12:57, on Zulip):

i.e., drop_in_place is conceptually std::mem::drop(ptr::read(ptr))

Except that drop_in_place doesn't move the value. (Important for pinning)

gnzlbg (Dec 23 2019 at 12:57, on Zulip):

we don't say that mem::drop can't be called twice

gnzlbg (Dec 23 2019 at 12:57, on Zulip):

we say that a value cannot be dropped twice

gnzlbg (Dec 23 2019 at 12:57, on Zulip):

that double-drops are UB

gnzlbg (Dec 23 2019 at 12:57, on Zulip):

the question is whether a drop that fails still drops or not

gnzlbg (Dec 23 2019 at 12:57, on Zulip):

if a drop that fails does not drop, then mem::drop(ptr::read(ptr) did not drop anything

simulacrum (Dec 23 2019 at 12:58, on Zulip):

okay

gnzlbg (Dec 23 2019 at 12:59, on Zulip):

So maybe what we need to clarify is whether "drop" is the act of actually running Drop::drop to "success", or the act of trying to run Drop::drop

simulacrum (Dec 23 2019 at 12:59, on Zulip):

Right, to me it's "obviously" the latter

gnzlbg (Dec 23 2019 at 12:59, on Zulip):

yeah, i think that's the consensus

simulacrum (Dec 23 2019 at 12:59, on Zulip):

But I guess that's not true for you, so we should document it somewhere

simulacrum (Dec 23 2019 at 12:59, on Zulip):

Since the confusion seems to center around drop_in_place, maybe adding a section there makes sense?

gnzlbg (Dec 23 2019 at 13:00, on Zulip):

I was just wondering if we actually had written that somewhere, when I filled that issue, is because I actually wrote code that caught whether dropping a value failed, and if so, tried to re-drop again

gnzlbg (Dec 23 2019 at 13:00, on Zulip):

I think it would be better to add this to wherever we define what "drop" is, e.g., maybe to Drop::drop

gnzlbg (Dec 23 2019 at 13:00, on Zulip):

or the Drop trait docs

simulacrum (Dec 23 2019 at 13:01, on Zulip):

hm, okay, I could see that I guess

bjorn3 (Dec 23 2019 at 13:01, on Zulip):
struct VecWrapper {
    x: ManuallyDrop<Vec<u8>>,
}

impl Drop for VecWrapper {
    fn drop(&mut self) {
        unsafe { ManuallyDrop::drop(&mut self.x) }
    }
}

would be unsound if Vec<u8>::drop could panic.

simulacrum (Dec 23 2019 at 13:02, on Zulip):

er, no, that's definitely sound?

gnzlbg (Dec 23 2019 at 13:02, on Zulip):

https://doc.rust-lang.org/std/ops/trait.Drop.html

simulacrum (Dec 23 2019 at 13:02, on Zulip):

I'm confused how that's unsound if it can panic

gnzlbg (Dec 23 2019 at 13:02, on Zulip):

So there, maybe add a section explaining what happens when Drop::drop panics

gnzlbg (Dec 23 2019 at 13:02, on Zulip):

that kind of ties to the issue in the rust-lang/reference about documenting that as well

bjorn3 (Dec 23 2019 at 13:02, on Zulip):

It is unsound if dropping after a panic is allowed.

bjorn3 (Dec 23 2019 at 13:03, on Zulip):

Should have been more clear

simulacrum (Dec 23 2019 at 13:03, on Zulip):

I don't follow

gnzlbg (Dec 23 2019 at 13:03, on Zulip):

I don't follow either

simulacrum (Dec 23 2019 at 13:03, on Zulip):

Dropping T after Drop::drop(&mut T) fails or succeeds is not allowed.

simulacrum (Dec 23 2019 at 13:04, on Zulip):

But that code looks safe in all cases?

gnzlbg (Dec 23 2019 at 13:04, on Zulip):

Yes, it looks good to me as well

gnzlbg (Dec 23 2019 at 13:04, on Zulip):

If dropping the vector panics, the fields of VecWrapper will still be dropped

gnzlbg (Dec 23 2019 at 13:04, on Zulip):

but the ManuallyDrop field doesn't impl Drop

simulacrum (Dec 23 2019 at 13:04, on Zulip):

i.e., is no different than this safe code:

struct VecWrapper {
    x: Vec<u8>,
}
bjorn3 (Dec 23 2019 at 13:05, on Zulip):

Better example:

struct VecWrapper {
    x: ManuallyDrop<Vec<u8>>,
}

impl Drop for VecWrapper {
    fn drop(&mut self) {
        unsafe { ManuallyDrop::drop(&mut self.x) }
        if rand_bool() { panic!() }
    }
}

If VecWrapper panics after the drop of Vec<u8>, then allowing redropping it would be unsound, as Vec<u8> would be dropped again.

simulacrum (Dec 23 2019 at 13:05, on Zulip):

Sure, yes, I think that's what we've agreed upon

gnzlbg (Dec 23 2019 at 13:06, on Zulip):
struct VecWrapper {
    x: Vec<u8>,
}

impl Drop for VecWrapper {
    fn drop(&mut self) {
        unsafe { ptr::drop_in_place(&mut self.x) }
        // mem::forget(mem::swap(self.x, Vec::new()));
         ptr::write(&mut self.x, Vec::new());
    }
}

is unsound.

gnzlbg (Dec 23 2019 at 13:06, on Zulip):

That tries to drop the Vec<u8>, which can panic, and if it does, then the x field will be dropped again, which is a double drop.

simulacrum (Dec 23 2019 at 13:06, on Zulip):

Most likely yes, though you can do ptr::write(&mut self.x, Vec::new()).

gnzlbg (Dec 23 2019 at 13:07, on Zulip):

Ah yes, you can do that as well

simulacrum (Dec 23 2019 at 13:07, on Zulip):

though actually, no, that's still unsound

simulacrum (Dec 23 2019 at 13:07, on Zulip):

you can't ptr::drop_in_place inside a Drop impl ~ever pretty much

simulacrum (Dec 23 2019 at 13:07, on Zulip):

(on a field, at least)

gnzlbg (Dec 23 2019 at 13:07, on Zulip):

oh, why not ?

simulacrum (Dec 23 2019 at 13:08, on Zulip):

Since Drop impls can't drop their fields

simulacrum (Dec 23 2019 at 13:08, on Zulip):

since they will be dropped after the drop impl

simulacrum (Dec 23 2019 at 13:08, on Zulip):

i.e., you need to "move out" of the field if you're going to drop it

simulacrum (Dec 23 2019 at 13:08, on Zulip):

otherwise every drop impl would need to drop all fields, even if you're e.g. closing a file or something

gnzlbg (Dec 23 2019 at 13:09, on Zulip):

i'm not sure I follow, are you saying that this is not possible, because the compiler errors ?

simulacrum (Dec 23 2019 at 13:09, on Zulip):

no, I'm saying that the ptr::drop_in_place as written is not sound

simulacrum (Dec 23 2019 at 13:09, on Zulip):

even if it doesn't panic

gnzlbg (Dec 23 2019 at 13:09, on Zulip):

or that it is not possible, because it is unsound ? (e.g. because there is a live &mut self reference, which implies there is a live self, and dropping the fields "invalidate" that

gnzlbg (Dec 23 2019 at 13:09, on Zulip):

since you have a reference to a dropped value

simulacrum (Dec 23 2019 at 13:09, on Zulip):

not quite

simulacrum (Dec 23 2019 at 13:09, on Zulip):

that too

simulacrum (Dec 23 2019 at 13:10, on Zulip):

that's I guess the reasoning you can use

simulacrum (Dec 23 2019 at 13:10, on Zulip):

but also we'll essentially call drop_in_place again

simulacrum (Dec 23 2019 at 13:10, on Zulip):

i.e., the fields will be dropped after Drop::drop for some struct executes

gnzlbg (Dec 23 2019 at 13:10, on Zulip):

sure, but when you do, after the ptr::write there will be a Vec::new() in that place, which can be dropped

simulacrum (Dec 23 2019 at 13:10, on Zulip):

and you cannot stop that

simulacrum (Dec 23 2019 at 13:10, on Zulip):

right, but if ptr::drop_in_place panics then you have a problem because it'll be dropped on the unwind edge I suspect

gnzlbg (Dec 23 2019 at 13:11, on Zulip):

yes, that's the point i was trying to illustrate

simulacrum (Dec 23 2019 at 13:11, on Zulip):

(and per earlier, that's UB)

gnzlbg (Dec 23 2019 at 13:11, on Zulip):

we do guarantee that the fields are dropped even if something within the Drop::drop panics

simulacrum (Dec 23 2019 at 13:11, on Zulip):

so there's ~no situation in which ptr::drop_in_place(&mut self.foo) is safe in Drop

simulacrum (Dec 23 2019 at 13:11, on Zulip):

since it's guaranteed to be called twice

gnzlbg (Dec 23 2019 at 13:11, on Zulip):

well, if you use ManuallyDrop, then you can

gnzlbg (Dec 23 2019 at 13:11, on Zulip):

I think we agree and are just talking past each other

simulacrum (Dec 23 2019 at 13:11, on Zulip):

sure, but then it's not self.foo :)

gnzlbg (Dec 23 2019 at 13:12, on Zulip):

@bjorn3 example "works"

gnzlbg (Dec 23 2019 at 13:12, on Zulip):

my example is one that would be unsound, precisely because drop_in_place can panic, and that means that some field will be dropped twice

simulacrum (Dec 23 2019 at 13:13, on Zulip):

sure, yes

gnzlbg (Dec 23 2019 at 13:13, on Zulip):

it might also be unsound because we might invalidate the &mut self , that's probably worth asking in the UCGs as well

simulacrum (Dec 23 2019 at 13:14, on Zulip):

https://github.com/rust-lang/rust/pull/67559

gnzlbg (Dec 23 2019 at 13:16, on Zulip):

https://rust-lang.zulipchat.com/#narrow/stream/136281-t-lang.2Fwg-unsafe-code-guidelines/topic/invalidate.20through.20drop/near/184103427

gnzlbg (Dec 23 2019 at 13:19, on Zulip):

@simulacrum that LGTM, let's wait to see what the others say, but this is already how things work, so I'm pretty sure we can't document this in any other way.

Last update: Jan 21 2020 at 08:20UTC