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?

[pnkfelix]

Reopening due to PR #81257

[lcnr]

This has a conceptionally clear fix by changing MIR building which does not cause any issues, expect a single borrowck regression in #80949. It has also been encountered in the wild at least once in #47949 (comment). Going to bump this to P-high.

I personally would be in favor of merging #78373 again, even if it still breaks euca if it is difficult to avoid that issue.

[nikomatsakis]

I agree we should fix this. I don't recall the solution in detail nor the issue though. @lcnr it seems to me that FCP is appropriate -- this is likely a @rust-lang/types FCP with @rust-lang/lang cc'd?

[lcnr]

An FCP to merge #78373 again, accepting the regressions caused by it, assuming no additional major crates started to depend on the current behavior?

hmm, not totally sure which team is the most responsible here. I feel like it may be types > @rust-lang/compiler > lang as it feels more like an implementation detail/bug than an actual lang design question🤔

Yes, that seems good to me. Will put this on my list of things I'd like to do, though I would love for someone else to summarize this for an FCP proposal, as I may not get to it in the near future.

[estebank]

@nikomatsakis this is something I explored with the panic reachability logic in redpen. I can detect that a panic might happen in a Drop impl, but for a clippy or rustc lint we'd need something a bit stronger where we know for a fact that the panic can hit (it's not just dead unreachable code).

[nikomatsakis]

@lcnr I agree with that ordering.

…

On Thu, May 23, 2024, at 3:40 PM, lcnr wrote: An FCP to merge #78373 <#78373> again, accepting the regressions caused by it, assuming no additional major crates started to depend on the current behavior? hmm, not totally sure which team is the most responsible here. I feel like it may be types > @rust-lang/compiler <https://github.com/orgs/rust-lang/teams/compiler> > lang as it feels more like an implementation detail/bug than an actual lang design question🤔 Yes, that seems good to me. Will put this on my list of things I'd like to do, though I would love for someone else to summarize this for an FCP proposal, as I may not get to it in the near future. — Reply to this email directly, view it on GitHub <#47949 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABF4ZT5HXOWTJMHOBNMZ23ZDZA2VAVCNFSM4EOZTMO2U5DIOJSWCZC7NNSXTN2JONZXKZKDN5WW2ZLOOQ5TEMJSG44DSNJTHA3Q>. You are receiving this because you modified the open/close state.Message ID: ***@***.***>