Skip to content

Fix double free when an element's Drop panics - #1618

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

Fix double free when an element's Drop panics#1618
tooson9010-spec wants to merge 1 commit into
dimforge:mainfrom
tooson9010-spec:fix-panic-safety-double-free

Conversation

@tooson9010-spec

Copy link
Copy Markdown

Five editing methods drop the removed elements in place and only shrink the
matrix afterwards, via reallocate_copy. If an element's Drop panics the
shrink never happens, so unwinding drops the whole matrix and destroys the
removed entries a second time.

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

Affected:

  • Matrix::remove_columns_at
  • Matrix::remove_rows_at
  • Matrix::remove_columns_generic
  • Matrix::remove_rows_generic (via compress_rows)
  • Matrix::resize_generic (both the direct drop_in_place and the
    compress_rows path)

Reproducer

The added tests count destructor calls on a 4×4 matrix with one armed Drop.
On the current code every case destroys more elements than exist:

remove_columns_at:       20 drops for 16 elements
remove_columns_generic:  24 drops for 16 elements
remove_rows_at:          17 drops for 16 elements
remove_rows_generic:     18 drops for 16 elements
resize_generic:          25 drops for 16 elements

The excess matches the number of entries each method removes. With a
heap-owning element type, AddressSanitizer reports attempting double-free on
the same paths.

The fix

Hold the owned matrix in a ManuallyDrop and take it back out immediately
before reallocate_copy consumes its storage. A panic then leaks the remaining
elements and the buffer instead of destroying them twice.

That's a trade — a double free becomes a leak on the unwind path. Recovering
the leak would need a guard that drops the still-valid ranges and then
deallocates, but deallocation is specific to DefaultAllocator::Buffer, so I
left it out.

Regression test

tests/core/edition.rs gains five cases that arm one element's Drop and
assert that no element is destroyed more than once. Plain cargo test catches
it, no sanitizer needed; the rest of the suite is unaffected.

The three macros::*_trybuild_tests failures on a full cargo test are
pre-existing — they fail without this patch as well.

remove_columns_at, remove_rows_at, remove_columns_generic,
remove_rows_generic and resize_generic drop the removed elements in place
and only shrink the matrix afterwards, via reallocate_copy. If T::drop
panics the shrink never happens, so unwinding drops the whole matrix and
destroys the removed entries a second time.

Hold the owned matrix in a ManuallyDrop and take it back out on the success
path, so a panic leaks the remaining elements instead.
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