Advance list.first before freeing the chunk in cleanup_impl

src/event_queue.rs

@@ -350,8 +350,12 @@ impl<T, S: Settings> EventQueue<T, S>
                     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::<true>(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::<true>(chunk, list);
 
                     Continue(())
                 }

src/event_queue/test.rs

@@ -247,4 +247,54 @@ fn CleanupMode_Never_test(){
 
     event.cleanup();
     assert_equal(get_chunks_capacities(&event), [4]);
-}
+}
+
+#[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::<PanicOnDrop, S>::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);
+}