Fix panic-safety of eleven methods, and Deque's ring buffer indexing - #9
Open
tooson9010-spec wants to merge 6 commits into
Open
Fix panic-safety of eleven methods, and Deque's ring buffer indexing#9tooson9010-spec wants to merge 6 commits into
tooson9010-spec wants to merge 6 commits into
Conversation
An element's destructor may unwind. truncate committed the new length only after destroying the tail, so an unwind left the vector claiming ownership of already-destroyed elements, which Drop for Vec then destroyed a second time. Commit the length first and destroy the tail as a slice, matching Drop for Vec and std's Vec::truncate. Destroying the tail as a slice also keeps the remaining elements from leaking when one of them unwinds. Adds an opt-in panic_on_drop to test_utils::Droppable, and pulls in std under cfg(test) for catch_unwind.
These four methods destroy values in place and commit the metadata that removes them from the collection's logical state only afterwards. A destructor is user-controlled and may unwind, and for ListMap and PackedPool the Drop impl calls clear again, re-entering the same loop over stale metadata. ListMap::clear, PackedPool::clear: commit the emptied state first, then destroy the values as a slice so a panicking destructor does not leak the remaining ones. PackedPool recycles its slots in a separate loop beforehand, as that part cannot panic. ListMap::remove, LruCache2::get_or_insert_with, UnitCache::clear: move the removed values out and finish updating the collection before running their destructors, so an unwind cannot leave a slot both uninitialized and claimed.
The method bumped the length and made the slot's generation counter odd -- the pool's encoding of an occupied slot -- before calling the user-supplied closure that produces the value. If the closure unwound, the slot stayed uninitialized while the pool still claimed it, and Drop for DirectPool destroyed that uninitialized memory. Run the closure first and update the pool only once the value exists. The free-list link is read out of the slot union before the value is written over it. Without the fix the added test terminates the process with SIGSEGV.
Both methods addressed the ring buffer with a bare 'i % capacity', ignoring the 'front' offset that physical_index applies everywhere else. With a non-zero front -- one push_front is enough -- they destroyed slots that were never initialized, passed uninitialized memory to the user's predicate, and left live elements untouched. No panic is needed to reach this. Both also committed the new length only after destroying or compacting, so an unwinding destructor or predicate left the deque claiming values it no longer owned. truncate now translates logical indices through front, commits the length first, and drops the removed range as one or two slices so a panicking destructor does not leak the elements behind it. retain translates its source and destination indices through front and keeps its progress in a drop guard, which compacts the unvisited tail back into the deque when the predicate or a destructor unwinds.
The previous version pushed to the front, which meant the indexing bug diverted the traversal before any element was compacted -- the test passed against the unfixed code. Keep the front at zero so the predicate unwinds after two elements have been copied forward, which is the state the fix is about.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes the panic-safety issues from #8. While writing the tests I hit a
separate
Dequebug that isn't in that issue — it's fixed here too, seebelow.
Deque addresses the ring buffer without the front offset
Deque::truncateandretaincompute physical slots asi % capacity,but everywhere else in the type logical indices go through
physical_index, which is(front + i) % capacity. Onepush_frontmakes
frontnon-zero, and from then on both methods walk the wrongslots — destroying memory that was never initialized, handing it to the
user's predicate as a
&T, and leaving live elements alone. No panicinvolved.
retain_respects_frontbuilds a deque holding[10, 20, 30]and recordswhat the predicate sees; before the fix that's
[20, 30, <garbage>].truncate_respects_frontends the test process with SIGSEGV.Panic-safety
Nine of the reported methods destroy values and only afterwards commit the
metadata that removes them from the collection. For
ListMapandPackedPoolthat's worse than a stale length, since theirDropimplscall
clearand re-enter the loop they just unwound out of.Vec::truncate,Deque::truncate,ListMap::clear,PackedPool::clearand
UnitCache::clearnow commit first. Where the values are contiguousthey're dropped as a slice, so a panicking destructor doesn't leak the
ones behind it.
PackedPoolrecycles its slots in a separate passbeforehand — that part can't panic.
ListMap::removeandLruCache2::get_or_insert_withinstead read thevalue out of the slot before running anything user-controlled, which ends
the collection's claim on it right away.
DirectPool::try_insert_with_handleneeded the opposite: it marked theslot occupied before calling the closure that fills it, so it now runs the
closure first. Its test segfaults without the fix.
Deque::retainis the one case the predicate alone can break —ptr::copyleaves the source valid, so a compacted element briefly lives in two slots
that are both inside the current length. It now keeps its progress in a
drop guard that compacts the unvisited tail back in on unwind.
Tests
Eleven, all in the existing
mod testsblocks exceptcache.rsandlist_map.rs, which had none. Each was run against the unpatched codefirst and fails there — inflated drop count, wrong value reaching user
code, or SIGSEGV.
test_utils::Droppablegets an opt-inpanic_on_dropthat disarms itselfbefore unwinding, so a double drop shows up as a count rather than an
abort.
stdcomes in undercfg(test)forcatch_unwind. 223 doc-testsand the existing unit tests still pass.