Rust: Panics in destructors can cause the return value to be leaked

[arielb1]

STR

struct NoisyDrop;
impl Drop for NoisyDrop {
    fn drop(&mut self) {
        println!("dropping a NoisyDrop");
    }
}

impl NoisyDrop {
    fn new() -> Self {
        println!("creating a NoisyDrop");
        NoisyDrop
    }
}

struct PanickyDrop;
impl Drop for PanickyDrop {
    fn drop(&mut self) {
        panic!()
    }
}

fn foo() -> NoisyDrop {
    let p = PanickyDrop;
    NoisyDrop::new()
}

fn main() {
    foo();
}

Expected Result

"creating a NoisyDrop" and "dropping a NoisyDrop" should appear the same number of times

Actual Result

creating a NoisyDrop
thread 'main' panicked at 'explicit panic', src/main.rs:18:8
note: Run with `RUST_BACKTRACE=1` for a backtrace.

The destructor is ignored.

[jonas-schievink]

Heh, this even happens on Rust 1.0

[arielb1]

@jonas-schievink

Rust 1.0 is far worse than this - it leaks everything when a destructor panics.

[solb]

@arielb1, could you explain why this is tagged as a soundness bug? The language makes no guarantee that destructors will ever be invoked.

[gnzlbg]

ping @rust-lang/compiler this should be triagged, even if it is not a soundness bug, unwinding working properly is something that a lot of code relies on.

[nikomatsakis]

So, to be clear, it's not that we leak all destructors when panic in drop occurs. For example, this function drops both p and n as expected:

fn foo() {
    let n = NoisyDrop::new();
    let p = PanickyDrop;
}

The specific scenario here is that:

• we are in the middle of returning the NoisyDrop to our caller (so our caller "owns it")
• but we panic before our return completes (so they don't realize they own it)

I don't think it's quite right to say that "we make no guarantee that destructors will ever be invoked". We don't guarantee that all destructors are always invoked, but we do guarantee some things: for example, that if you have a local variable whose scope includes a call, it will be dropped when the call panics (here, n would be dropped):

let n = ...;
foo();

So the question is what we should guarantee in this particular scenario. Perhaps the answer is that we should just be very clear to document the current behavior, given how long it has existed.

[nikomatsakis]

Adding T-lang -- I think that documenting the expected order would be a good first step.

[nikomatsakis]

We discussed in the @rust-lang/lang meeting. Everyone agreed this behavior is wrong; the returned value ought to be dropped, presumably after all other variables in the frame have been dropped. Basically it should be equivalent to having stored the value into an outer let.

However, as a good first step, it seems like we should begin by verifying the behavior with a more thorough set of test cases. For example, I found (playground) that the problem applies not only to returns but to any store:

fn foo() -> NoisyDrop {
    let n = NoisyDrop::new(1);
    let x = {
        let p = PanickyDrop;
        NoisyDrop::new(2) // <-- dtor for this never runs 
    };
    panic!()
}

Ideally what we would do is this:

• Create various tests covering these corner cases (also perhaps things like panic when running the dtor of a struct field) so we know the current behavior
• Decide on the desired behavior and, depending on what we think, maybe writing up an RFC
• Implementing the fixes in drop elaboration

We thought perhaps @matthewjasper might be interested in tackling this, in particular =)

[nikomatsakis]

I'm leaving nominated so we follow up next meeting and discuss priority -- how serious do we think it is to fix this. Obviously, this is a long-standing behavior.

[matthewjasper]

Oh, there's an issue for this.

• Create various tests covering these corner cases (also perhaps things like panic when running the dtor of a struct field) so we know the current behavior

This should be done for evaluation order as well.

• Implementing the fixes in drop elaboration

The bug is in MIR construction. I don't think it's that hard to fix (except possibly for the return place).

We thought perhaps @matthewjasper might be interested in tackling this, in particular =)

=)

[nikomatsakis]

@matthewjasper

The bug is in MIR construction.

Oh?

[matthewjasper]

The latest perf results in https://github.com/rust-lang/rust/pull/66703#issuecomment-593002683 are a bit higher than I would like, but I can't see any way to reduce it.

[RalfJung]

So https://github.com/rust-lang/rust/pull/61430 was marked as fixing this, but actually did not, or why did the issue get reopened?

[matthewjasper]

61430 was reverted for being far too large a perf regression.

[RalfJung]

Oh, looks like it was backed out due to a perf regression. (EDIT: you beat me to it.^^)

I agree we should avoid perf regressions, but ultimately I think correct compilation is more important than performance... otherwise I have some good ideas for making the compiler go faster by producing incorrect code. ;) In particular I do think this qualifies as a soundness bug (like all incorrect compilations do); there might be unsafe code relying on rustc doing the right thing here.

[RalfJung]

Here's an example for why this is a soundness bug. weird_drop::drop is silly, but it should be correct, and this bug makes it cause UB (as you can see when running this code in Miri).

I'll re-add the I-unsound label.

[nikomatsakis]

Hmm, yeah, I had forgotten about this.

[Diggsey]

@RalfJung you predicted correctly, https://github.com/taiki-e/pin-project/pull/194 ran into this issue, and if @taiki-e hadn't spotted it, it would have caused unsoundness in the generated code.

[RalfJung]

@Diggsey @taiki-e that's a good catch!

So, can we revive #66703? I don't know what the policy is on how much perf impact we are willing to accept for soundness fixes -- this is probably a case-by-case thing.

[jonas-schievink]

@RalfJung It looks like Matthew is now going for a different approach in #71840 that should be more efficient

[matthewjasper]

It's the same approach as in #66703, which has perf runs.

[RalfJung]

https://github.com/rust-lang/rust/pull/71840 had to be reverted unfortunately to fix https://github.com/rust-lang/rust/issues/72470, it seems.

[matthewjasper]

Indeed, I'll create an issue to reapply it.

[Aaron1011]

https://github.com/rust-lang/rust/pull/71840 has been re-landed in https://github.com/rust-lang/rust/pull/77466. What's the next step for this issue?

[matthewjasper]

The next step is for me to resurrect #72152 and see how a perf run looks.