diff --git a/src/event_queue.rs b/src/event_queue.rs index b8b0873..a10ccf2 100644 --- a/src/event_queue.rs +++ b/src/event_queue.rs @@ -350,8 +350,12 @@ impl EventQueue debug_assert!(std::ptr::eq(chunk, list.first)); // Do not lock start_position permanently, because reader will // never enter chunk before list.first - self.free_chunk::(chunk, list); + // Retire the chunk before freeing it. `free_chunk` runs each + // element's `Drop`, which may unwind; if `list.first` were + // advanced afterwards it would still point at the freed chunk, + // and `Drop for EventQueue` would walk the list from there. list.first = next_chunk_ptr; + self.free_chunk::(chunk, list); Continue(()) } diff --git a/src/event_queue/test.rs b/src/event_queue/test.rs index 434978e..6734dcb 100644 --- a/src/event_queue/test.rs +++ b/src/event_queue/test.rs @@ -247,4 +247,54 @@ fn CleanupMode_Never_test(){ event.cleanup(); assert_equal(get_chunks_capacities(&event), [4]); -} \ No newline at end of file +} + +#[test] +fn cleanup_unwind_does_not_double_drop_test(){ + use std::panic::{catch_unwind, AssertUnwindSafe}; + use crate::sync::{AtomicUsize, AtomicBool}; + + struct S{} impl Settings for S{ + const MIN_CHUNK_SIZE: u32 = 4; + const MAX_CHUNK_SIZE: u32 = 4; + const CLEANUP: CleanupMode = CleanupMode::Never; + } + + static DROPS: AtomicUsize = AtomicUsize::new(0); + static PANICKED: AtomicBool = AtomicBool::new(false); + + struct PanicOnDrop{ id: usize } + impl Drop for PanicOnDrop{ + fn drop(&mut self) { + DROPS.fetch_add(1, Ordering::Relaxed); + if self.id == 0 && !PANICKED.swap(true, Ordering::Relaxed) { + panic!("drop panicked on purpose"); + } + } + } + + let event = EventQueue::::new(); + let mut reader = EventReader::new(&event); + + for id in 0..12 { + event.push(PanicOnDrop{ id }); + } + skip(&mut reader.iter(), 12); + + // cleanup_impl frees a chunk and only then advances list.first. When an + // element's Drop unwinds, list.first still points at the freed chunk, and + // Drop for EventQueue walks the list from there and frees it again. + let _ = catch_unwind(AssertUnwindSafe(||{ + event.cleanup(); + })); + + drop(reader); + drop(event); + + // Before the fix `list.first` still pointed at the freed chunk, so + // `Drop for EventQueue` freed it again and the count came out above 12. + // After the fix the elements left in the chunk that panicked are leaked, + // which is safe. + let drops = DROPS.load(Ordering::Relaxed); + assert!(drops <= 12, "{} drops for 12 elements", drops); +}