Audit liballoc for leaks in Drop impls when user destructor panics

[jonas-schievink]

Inspired by #67243 and #67235, this audits and hopefully fixes the remaining Drop impls in liballoc for resource leaks in the presence of panics in destructors called by the affected Drop impl.

This does not touch Hash{Map,Set} since they live in hashbrown. They have similar issues though.

r? @KodrAus

[KodrAus]

Thanks for working on this @jonas-schievink! I need to spend more time digging through the changes, especially to Vec. In general, I think we need comments on all of these drop guards that document what they're doing.

cc @rust-lang/libs do you have any thoughts on this? This PR makes collections try a bit harder to drop their contents in the presence of panics, so they behave in the same as ptr::drop_in_place and multiple locals when one of them panics during drop.

We've already done this for LinkedList and VecDeque. This PR includes:

• BinaryHeap::drain_sorted (which is unstable)
• BTreeMap::into_iter (used by BTreeMap during drop)
• LinkedList::drain_filter
• VecDeque::truncate
• VecDeque::drain
• Vec::into_iter
• Vec::drain

[alexcrichton]

I've historically hoped that it would never come to this myself, maintaining this sort of code is really hard and provides very little benefit in practice. In some sense though this was inevitable, so beyond auditing for correctness I don't think there's much we can do but merge this.

[Amanieu]

I would personally like to go the way C++ did and always abort on any unwind from a destructor (even if we weren't already unwinding). However I'm not quite sure whether that is a breaking change.

[dtolnay]

I haven't reviewed the implementation but if it does what it says (matching the behavior of ptr::drop_in_place in the case of panics) then I am on board with accepting this.

[SimonSapin]

What does ptr::drop_in_place do, btw? In particular if a second panic happens after the first one was caught.

[jonas-schievink]

Double panics always abort

[Mark-Simulacrum]

It sounds like we need to come to consensus about whether to accept this patch, or instead aim to opt for the behavior C++ has, i.e., aborting on panic in destructors unconditionally.

I think we should explore making destructors abort on panic, but in a separate PR (I would be interested in code size and possible performance benefits). For now I would land this PR -- it seems to be consistent with what we have been doing elsewhere (I have not verified the implementation in too much detail, but it seems to be correct -- I think there are tests for every new added impl, but they appear to be in different files so it's a bit hard to be certain without comparing one by one).

---

In terms of the PR itself, it seems like it would be nice to have some primitive for what amounts to Iterator::for_each(drop), but continuing on panic, rather than duplicating similar code across different Drop impls.

[bors]

☔ The latest upstream changes (presumably #67758) made this pull request unmergeable. Please resolve the merge conflicts.

[rust-highfive]

The job x86_64-gnu-llvm-7 of your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

2020-01-19T19:31:08.9563979Z ========================== Starting Command Output ===========================
2020-01-19T19:31:08.9565475Z [command]/bin/bash --noprofile --norc /home/vsts/work/_temp/9f4e992f-892f-4f96-a74e-ca851a57db42.sh
2020-01-19T19:31:08.9565508Z 
2020-01-19T19:31:08.9567941Z ##[section]Finishing: Disable git automatic line ending conversion
2020-01-19T19:31:08.9573874Z ##[section]Starting: Checkout rust-lang/rust@refs/pull/67290/merge to s
2020-01-19T19:31:08.9575504Z Task         : Get sources
2020-01-19T19:31:08.9575536Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.
2020-01-19T19:31:08.9575568Z Version      : 1.0.0
2020-01-19T19:31:08.9575601Z Author       : Microsoft
---
2020-01-19T19:31:09.9569875Z ##[command]git remote add origin https://github.com/rust-lang/rust
2020-01-19T19:31:09.9581477Z ##[command]git config gc.auto 0
2020-01-19T19:31:09.9583779Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2020-01-19T19:31:09.9585690Z ##[command]git config --get-all http.proxy
2020-01-19T19:31:09.9592121Z ##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/67290/merge:refs/remotes/pull/67290/merge

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

[KodrAus]

Thanks for your patience @jonas-schievink! It looks like we're generally on-board with doing this, so I've got some time this week to give it a thorough 👀

[dtolnay]

I agree with #67290 (comment) -- let's land this change once the implementation is reviewed, and then look into what it would take to make all panic inside Drop behave like a double panic. I think it should be doable. We'd be turning currently panicking programs into aborting programs which is no different from what this PR does.

[ssomers]

Am I right that in the case of drain_filter, the fix and tests are not about leaks, but about the desired stamina of the DrainFilter iterators. The way it has been and is in master, inside a DrainFilter's own Drop,

• When a panic occurs in the predicate, DrainFilter quits abruptly. It does not drain the element that freaked out the predicate, it does not do anything for any further element in the container. That behaviour stays (and is intuitive to me: you asked us to drain with a predicate, the predicate acts like a clown, so we're taking the easy way out).
• When the predicate said "yes, drain", but a panic occurs in the drop of a drained element, DrainFilter also quits. That's not a leak, because the remaining elements are still happily owned by the container. That behaviour is about to change: DrainFilter will invoke the predicate on all further elements, and drain and drop them if the predicate says to.

[KodrAus]

From a safety perspective this looks valid to me. We're mostly just shifting existing logic into guards, and I can't spot any invalid behavior when those guards are triggered partway through a dropping operation, since they remain internally consistent up to that point.

[Dylan-DPC-zz]

@KodrAus what's blocking this from getting merged?

[KodrAus]

@Dylan-DPC I wanted to look at @ssomers comment about DrainFilter more closely, but am happy to push forward now. The path we're on in this PR is:

• Panics during drop in all standard containers will try to continue dropping elements. If a subsequent element panics then, because we're already unwinding, we abort.
• Chances are if one element panicked then the next one will too, so a panicky drop in a collection will just end up aborting in practice.
• Panics in collections outside of drop don't do anything special.
• The next step will be considering making panic in drop always abort. I imagine that would need an RFC.

@bors r+

[bors]

📌 Commit e5987a0 has been approved by KodrAus

[ssomers]

My comments were really confined to linked_list::DrainFilter. I now know that part of this PR is mimicking the behaviour of vec::DrainFilter, which is a good thing regardless of the ideal behaviour. For its implementation, vec::DrainFilter even has a panic_flag (which means "panic in predicate", not in drop). I don't doubt that vec::Drain (which takes a range argument) probably is easier to understand and to implement, if the specified range is always completely removed, rather than leaving half of it in the vector after an element's drop panicked.

But unlike Drain, the DrainFilters have this predicate to invoke, which could also panic. Everyone seems to agree that it's better they don't try to finish the job by invoking the predicate on the remaining elements. So you've already lost an equivalent of Drains guarantee. Why add code to provide some guarantee in the case of drop panic, other than container invariants and avoiding actual leaks? It's stating that:

v.drain_filter(|_| true); 

cannot leave any element behind because that would be as bad as a leak, while it's normal to see elements left behind after:

v.drain_filter(|_| {
    assert!(something_mostly_true());
    true
}); 

And if the guarantee is considered important enough to provide code for, it should be important enough to document (for vec and linked_list and probably more alike). Right now, drain_filter's documentation doesn't even say that a drop of the iterator will continue the drain, which I genuinely wondered about and which the documentation of the stable vec::drain does clarify.

[bors]

⌛ Testing commit e5987a0 with merge 892cb14...

[bors]

☀️ Test successful - checks-azure
Approved by: KodrAus
Pushing 892cb14 to master...

[programmerjake]

Should the stuff in this unsafe block also run when unwinding from user drop? It looks like it does more deallocation.

[RalfJung]

FWIW, Miri can also detect memory leaks and I am running it on the liballoc test suite. It seems you are covered in terms of tests here, but if you think Miri could be helpful, please let me know.

Also, the latest run of Miri actually failed with a memory leak -- something landed during the last 9 days that made that happen. I'll have some fun figuring out the details.^^

[RalfJung]

According to Miri, an allocation made during this call leaks:

note: created allocation with id 143756
    --> /home/r/src/rust/rustc/src/liballoc/alloc.rs:81:47
     |
81   |     __rust_alloc(layout.size(), layout.align())
     |                                               ^ created allocation with id 143756
     |
     = note: inside call to `std::alloc::alloc` at /home/r/src/rust/rustc/src/liballoc/alloc.rs:169:22
     = note: inside call to `<std::alloc::Global as std::alloc::AllocRef>::alloc` at /home/r/src/rust/rustc/src/liballoc/alloc.rs:203:15
     = note: inside call to `alloc::alloc::exchange_malloc` at /home/r/src/rust/rustc/src/liballoc/boxed.rs:175:9
     = note: inside call to `std::boxed::Box::<alloc::collections::btree::node::LeafNode<&str, btree::map::test_into_iter_drop_leak::D>>::new` at /home/r/src/rust/rustc/src/liballoc/collections/btree/node.rs:211:43
     = note: inside call to `alloc::collections::btree::node::Root::<&str, btree::map::test_into_iter_drop_leak::D>::new_leaf` at /home/r/src/rust/rustc/src/liballoc/collections/btree/map.rs:1333:25
     = note: inside call to `std::collections::BTreeMap::<&str, btree::map::test_into_iter_drop_leak::D>::ensure_root_is_owned` at /home/r/src/rust/rustc/src/liballoc/collections/btree/map.rs:1067:9
     = note: inside call to `std::collections::BTreeMap::<&str, btree::map::test_into_iter_drop_leak::D>::entry` at /home/r/src/rust/rustc/src/liballoc/collections/btree/map.rs:838:15
note: inside call to `std::collections::BTreeMap::<&str, btree::map::test_into_iter_drop_leak::D>::insert` at alloc_miri_test/../liballoc/tests/btree/map.rs:1038:5
    --> alloc_miri_test/../liballoc/tests/btree/map.rs:1038:5
     |
1038 |     map.insert("a", D);
     |     ^^^^^^^^^^^^^^^^^^

### LEAK REPORT ###
Alloc 143756: 00 00 00 00 00 00 00 00 __ __ 05 00 __ __ __ __ 00 00 00 00 00 00 00 00 01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 01 00 00 00 00 00 00 00 __ __ __ __ __ __ __ __ __ __ __ __ __ __ __ __ __ __ __ __ __ __ __ __ __ __ __ __ __ __ __ __ __ __ __ __ __ __ __ __ __ __ __ __ __ __ __ __ __ __ __ __ __ __ __ __ __ __ __ __ __ __ __ __ __ __ __ __ __ __ __ __ __ __ __ __ __ __ __ __ __ __ __ __ __ __ __ __ __ __ __ __ __ __ __ __ (192 bytes, alignment 8) (Rust)
                                                              └──────(143732)───────┘                         └──────(143850)───────┘                         └──────(143963)───────┘                         └──────(144092)───────┘                         └──────(144237)───────┘ 
Alloc 143732: 61 (1 bytes, alignment 1) (Static)
Alloc 143850: 62 (1 bytes, alignment 1) (Static)
Alloc 143963: 63 (1 bytes, alignment 1) (Static)
Alloc 144092: 64 (1 bytes, alignment 1) (Static)
Alloc 144237: 65 (1 bytes, alignment 1) (Static)

Is that expected, or should all memory be properly freed?

[ssomers]

The root node of the tree in this simple test case is not freed. If the intention is to free all BTreeMap nodes as well, then the DropGuard in IntoIter::drop should include the rest of the normal body that frees the last pile of nodes of the tree (but doesn't need the is_shared_root test because the shared root doesn't have any elements to drop).

Most nodes of the tree have already been freed rather secretly in next calls, and as far as I see, these nodes have already been freed before the last key-value pair in them is dropped (which sounds peculiar, but the key and value dropped are in fact last minute copies). But to be sure of this, some test should cover trees with more than just a root node.

[RalfJung]

I'll open an issue around this

[RalfJung]

Issue is at #69769

[jonas-schievink]

Yeah looks like this is what @programmerjake pointed out above

[RalfJung]

You mean this comment, I presume? Yeah that seems related.

[RalfJung]

There's another memory leak in this test: the Vec backing store does not get deallocated. See #69770.