Skip to content

Commit

Permalink
remove -Zmiri-ignore-leaks from miri CI job (#4959)
Browse files Browse the repository at this point in the history
remove `-Zmiri-ignore-leaks` from CI args and fix leaks that miri detects.

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:
  • Loading branch information
BoxyUwU committed Jun 30, 2022
1 parent 5f8e438 commit 067cc22
Show file tree
Hide file tree
Showing 3 changed files with 19 additions and 4 deletions.
4 changes: 1 addition & 3 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -100,11 +100,9 @@ jobs:
# -Zrandomize-layout makes sure we dont rely on the layout of anything that might change
RUSTFLAGS: -Zrandomize-layout
# -Zmiri-disable-isolation is needed because our executor uses `fastrand` which accesses system time.
# -Zmiri-ignore-leaks is needed because running bevy_ecs tests finds a memory leak but its impossible
# to track down because allocids are nondeterministic.
# -Zmiri-tag-raw-pointers is not strictly "necessary" but enables a lot of extra UB checks relating
# to raw pointer aliasing rules that we should be trying to uphold.
MIRIFLAGS: -Zmiri-disable-isolation -Zmiri-ignore-leaks -Zmiri-tag-raw-pointers
MIRIFLAGS: -Zmiri-disable-isolation -Zmiri-tag-raw-pointers

check-compiles:
runs-on: ubuntu-latest
Expand Down
18 changes: 17 additions & 1 deletion crates/bevy_ecs/src/storage/blob_vec.rs
Original file line number Diff line number Diff line change
Expand Up @@ -151,8 +151,20 @@ impl BlobVec {
let ptr = self.get_unchecked_mut(index).promote().as_ptr();
self.len = 0;
// Drop the old value, then write back, justifying the promotion
// If the drop impl for the old value panics then we run the drop impl for `value` too.
if let Some(drop) = self.drop {
struct OnDrop<F: FnMut()>(F);
impl<F: FnMut()> Drop for OnDrop<F> {
fn drop(&mut self) {
(self.0)();
}
}
let value = value.as_ptr();
let on_unwind = OnDrop(|| (drop)(OwningPtr::new(NonNull::new_unchecked(value))));

(drop)(OwningPtr::new(NonNull::new_unchecked(ptr)));

core::mem::forget(on_unwind);
}
std::ptr::copy_nonoverlapping::<u8>(value.as_ptr(), ptr, self.item_layout.size());
self.len = old_len;
Expand Down Expand Up @@ -304,12 +316,16 @@ impl BlobVec {
impl Drop for BlobVec {
fn drop(&mut self) {
self.clear();
if self.item_layout.size() > 0 {
unsafe {
std::alloc::dealloc(self.swap_scratch.as_ptr(), self.item_layout);
}
}
let array_layout =
array_layout(&self.item_layout, self.capacity).expect("array layout should be valid");
if array_layout.size() > 0 {
unsafe {
std::alloc::dealloc(self.get_ptr_mut().as_ptr(), array_layout);
std::alloc::dealloc(self.swap_scratch.as_ptr(), self.item_layout);
}
}
}
Expand Down
1 change: 1 addition & 0 deletions crates/bevy_ecs/src/world/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1666,6 +1666,7 @@ mod tests {
DropLogItem::Create(0),
DropLogItem::Create(1),
DropLogItem::Drop(0),
DropLogItem::Drop(1),
]
);
}
Expand Down

0 comments on commit 067cc22

Please sign in to comment.