diff --git a/ext/Makefile b/ext/Makefile index 3045d2db0..e96aa52cb 100644 --- a/ext/Makefile +++ b/ext/Makefile @@ -75,5 +75,5 @@ examples/benchmarks/%-concurrent: dummy fi miri: - cargo miri test -p once + MIRIFLAGS="-Zmiri-ignore-leaks" cargo miri test -p once MIRIFLAGS="-Zmiri-ignore-leaks -Zmiri-disable-isolation" cargo miri test -p once --features concurrent diff --git a/ext/crates/once/src/lib.rs b/ext/crates/once/src/lib.rs index 9588239f4..e783a4af2 100644 --- a/ext/crates/once/src/lib.rs +++ b/ext/crates/once/src/lib.rs @@ -169,16 +169,30 @@ impl Drop for OnceVec { fn drop(&mut self) { let len = self.len(); - unsafe { - // The lock may be poisoned. Access is always safe because we have mutable reference, - // but if we can acquire the lock we want to drop the elements inside. If the lock is - // poisoned, then we are probably panicking so we don't care about memory leakage. - if let Ok(ooo) = self.ooo.lock() { - let ooo_iter = ooo.0.iter(); - for entry in ooo_iter { - std::ptr::drop_in_place(self.entry_ptr(*entry)); - } + if std::thread::panicking() { + // If we are panicking, the unsafe deallocation block at the end of this method might + // contain UB due to double-free. I'm (@JoeyBF) not sure why exactly this happens, but + // I'm definitely encountering this UB in some other code I'm writing. It's not the end + // of the world to cause UB when panicking, since we're already in an unrecoverable + // state, but having any kind of UB in the code makes me uneasy. + // + // There's probably more elegant solutions than just leaking all the memory, but that + // does solve the issue. A proper fix would need a solid understanding of the guarantees + // that are upheld while panicking. Again, since we're already panicking, it doesn't + // matter much. + return; + } + + // The lock may be poisoned. Access is always safe because we have mutable reference, but if + // we can acquire the lock we want to drop the elements inside. + if let Ok(ooo) = self.ooo.lock() { + let ooo_iter = ooo.0.iter(); + for entry in ooo_iter { + unsafe { std::ptr::drop_in_place(self.entry_ptr(*entry)) }; } + } + + unsafe { for idx in 0..MAX_OUTER_LENGTH { // We have mutable reference so we can do whatever we want let page = &mut *self.data.as_ptr().add(idx); @@ -904,4 +918,15 @@ mod tests { v.push_ooo(6, 7); drop(v); } + + // This is just so that MIRI can check for UB when we panic + #[test] + #[should_panic] + fn test_drop_panic() { + let v: OnceVec = OnceVec::new(); + v.push(4); + v.push(3); + v.push_ooo(6, 7); + panic!(); + } }