Fix panic-safety in AlignedBox::realloc (double-free on panicking element Drop) - #6
Conversation
…ment Drop) When shrinking, realloc takes the Box out of self.container and drops the tail elements. Each element's Drop is user-controlled and may panic; if it does, self.container still holds the old pointer while ownership has moved out, so AlignedBox's own Drop frees those elements a second time -- a double-free reachable from safe Rust. Drop the tail back to front under a guard that, on unwind, restores self.container/self.layout to the still-live prefix, so every element is freed exactly once. Adds a regression test.
|
Two notes:
|
|
Hi. Thanks for tracking this down and providing a fix! The first CI fail is just a missing I think there is an off-by-one error in the set value for |
|
Ah, this is what you described in your second note. The reasoning is that we don't really know about the state of that object, so we don't restore it, correct? I guess we would have to call realloc again to reduce the size of the memory allocation accordingly. |
|
I was able to fix the miri issue with the following change: --- a/src/lib.rs
+++ b/src/lib.rs
@@ -296,10 +296,13 @@ impl<T> AlignedBox<[T]> {
impl<T> Drop for ShrinkGuard<T> {
fn drop(&mut self) {
unsafe {
- let slice = std::slice::from_raw_parts_mut(self.elem_ptr, self.valid);
let memsize = std::mem::size_of::<T>() * self.valid;
let layout = alloc::alloc::Layout::from_size_align(memsize, self.align)
- .expect("prefix layout is valid");
+ .expect("prefix layout is invalid");
+ let new_ptr =
+ alloc::alloc::realloc(self.elem_ptr as *mut u8, *self.layout, memsize)
+ as *mut T; // FIXME check ret
+ let slice = std::slice::from_raw_parts_mut(new_ptr, self.valid);
*self.container =
std::mem::ManuallyDrop::new(alloc::boxed::Box::from_raw(slice));
*self.layout = layout;However, we need to check the return value of |
|
Thanks for reviewing, and sorry for the bad patch. You're right — the guard shrinks On the off-by-one: Rather than calling I don't have a machine to test on right now, so I'll check it under miri in a couple of days and report back! Thank you again for the detailed review! |
The guard rebuilt the box at the prefix length but left the allocation at its original size, so Drop deallocated with a mismatched layout and a pointer whose provenance no longer covered the allocation. Shrink the allocation with realloc instead, keeping container.len() * size_of::<T>() equal to layout.size(). If that realloc fails the original allocation is untouched, so reinitialize the dropped slots and restore the full-length box, mirroring the existing realloc-failure path.
|
Sorry for the delay. Leaving the layout alone fixes the size mismatch, but miri then reports a The guard now reallocs, with the return value checked. On failure the original Miri, the sanitizers, tests and clippy all pass locally. The CI run needs your Still not covered: if initializer panics inside the guard, that aborts. That's |
|
Thanks a lot for the follow-up fix! If you want to look into the handling of a panic during the initializer call as well, I would be happy to review any changes. Otherwise, I hope to find some time to look into it soon. |
|
@michaellass I'd rather leave the initializer path to you, if that's alright. It's a different Two questions on process. Are you planning a release with this fix? And would you |
|
@tooson9010-spec |
|
@michaellass |
2 participants
Found while auditing this crate's unsafe teardown paths for panic-safety.
Summary
When shrinking, realloc takes the Box out of self.container and drops the tail
elements. If an element's Drop panics, self.container still holds the old pointer
while ownership has moved out, so AlignedBox's own Drop frees those elements a
second time, a double-free (CWE-415) reachable from safe Rust.
Fix
Drop the tail back to front under a guard. On unwind the guard restores
self.container and self.layout to the still-live prefix [0..valid], so every
element is freed exactly once. On the normal path the guard is disarmed and the
existing realloc flow continues unchanged.
Verification
Added realloc_shrink_panicking_drop_is_sound: a box of elements whose Drop panics
is shrunk, then dropped. Without the fix the second drop double-frees the tail
(glibc "double free detected", SIGABRT); with the fix it unwinds cleanly.
Existing tests pass. Confirmed on 0.3.0.