rust-lang / rust-lang/rust

HashMap: panic in element destructor causes leaks of unrelated elements

Open
#132,222 3 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

A-collections A-destructors C-discussion I-memleak T-libs
Dominant language
Rust
Stars
119k
Forks
16.1k
PR metrics pending

Description

When dropping a HashMap (or HashSet) and an element's destructor panics, then all elements that would be dropped after are leaked. This is inconsistent with other std collections (Vec, LinkedList, BTreeMap), where after panic in one destructor the remaining destructors are still called, potentially causing an abort if another one panics.

I tried this code: playground

use std::cell::Cell;
use std::collections::{BTreeMap, HashMap, LinkedList};
use std::panic::{catch_unwind, AssertUnwindSafe};

struct Dropper<'a>(&'a Cell<u32>);

impl Drop for Dropper<'_> {
    fn drop(&mut self) {
        let count = self.0.get();
        self.0.set(count + 1);
        if count == 0 {
            panic!("oh no");
        }
    }
}

fn main() {
    // Vec
    let count = Cell::new(0);
    catch_unwind(AssertUnwindSafe(|| {
        drop(vec![[Dropper(&count), Dropper(&count)]]);
    }))
    .unwrap_err();
    println!("vec: {}", count.get());

    // LinkedList
    let count = Cell::new(0);
    catch_unwind(AssertUnwindSafe(|| {
        drop(LinkedList::from([Dropper(&count), Dropper(&count)]));
    }))
    .unwrap_err();
    println!("linked list: {}", count.get());

    // BTreeMap
    let count = Cell::new(0);
    catch_unwind(AssertUnwindSafe(|| {
        drop(BTreeMap::from([(1, Dropper(&count)), (2, Dropper(&count))]));
    }))
    .unwrap_err();
    println!("b-tree map: {}", count.get());

    // HashMap
    let count = Cell::new(0);
    catch_unwind(AssertUnwindSafe(|| {
        drop(HashMap::from([(1, Dropper(&count)), (2, Dropper(&count))]));
    }))
    .unwrap_err();
    println!("hash map: {}", count.get());
}

I expected to see this happen: The drop behavior of Vec, LinkedList, BTreeMap and HashMap should be consistent.

Instead, this happened: HashMap drops only one element if the destructor unwinds, but the others drop both elements.

running Miri on the code shows a memory leak 
error: memory leaked: alloc17016 (Rust heap, size: 76, align: 8), allocated here:
  --> /playground/.cargo/registry/src/index.crates.io-6f17d22bba15001f/hashbrown-0.15.0/src/raw/alloc.rs:15:15
   |
15 |         match alloc.allocate(layout) {
   |               ^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: BACKTRACE:
   = note: inside `hashbrown::raw::alloc::inner::do_alloc::<std::alloc::Global>` at /playground/.cargo/registry/src/index.crates.io-6f17d22bba15001f/hashbrown-0.15.0/src/raw/alloc.rs:15:15: 15:37
   = note: inside `hashbrown::raw::RawTableInner::new_uninitialized::<std::alloc::Global>` at /playground/.cargo/registry/src/index.crates.io-6f17d22bba15001f/hashbrown-0.15.0/src/raw/mod.rs:1534:38: 1534:61
   = note: inside `hashbrown::raw::RawTableInner::fallible_with_capacity::<std::alloc::Global>` at /playground/.cargo/registry/src/index.crates.io-6f17d22bba15001f/hashbrown-0.15.0/src/raw/mod.rs:1572:30: 1572:96
   = note: inside `hashbrown::raw::RawTableInner::prepare_resize::<std::alloc::Global>` at /playground/.cargo/registry/src/index.crates.io-6f17d22bba15001f/hashbrown-0.15.0/src/raw/mod.rs:2633:13: 2633:94
   = note: inside `hashbrown::raw::RawTableInner::resize_inner::<std::alloc::Global>` at /playground/.cargo/registry/src/index.crates.io-6f17d22bba15001f/hashbrown-0.15.0/src/raw/mod.rs:2829:29: 2829:86
   = note: inside `hashbrown::raw::RawTableInner::reserve_rehash_inner::<std::alloc::Global>` at /playground/.cargo/registry/src/index.crates.io-6f17d22bba15001f/hashbrown-0.15.0/src/raw/mod.rs:2719:13: 2725:14
   = note: inside `hashbrown::raw::RawTable::<(i32, Dropper<'_>)>::reserve_rehash::<{closure@hashbrown::map::make_hasher<i32, Dropper<'_>, std::hash::RandomState>::{closure#0}}>` at /playground/.cargo/registry/src/index.crates.io-6f17d22bba15001f/hashbrown-0.15.0/src/raw/mod.rs:1045:13: 1056:14
   = note: inside `hashbrown::raw::RawTable::<(i32, Dropper<'_>)>::reserve::<{closure@hashbrown::map::make_hasher<i32, Dropper<'_>, std::hash::RandomState>::{closure#0}}>` at /playground/.cargo/registry/src/index.crates.io-6f17d22bba15001f/hashbrown-0.15.0/src/raw/mod.rs:993:20: 994:81
   = note: inside `hashbrown::map::HashMap::<i32, Dropper<'_>, std::hash::RandomState>::reserve` at /playground/.cargo/registry/src/index.crates.io-6f17d22bba15001f/hashbrown-0.15.0/src/map.rs:1102:9: 1103:77
   = note: inside `<hashbrown::map::HashMap<i32, Dropper<'_>, std::hash::RandomState> as std::iter::Extend<(i32, Dropper<'_>)>>::extend::<[(i32, Dropper<'_>); 2]>` at /playground/.cargo/registry/src/index.crates.io-6f17d22bba15001f/hashbrown-0.15.0/src/map.rs:4489:9: 4489:30
   = note: inside `<std::collections::HashMap<i32, Dropper<'_>> as std::iter::Extend<(i32, Dropper<'_>)>>::extend::<[(i32, Dropper<'_>); 2]>` at /playground/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/collections/hash/map.rs:3185:9: 3185:31
   = note: inside `<std::collections::HashMap<i32, Dropper<'_>> as std::iter::FromIterator<(i32, Dropper<'_>)>>::from_iter::<[(i32, Dropper<'_>); 2]>` at /playground/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/collections/hash/map.rs:3170:9: 3170:25
   = note: inside `<std::collections::HashMap<i32, Dropper<'_>> as std::convert::From<[(i32, Dropper<'_>); 2]>>::from` at /playground/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/collections/hash/map.rs:1405:9: 1405:29
note: inside closure
  --> src/main.rs:45:14
   |
45 |         drop(HashMap::from([(1, Dropper(&count)), (2, Dropper(&count))]));
   |              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   = note: inside `<{closure@src/main.rs:44:35: 44:37} as std::ops::FnOnce<()>>::call_once - shim` at /playground/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ops/function.rs:250:5: 250:71
   = note: inside `<std::panic::AssertUnwindSafe<{closure@src/main.rs:44:35: 44:37}> as std::ops::FnOnce<()>>::call_once` at /playground/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/panic/unwind_safe.rs:272:9: 272:19
   = note: inside `std::panicking::r#try::do_call::<std::panic::AssertUnwindSafe<{closure@src/main.rs:44:35: 44:37}>, ()>` at /playground/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/panicking.rs:557:40: 557:43
   = note: inside `std::panicking::r#try::<(), std::panic::AssertUnwindSafe<{closure@src/main.rs:44:35: 44:37}>>` at /playground/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/panicking.rs:520:19: 520:88
   = note: inside `std::panic::catch_unwind::<std::panic::AssertUnwindSafe<{closure@src/main.rs:44:35: 44:37}>, ()>` at /playground/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/panic.rs:358:14: 358:33
note: inside `main`
  --> src/main.rs:44:5
   |
44 | /     catch_unwind(AssertUnwindSafe(|| {
45 | |         drop(HashMap::from([(1, Dropper(&count)), (2, Dropper(&count))]));
46 | |     }))
   | |_______^
Meta

rustc --version --verbose:

rustc 1.84.0-nightly (c1db4dc24 2024-10-25)
binary: rustc
commit-hash: c1db4dc24267a707409c9bf2e67cf3c7323975c8
commit-date: 2024-10-25
host: x86_64-unknown-linux-gnu
release: 1.84.0-nightly
LLVM version: 19.1.1

@rustbot label T-libs A-collections A-destructors I-memleak

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Reproduce the panic and leak with the issue's example, then inspect library/std/src/collections/hash/map.rs and the related hashbrown drop paths shown in the trace. Compare the behavior with Vec, LinkedList, and BTreeMap, and use a regression test to verify that remaining HashMap and HashSet elements are dropped after unwinding.

Written by the indexing model from the issue text.

Assessment

Tech stack
rust
Domain
backend
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
38/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.