Skip to content

Commit a1a0794

Browse files
committed
fix some memory leaks detected by miri (#4959)
The first leak: ```rust #[test] fn blob_vec_drop_empty_capacity() { let item_layout = Layout::new::<Foo>(); let drop = drop_ptr::<Foo>; let _ = unsafe { BlobVec::new(item_layout, Some(drop), 0) }; } ``` this is because we allocate the swap scratch in blobvec regardless of what the capacity is, but we only deallocate if capacity is > 0 The second leak: ```rust #[test] fn panic_while_overwriting_component() { let helper = DropTestHelper::new(); let res = panic::catch_unwind(|| { let mut world = World::new(); world .spawn() .insert(helper.make_component(true, 0)) .insert(helper.make_component(false, 1)); println!("Done inserting! Dropping world..."); }); let drop_log = helper.finish(res); assert_eq!( &*drop_log, [ DropLogItem::Create(0), DropLogItem::Create(1), DropLogItem::Drop(0), ] ); } ``` this is caused by us not running the drop impl on the to-be-inserted component if the drop impl of the overwritten component panics --- managed to figure out where the leaks were by using this 10/10 command ``` cargo --quiet test --lib -- --list | sed 's/: test$//' | MIRIFLAGS="-Zmiri-disable-isolation" xargs -n1 cargo miri test --lib -- --exact ``` which runs every test one by one rather than all at once which let miri actually tell me which test had the leak :upside_down_face:
1 parent cdbabb7 commit a1a0794

File tree

3 files changed

+20
-4
lines changed

3 files changed

+20
-4
lines changed

.github/workflows/ci.yml

+2-3
Original file line numberDiff line numberDiff line change
@@ -99,11 +99,10 @@ jobs:
9999
RUSTFLAGS: -Zrandomize-layout
100100
# https://github.com/rust-lang/miri#miri--z-flags-and-environment-variables
101101
# -Zmiri-disable-isolation is needed because our executor uses `fastrand` which accesses system time.
102-
# -Zmiri-ignore-leaks is needed because running bevy_ecs tests finds a memory leak but its impossible
103-
# to track down because allocids are nondeterministic.
104102
# -Zmiri-permissive-provenance disables warnings against int2ptr casts (since those are used by once_cell)
105103
# -Zmiri-disable-weak-memory-emulation works around https://github.com/bevyengine/bevy/issues/5164.
106-
MIRIFLAGS: -Zmiri-disable-isolation -Zmiri-ignore-leaks -Zmiri-permissive-provenance -Zmiri-disable-weak-memory-emulation
104+
# -Zmiri-ignore-leaks is necessary because a bunch of tests don't join all threads before finishing.
105+
MIRIFLAGS: -Zmiri-ignore-leaks -Zmiri-disable-isolation -Zmiri-permissive-provenance -Zmiri-disable-weak-memory-emulation
107106

108107
check-compiles:
109108
runs-on: ubuntu-latest

crates/bevy_ecs/src/storage/blob_vec.rs

+17-1
Original file line numberDiff line numberDiff line change
@@ -151,8 +151,20 @@ impl BlobVec {
151151
let ptr = self.get_unchecked_mut(index).promote().as_ptr();
152152
self.len = 0;
153153
// Drop the old value, then write back, justifying the promotion
154+
// If the drop impl for the old value panics then we run the drop impl for `value` too.
154155
if let Some(drop) = self.drop {
156+
struct OnDrop<F: FnMut()>(F);
157+
impl<F: FnMut()> Drop for OnDrop<F> {
158+
fn drop(&mut self) {
159+
(self.0)();
160+
}
161+
}
162+
let value = value.as_ptr();
163+
let on_unwind = OnDrop(|| (drop)(OwningPtr::new(NonNull::new_unchecked(value))));
164+
155165
(drop)(OwningPtr::new(NonNull::new_unchecked(ptr)));
166+
167+
core::mem::forget(on_unwind);
156168
}
157169
std::ptr::copy_nonoverlapping::<u8>(value.as_ptr(), ptr, self.item_layout.size());
158170
self.len = old_len;
@@ -304,12 +316,16 @@ impl BlobVec {
304316
impl Drop for BlobVec {
305317
fn drop(&mut self) {
306318
self.clear();
319+
if self.item_layout.size() > 0 {
320+
unsafe {
321+
std::alloc::dealloc(self.swap_scratch.as_ptr(), self.item_layout);
322+
}
323+
}
307324
let array_layout =
308325
array_layout(&self.item_layout, self.capacity).expect("array layout should be valid");
309326
if array_layout.size() > 0 {
310327
unsafe {
311328
std::alloc::dealloc(self.get_ptr_mut().as_ptr(), array_layout);
312-
std::alloc::dealloc(self.swap_scratch.as_ptr(), self.item_layout);
313329
}
314330
}
315331
}

crates/bevy_ecs/src/world/mod.rs

+1
Original file line numberDiff line numberDiff line change
@@ -1693,6 +1693,7 @@ mod tests {
16931693
DropLogItem::Create(0),
16941694
DropLogItem::Create(1),
16951695
DropLogItem::Drop(0),
1696+
DropLogItem::Drop(1),
16961697
]
16971698
);
16981699
}

0 commit comments

Comments
 (0)