Skip to content

Fix double free when retain unwinds - #4

Merged
OverwrittenCode merged 3 commits into
OverwrittenCode:mainfrom
tooson9010-spec:fix-panic-safety-double-free
Sep 22, 2026
Merged

OverwrittenCode merged 3 commits into
OverwrittenCode:mainfrom
tooson9010-spec:fix-panic-safety-double-free

Conversation

@tooson9010-spec

Copy link
Copy Markdown
Contributor

retain compacts kept elements with ptr::copy_nonoverlapping and commits the
new length only after the loop. Two ways a panic leaves the buffer unsound:

  • The predicate unwinds after a compaction. copy_nonoverlapping leaves the
    source slot as a byte-for-byte duplicate, so both copies stay inside 0..len
    and StackVec::drop frees the same value twice. No element Drop has to
    panic — an unwrap() or assert! in the predicate is enough.
  • A removed element's Drop unwinds. The slot was already destroyed but stays
    inside 0..len.

Reported in #3.

Reproducer

The added tests count destructor calls on a 4-element vec. Both paths destroy
more than exist:

5 drops for 4 elements

AddressSanitizer reports attempting double-free with a heap-owning element
type.

The fix

Retire the buffer up front and let a backshift guard restore a correct length on
every exit path, the same shape Vec::retain uses. The removed slot is counted
out before drop_in_place, so a panicking element Drop can't be revisited
either.

Regression test

Two cases in src/stack_vec.rs, one per path. cargo test catches both, no
sanitizer needed; retain_with_drops and the retain doc-test still pass.

@tooson9010-spec
tooson9010-spec force-pushed the fix-panic-safety-double-free branch 2 times, most recently from 736589d to d2e5aed Compare August 27, 2026 04:40
retain compacts kept elements with copy_nonoverlapping and commits the new
length only after the loop. Both the predicate and a removed element's Drop
are user code, so an unwind leaves either a duplicated slot or an
already-dropped one inside 0..len, and StackVec::drop destroys it again.

Use a backshift guard that restores a correct length on every exit path,
matching what Vec::retain does.
@tooson9010-spec
tooson9010-spec force-pushed the fix-panic-safety-double-free branch from d2e5aed to cbaf9e2 Compare August 27, 2026 04:42
@OverwrittenCode
OverwrittenCode self-requested a review September 22, 2026 18:26
Signed-off-by: OverwrittenCode <83100104+OverwrittenCode@users.noreply.github.com>
Signed-off-by: OverwrittenCode <83100104+OverwrittenCode@users.noreply.github.com>

@OverwrittenCode OverwrittenCode left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verified the fix locally: cargo miri test --all-features and
cargo clippy --all-features -- -D warnings both pass, including
the two new panic-safety regression tests.

Pushed two small fixups on top:

  • resolved a Stacked Borrows violation Miri caught in the
    BackshiftOnDrop guard and the retain loop (interleaved
    as_ptr()/as_mut_ptr() calls were invalidating each other's
    reborrows)
  • moved the item definitions before the first statement and
    split a 3-op unsafe block to satisfy clippy::pedantic

Thanks for the clear repro and the regression tests - good catch.

@OverwrittenCode
OverwrittenCode merged commit fa5b98d into OverwrittenCode:main Sep 22, 2026
13 checks passed
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.

2 participants