Drop called multiple times if drop panics.

[mahkoh]

Bevy version

Master

Operating system & version

Arch Linux

What you did

use bevy::prelude::*;

struct Dropper(i32);

impl Drop for Dropper {
    fn drop(&mut self) {
        println!("drop {}", self.0);
        if self.0 == 0 {
            panic!();
        }
    }
}

fn main() {
    App::new()
        .add_startup_system(setup.system())
        .run();
}

fn setup(mut commands: Commands) {
    commands.spawn()
        .insert(Dropper(0))
        .insert(Dropper(1));
}

What you expected to happen

drop 0 is printed at most once.

What actually happened

drop 0 is printed twice.

Additional information

Several functions in https://github.com/bevyengine/bevy/blob/43d99bb583866c1adb4aa88f19b88637df0a7f33/crates/bevy_ecs/src/storage/blob_vec.rs do not handle self.drop unwinding (nor require their callers to do so.)

[bjorn3]

Only replace_unchecked doesn't handle it I think:

bevy/crates/bevy_ecs/src/storage/blob_vec.rs

Lines 101 to 106 in 43d99bb

	pub unsafe fn replace_unchecked(&mut self, index: usize, value: *mut u8) {
	debug_assert!(index < self.len());
	let ptr = self.get_unchecked(index);
	(self.drop)(ptr);
	std::ptr::copy_nonoverlapping(value, ptr, self.item_layout.size());
	}

swap_remove_and_drop_unchecked calls self.drop as last action before returning:

bevy/crates/bevy_ecs/src/storage/blob_vec.rs

Lines 160 to 164 in 43d99bb

	pub unsafe fn swap_remove_and_drop_unchecked(&mut self, index: usize) {
	debug_assert!(index < self.len());
	let value = self.swap_remove_and_forget_unchecked(index);
	(self.drop)(value)
	}

clear sets self.len = 0 first before dropping anything, ensuring a leak in case of a panicking drop:

bevy/crates/bevy_ecs/src/storage/blob_vec.rs

Lines 183 to 196 in 43d99bb

	pub fn clear(&mut self) {
	let len = self.len;
	// We set len to 0 _before_ dropping elements for unwind safety. This ensures we don't
	// accidentally drop elements twice in the event of a drop impl panicking.
	self.len = 0;
	for i in 0..len {
	unsafe {
	// NOTE: this doesn't use self.get_unchecked(i) because the debug_assert on index
	// will panic here due to self.len being set to 0
	let ptr = self.get_ptr().as_ptr().add(i * self.item_layout.size());
	(self.drop)(ptr);
	}
	}
	}

[mahkoh]

swap_remove_and_drop_unchecked calls self.drop as last action before returning:

It depends on what the callers do. In the code path I've looked at, the entity is removed from the world before drop is called. So there is no problem.

Without the additional knowledge that swap_remove_unchecked can panic and leave memory in a half-dropped state, it might also seem valid to swap the relative positions of the linked lines.

[bjorn3]

swap_remove_and_drop_unchecked and clear should be fine. Only replace_unchecked has a bug.

[mahkoh]

Like I said it depends on what the contract between swap_remove_and_drop_unchecked and its callers is. For example, the following program also prints drop 0 twice but replace_unchecked is never called.

use bevy::prelude::*;

struct Dropper(i32);

impl Drop for Dropper {
    fn drop(&mut self) {
        println!("drop {}", self.0);
        if self.0 == 0 {
            panic!();
        }
    }
}

fn main() {
    let mut world = World::new();

    let e1 = world.spawn().insert(Dropper(0)).id();

    std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| {
        world.despawn(e1);
    }));

    world.spawn().insert(Dropper(1)).id();
}

[bjorn3]

That did be a bug in World, not BlobVec I think.