Skip to content

Fix double free when an item's Drop panics - #60

Open
tooson9010-spec wants to merge 1 commit into
agerasev:masterfrom
tooson9010-spec:fix-panic-safety-double-free
Open

Fix double free when an item's Drop panics#60
tooson9010-spec wants to merge 1 commit into
agerasev:masterfrom
tooson9010-spec:fix-panic-safety-double-free

Conversation

@tooson9010-spec

Copy link
Copy Markdown

skip() and clear() drop the occupied items in a loop and advance the read
index only afterwards. If an item's Drop panics the index update never
happens, so the ring buffer still treats the destroyed items as live and its own
Drop — which calls clear() — destroys them again.

Reachable from safe Rust with any item type whose Drop can panic. Reported
earlier in #59.

Reproducer

The added test counts destructor calls on a 4-item buffer with one armed
Drop. On the current code clear destroys more items than exist:

clear: 5 drops for 4 items

With a heap-owning item type, AddressSanitizer reports attempting double-free
while the ring buffer's destructor runs.

The fix

Advance the read index one slot at a time, before each drop_in_place, so a
panicking Drop leaves the item outside the occupied range. clear() now
delegates to skip() so the ordering rule lives in one place.

This re-derives the occupied slices on every iteration, which costs more than
the single call the old loop made. If that matters here, a drop guard that keeps
the batch loop and fixes up the index on unwind would work too.

Regression test

src/tests/skip.rs gains a case that arms one item's Drop and asserts that no
item is destroyed more than once, for both clear and a partial skip. Plain
cargo test catches it, no sanitizer needed; the rest of the suite passes.

skip() and clear() dropped the occupied items in a loop and advanced the
read index only afterwards. If T::drop panics the index update never
happens, so the ring buffer's own Drop destroys the same items again.

Advance the read index one slot at a time, before each drop_in_place.
clear() now delegates to skip().
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 this pull request may close these issues.

1 participant