Fix double free when an element's Drop panics - #1618
Open
tooson9010-spec wants to merge 1 commit into
Open
Conversation
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.
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.
Five editing methods drop the removed elements in place and only shrink the
matrix afterwards, via
reallocate_copy. If an element'sDroppanics theshrink 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
Dropcan panic. Reportedearlier in #1615.
Affected:
Matrix::remove_columns_atMatrix::remove_rows_atMatrix::remove_columns_genericMatrix::remove_rows_generic(viacompress_rows)Matrix::resize_generic(both the directdrop_in_placeand thecompress_rowspath)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:
The excess matches the number of entries each method removes. With a
heap-owning element type, AddressSanitizer reports
attempting double-freeonthe same paths.
The fix
Hold the owned matrix in a
ManuallyDropand take it back out immediatelybefore
reallocate_copyconsumes its storage. A panic then leaks the remainingelements 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 Ileft it out.
Regression test
tests/core/edition.rsgains five cases that arm one element'sDropandassert that no element is destroyed more than once. Plain
cargo testcatchesit, no sanitizer needed; the rest of the suite is unaffected.
The three
macros::*_trybuild_testsfailures on a fullcargo testarepre-existing — they fail without this patch as well.