Repository navigation
Fix panic-safety unsoundness in LinkedHashMap::into_iter and clear — #116 - #116
Open
tooson9010-spec wants to merge 2 commits into
Open
tooson9010-spec wants to merge 2 commits into
tooson9010-spec wants to merge 2 commits into
Conversation
…king hasher Drop) into_iter frees the sentinel node and drops the internal HashMap, then commits the ownership transfer with mem::forget(self). The HashMap owns the hasher S, whose Drop is user-controlled and may panic. If it panics after the sentinel is freed, mem::forget is skipped and the original LinkedHashMap is dropped during unwinding, whose destructor follows the now-dangling head pointer -- a use-after-free (CWE-416) reachable from safe Rust. Wrap self in ManuallyDrop so the original is never dropped automatically, and add a regression test.
…lue Drop) clear frees the value nodes (running each K/V's user-controlled Drop) and only afterwards resets the sentinel's guard links. If a value's Drop panics partway through, the reset is skipped and self.head is left pointing at freed memory; the map's own destructor then re-walks the chain during unwinding and frees it again -- a double-free (CWE-415) reachable from safe Rust. Detach the guard links before freeing so unwinding leaves an empty, self-consistent list, and add a regression test.
tooson9010-spec
changed the title
This branch has not been deployed
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.
Two panic-safety bugs in LinkedHashMap. Both are memory corruption triggered by
a user's Drop panicking mid-teardown.
into_iter (use-after-free, CWE-416)
into_iter frees the sentinel node and drops the internal HashMap, then commits
the ownership transfer with mem::forget(self). The HashMap owns the hasher S,
whose Drop can panic. If it panics after the sentinel is freed, mem::forget is
skipped and the original LinkedHashMap is dropped during unwinding — its
destructor follows the dangling head pointer.
Fix: wrap self in ManuallyDrop instead of relying on mem::forget(self) after the
fallible teardown. On panic the remaining nodes leak, which is fine.
clear (double-free, CWE-415)
clear frees the value nodes, running each K/V's Drop, and resets the guard links
afterwards. A value Drop that panics partway leaves the reset unrun, so self.head
still points into freed memory. The map's own destructor then walks that chain
again and frees it a second time.
Fix: reset the guard links before freeing anything. self.head is an empty list
by the time any unwind can start, so nothing gets freed twice.
Verification
Existing tests pass. Added two regression tests (catch_unwind):
into_iter_panicking_hasher_is_sound — ASAN reports heap-use-after-free without
the fix; clear_panicking_value_drop_is_sound — SIGSEGV / double-free without the
fix. Both unwind cleanly once fixed. Confirmed on 0.5.6.