Panics in destructors can cause the return value to be leaked

Opened by Ariel Ben-Yehuda at 2024-05-24 22:37:28

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.

Heh, this even happens on Rust 1.0
Jonas Schievink at 2018-02-01 23:21:55
@jonas-schievink

Rust 1.0 is far worse than this - it leaks everything when a destructor panics.
Ariel Ben-Yehuda at 2018-02-02 17:31:03
@arielb1, could you explain why this is tagged as a soundness bug? The language makes no guarantee that destructors will ever be invoked.
Sol Boucher at 2018-03-08 08:45:06
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.
gnzlbg at 2019-05-07 16:48:07
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.
Niko Matsakis at 2019-05-07 18:35:22
Adding T-lang -- I think that documenting the expected order would be a good first step.
Niko Matsakis at 2019-05-09 13:15:48
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 =)
Niko Matsakis at 2019-05-09 20:18:46
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.
Niko Matsakis at 2019-05-09 20:19:45
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 =)

=)
matthewjasper at 2019-05-09 20:38:43
@matthewjasper

The bug is in MIR construction.

Oh?
Niko Matsakis at 2019-05-16 14:29:10
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.
matthewjasper at 2020-04-19 08:43:32
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?
Ralf Jung at 2020-04-19 09:01:28
#61430 was reverted for being far too large a perf regression.
matthewjasper at 2020-04-19 09:03:19
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.
Ralf Jung at 2020-04-19 09:04:05
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.
Ralf Jung at 2020-04-19 09:35:59
Hmm, yeah, I had forgotten about this.
Niko Matsakis at 2020-04-20 22:23:35
@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.
Diggory Blake at 2020-05-03 14:42:22
@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.
Ralf Jung at 2020-05-03 15:17:14
@RalfJung It looks like Matthew is now going for a different approach in #71840 that should be more efficient
Jonas Schievink at 2020-05-03 15:20:17
It's the same approach as in #66703, which has perf runs.
matthewjasper at 2020-05-03 15:32:09
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.
Ralf Jung at 2020-06-07 08:02:01
Indeed, I'll create an issue to reapply it.
matthewjasper at 2020-06-07 10:30:25
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?
Aaron Hill at 2020-10-05 19:52:33
The next step is for me to resurrect #72152 and see how a perf run looks.
matthewjasper at 2020-10-10 20:04:35
Reopening due to PR #81257
Felix S Klock II at 2021-02-05 19:28:58
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 https://github.com/rust-lang/rust/issues/47949#issuecomment-623120647. Going to bump this to P-high.

I personally would be in favor of merging https://github.com/rust-lang/rust/pull/78373 again, even if it still breaks euca if it is difficult to avoid that issue.
lcnr at 2024-05-17 00:38:55
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?
Niko Matsakis at 2024-05-23 10:17:36
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.
lcnr at 2024-05-23 19:40:03
@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).
Esteban Kuber at 2024-05-24 16:29:32
@lcnr I agree with that ordering.

On Thu, May 23, 2024, at 3:40 PM, lcnr wrote:

An FCP to merge #78373 https://github.com/rust-lang/rust/pull/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 https://github.com/rust-lang/rust/issues/47949#issuecomment-2127895387, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABF4ZT5HXOWTJMHOBNMZ23ZDZA2VAVCNFSM4EOZTMO2U5DIOJSWCZC7NNSXTN2JONZXKZKDN5WW2ZLOOQ5TEMJSG44DSNJTHA3Q. You are receiving this because you modified the open/close state.Message ID: @.***>

Niko Matsakis at 2024-05-24 22:37:28