Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Double Drop with #![deny(rust_2024_compatibility)] #134482

Closed
Clipi-12 opened this issue Dec 18, 2024 · 3 comments · Fixed by #134486
Closed

Double Drop with #![deny(rust_2024_compatibility)] #134482

Clipi-12 opened this issue Dec 18, 2024 · 3 comments · Fixed by #134486
Assignees
Labels
A-edition-2024 Area: The 2024 edition C-bug Category: This is a bug. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness L-tail_expr_drop_order Lint: tail_expr_drop_order P-critical Critical priority regression-from-stable-to-beta Performance or correctness regression from stable to beta. S-has-bisection Status: a bisection has been found for this issue S-has-mcve Status: A Minimal Complete and Verifiable Example has been found for this issue T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@Clipi-12
Copy link

Clipi-12 commented Dec 18, 2024

I had a custom Rc that was doubly freeing memory because Drop was being called more times than it should.
After a little bit of testing I think this is a small enough Minimal-Reproducible-Example:
playground

// ---------------------------------------- //
// If commented, it only calls Drop 2 times //
// If not commented,  it calls Drop 3 times //
// ---------------------------------------- //
#![deny(rust_2024_compatibility)]

use std::backtrace::Backtrace;

struct Guard();
impl Clone for Guard {
    fn clone(&self) -> Self {
        Self()
    }
}
impl Drop for Guard {
    fn drop(&mut self) {
        println!("Drop!");
        eprintln!("Drop at {}!", Backtrace::force_capture());
    }
}

fn main() {
    let _ = method_1(Guard());
}

fn method_1(g: Guard) -> Result<String, ()> {
    // ----------------------- //
    // This calls Drop 2 times //
    // ----------------------- //

    // let argument = &g.clone();
    // match method_2(argument) {
    //     Ok(str) => Ok(str),
    //     Err(err) => Err(err),
    // }

    // --- //

    // ----------------------- //
    // This calls Drop 3 times //
    // ----------------------- //

    match method_2(&g.clone()) {
        Ok(str) => Ok(str),
        Err(err) => Err(err),
    }
}

fn method_2(_: &Guard) -> Result<String, ()> {
    panic!("Method 2 panics!");
}

I expected to see this happen: Drop is called twice.

Instead, this happened: Drop is called 3 times.

Notes

  • The return type doesn't need to be a Result<String, ()>
    It seems that it happens as long as it is an enum in which one arm has a non-Copy type.
    So it also happens if the return type is
enum CustomEnum {
    MyArm(Vec<u32>),
}
  • The problems seems to happen regardless of the presence of the --release flag
  • As seen in the backtrace below, the first drop is called at the end of the enum match, and the other two are called at the end of method_1

Meta

rustc --version --verbose:

rustc 1.85.0-nightly (a4cb3c831 2024-12-17)
binary: rustc
commit-hash: a4cb3c831823d9baa56c3d90514b75b2660116fa
commit-date: 2024-12-17
host: x86_64-pc-windows-msvc
release: 1.85.0-nightly
LLVM version: 19.1.5
Backtrace

Reduced stderr from the playground:

     Running `target/debug/playground`
thread 'main' panicked at src/main.rs:51:5:
Method 2 panics!
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
Drop at    0: <playground::Guard as core::ops::drop::Drop>::drop
             at ./src/main.rs:18:33
   1: core::ptr::drop_in_place<playground::Guard>
             at ./.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ptr/mod.rs:522:1
   2: playground::method_1
             at ./src/main.rs:46:5
   3: [...]

Drop at    0: <playground::Guard as core::ops::drop::Drop>::drop
             at ./src/main.rs:18:33
   1: core::ptr::drop_in_place<playground::Guard>
             at ./.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ptr/mod.rs:522:1
   2: playground::method_1
             at ./src/main.rs:47:1
   3: [...]

Drop at    0: <playground::Guard as core::ops::drop::Drop>::drop
             at ./src/main.rs:18:33
   1: core::ptr::drop_in_place<playground::Guard>
             at ./.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ptr/mod.rs:522:1
   2: playground::method_1
             at ./src/main.rs:47:1
   3: [...]

@Clipi-12 Clipi-12 added the C-bug Category: This is a bug. label Dec 18, 2024
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Dec 18, 2024
@lukas-code lukas-code added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. regression-from-stable-to-beta Performance or correctness regression from stable to beta. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness labels Dec 18, 2024
@rustbot rustbot added I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Dec 18, 2024
@lukas-code
Copy link
Member

caused by #131326 cc @dingxiangfei2009

@compiler-errors
Copy link
Member

compiler-errors commented Dec 18, 2024

I've begun to take a look at this. It has to do with the fact that we weren't handling the "fake" drop terminator we insert for lints as a real drop on unwind cleanups.

@compiler-errors compiler-errors self-assigned this Dec 18, 2024
@lukas-code lukas-code added S-has-bisection Status: a bisection has been found for this issue L-tail_expr_drop_order Lint: tail_expr_drop_order and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Dec 18, 2024
@ehuss ehuss added the A-edition-2024 Area: The 2024 edition label Dec 18, 2024
@traviscross traviscross added the S-has-mcve Status: A Minimal Complete and Verifiable Example has been found for this issue label Dec 19, 2024
@apiraino
Copy link
Contributor

WG-prioritization assigning priority (Zulip discussion).

@rustbot label -I-prioritize +P-high

@rustbot rustbot added P-high High priority and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Dec 19, 2024
@apiraino apiraino added P-critical Critical priority and removed P-high High priority labels Dec 19, 2024
@bors bors closed this as completed in 11663cd Dec 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-edition-2024 Area: The 2024 edition C-bug Category: This is a bug. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness L-tail_expr_drop_order Lint: tail_expr_drop_order P-critical Critical priority regression-from-stable-to-beta Performance or correctness regression from stable to beta. S-has-bisection Status: a bisection has been found for this issue S-has-mcve Status: A Minimal Complete and Verifiable Example has been found for this issue T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
7 participants