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

Panicking insert_many leaks memory #208

Closed
RalfJung opened this issue Apr 10, 2020 · 2 comments · Fixed by #213
Closed

Panicking insert_many leaks memory #208

RalfJung opened this issue Apr 10, 2020 · 2 comments · Fixed by #213

Comments

@RalfJung
Copy link
Contributor

RalfJung commented Apr 10, 2020

The test_insert_many_panic test leaks memory. Both of these allocations here to do get freed, presumably due to the panicking insert_many:

rust-smallvec/lib.rs

Lines 2096 to 2101 in 3957cd8

PanicOnDoubleDrop {
dropped: Box::new(false),
},
PanicOnDoubleDrop {
dropped: Box::new(false),
},

Found by Miri:

note: tracking was triggered
    --> /home/r/.rustup/toolchains/miri/lib/rustlib/src/rust/src/liballoc/alloc.rs:80:47
     |
80   |     __rust_alloc(layout.size(), layout.align())
     |                                               ^ created allocation with id 129830
     |
     = note: inside `alloc::alloc::alloc` at /home/r/.rustup/toolchains/miri/lib/rustlib/src/rust/src/liballoc/alloc.rs:80:47
     = note: inside `<alloc::alloc::Global as core::alloc::AllocRef>::alloc` at /home/r/.rustup/toolchains/miri/lib/rustlib/src/rust/src/liballoc/alloc.rs:174:49
     = note: inside `alloc::alloc::exchange_malloc` at /home/r/.rustup/toolchains/miri/lib/rustlib/src/rust/src/liballoc/alloc.rs:268:11
     = note: inside `alloc::boxed::Box::<bool>::new` at /home/r/.rustup/toolchains/miri/lib/rustlib/src/rust/src/liballoc/boxed.rs:174:9
note: inside `tests::test_insert_many_panic` at lib.rs:2100:26
    --> lib.rs:2100:26
     |
2100 |                 dropped: Box::new(false),
     |                          ^^^^^^^^^^^^^^^
note: inside closure at lib.rs:2072:5
    --> lib.rs:2072:5
     |
2072 | /     fn test_insert_many_panic() {
2073 | |         struct PanicOnDoubleDrop {
2074 | |             dropped: Box<bool>,
2075 | |         }
...    |
2107 | |         assert!(result.is_err());
2108 | |     }
     | |_____^

The following memory was leaked:
alloc129816 (Rust heap, size: 1, align: 1) {
    00                                              │ .
}
alloc129830 (Rust heap, size: 1, align: 1) {
    00                                              │ .
}
@mbrubeck
Copy link
Collaborator

This leak was intentional; see #96 and 26b2490 for details.

To fix this the leak without adding a double-free, the loop in insert_many will need to track which items are safe to drop, using a drop guard similar to the one used in from_elem and extend. (This might also have a performance impact, but I think fixing the leak is a higher priority than performance here, unless the performance impact is huge.)

@mbrubeck
Copy link
Collaborator

cc #186.

bors-servo added a commit that referenced this issue Apr 11, 2020
enable Miri leak checker

Works around #208
mbrubeck added a commit to mbrubeck/rust-smallvec that referenced this issue Apr 22, 2020
mbrubeck added a commit to mbrubeck/rust-smallvec that referenced this issue May 13, 2020
mbrubeck added a commit to mbrubeck/rust-smallvec that referenced this issue May 14, 2020
mbrubeck added a commit to mbrubeck/rust-smallvec that referenced this issue May 14, 2020
mbrubeck added a commit to mbrubeck/rust-smallvec that referenced this issue Jul 7, 2020
mbrubeck added a commit to mbrubeck/rust-smallvec that referenced this issue Jul 7, 2020
bors-servo added a commit that referenced this issue Jul 8, 2020
Fix leak on panic in `insert_many`.

Fixes #208 and reverts the workaround added in #209. CC @RalfJung.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants