From 727607741db6826a458ab1274b3951e36e6fa391 Mon Sep 17 00:00:00 2001 From: marc0246 <40955683+marc0246@users.noreply.github.com> Date: Sat, 25 Mar 2023 08:52:35 +0100 Subject: [PATCH 1/3] Fix potential panics during panics --- vulkano/src/buffer/subbuffer.rs | 5 +++++ vulkano/src/command_buffer/allocator.rs | 7 +++++++ vulkano/src/command_buffer/traits.rs | 5 +++++ vulkano/src/descriptor_set/allocator.rs | 7 +++++-- vulkano/src/swapchain/swapchain.rs | 11 ++++++++++- vulkano/src/sync/future/fence_signal.rs | 5 +++++ vulkano/src/sync/future/semaphore_signal.rs | 5 +++++ 7 files changed, 42 insertions(+), 3 deletions(-) diff --git a/vulkano/src/buffer/subbuffer.rs b/vulkano/src/buffer/subbuffer.rs index 7d4c5fb60c..3cb2fd8c70 100644 --- a/vulkano/src/buffer/subbuffer.rs +++ b/vulkano/src/buffer/subbuffer.rs @@ -33,6 +33,7 @@ use std::{ ops::{Deref, DerefMut, Range, RangeBounds}, ptr::{self, NonNull}, sync::Arc, + thread, }; pub use vulkano_macros::BufferContents; @@ -632,6 +633,10 @@ pub struct BufferWriteGuard<'a, T: ?Sized> { impl Drop for BufferWriteGuard<'_, T> { fn drop(&mut self) { + if thread::panicking() { + return; + } + let allocation = match self.subbuffer.buffer().memory() { BufferMemory::Normal(a) => a, BufferMemory::Sparse => unreachable!(), diff --git a/vulkano/src/command_buffer/allocator.rs b/vulkano/src/command_buffer/allocator.rs index 8a49bf102c..1ee420a65a 100644 --- a/vulkano/src/command_buffer/allocator.rs +++ b/vulkano/src/command_buffer/allocator.rs @@ -33,6 +33,7 @@ use std::{ marker::PhantomData, mem::ManuallyDrop, sync::Arc, + thread, }; use thread_local::ThreadLocal; @@ -497,6 +498,10 @@ impl Pool { impl Drop for Pool { fn drop(&mut self) { + if thread::panicking() { + return; + } + let inner = unsafe { ManuallyDrop::take(&mut self.inner) }; unsafe { inner.inner.reset(false) }.unwrap(); inner.primary_allocations.set(0); @@ -623,6 +628,8 @@ impl Drop for StandardCommandBufferAlloc { CommandBufferLevel::Primary => &self.pool.inner.primary_pool, CommandBufferLevel::Secondary => &self.pool.inner.secondary_pool, }; + // This can't panic, because if an allocation from a particular kind of pool was made, then + // the pool must exist. let _ = pool.as_ref().unwrap().push(inner); } } diff --git a/vulkano/src/command_buffer/traits.rs b/vulkano/src/command_buffer/traits.rs index d585f3331f..25af867561 100644 --- a/vulkano/src/command_buffer/traits.rs +++ b/vulkano/src/command_buffer/traits.rs @@ -34,6 +34,7 @@ use std::{ atomic::{AtomicBool, Ordering}, Arc, }, + thread, }; pub unsafe trait PrimaryCommandBufferAbstract: @@ -441,6 +442,10 @@ where F: GpuFuture, { fn drop(&mut self) { + if thread::panicking() { + return; + } + unsafe { if !*self.finished.get_mut() { // TODO: handle errors? diff --git a/vulkano/src/descriptor_set/allocator.rs b/vulkano/src/descriptor_set/allocator.rs index 18ac003339..ed0c0b1720 100644 --- a/vulkano/src/descriptor_set/allocator.rs +++ b/vulkano/src/descriptor_set/allocator.rs @@ -30,7 +30,7 @@ use crate::{ OomError, }; use crossbeam_queue::ArrayQueue; -use std::{cell::UnsafeCell, mem::ManuallyDrop, num::NonZeroU64, sync::Arc}; +use std::{cell::UnsafeCell, mem::ManuallyDrop, num::NonZeroU64, sync::Arc, thread}; use thread_local::ThreadLocal; const MAX_POOLS: usize = 32; @@ -416,8 +416,11 @@ impl VariablePool { impl Drop for VariablePool { fn drop(&mut self) { + if thread::panicking() { + return; + } + let inner = unsafe { ManuallyDrop::take(&mut self.inner) }; - // TODO: This should not return `Result`, resetting a pool can't fail. unsafe { inner.reset() }.unwrap(); // If there is not enough space in the reserve, we destroy the pool. The only way this can diff --git a/vulkano/src/swapchain/swapchain.rs b/vulkano/src/swapchain/swapchain.rs index ed67718b69..400b2b9d8f 100644 --- a/vulkano/src/swapchain/swapchain.rs +++ b/vulkano/src/swapchain/swapchain.rs @@ -42,6 +42,7 @@ use std::{ atomic::{AtomicBool, AtomicU64, Ordering}, Arc, }, + thread, time::Duration, }; @@ -1743,7 +1744,11 @@ unsafe impl GpuFuture for SwapchainAcquireFuture { impl Drop for SwapchainAcquireFuture { fn drop(&mut self) { - if let Some(ref fence) = self.fence { + if thread::panicking() { + return; + } + + if let Some(fence) = &self.fence { fence.wait(None).unwrap(); // TODO: handle error? self.semaphore = None; } @@ -2184,6 +2189,10 @@ where P: GpuFuture, { fn drop(&mut self) { + if thread::panicking() { + return; + } + unsafe { if !*self.flushed.get_mut() { // Flushing may fail, that's okay. We will still wait for the queue later, so any diff --git a/vulkano/src/sync/future/fence_signal.rs b/vulkano/src/sync/future/fence_signal.rs index 9e78be78b7..7f71d884b2 100644 --- a/vulkano/src/sync/future/fence_signal.rs +++ b/vulkano/src/sync/future/fence_signal.rs @@ -29,6 +29,7 @@ use std::{ pin::Pin, sync::Arc, task::{Context, Poll}, + thread, time::Duration, }; @@ -523,6 +524,10 @@ where F: GpuFuture, { fn drop(&mut self) { + if thread::panicking() { + return; + } + let mut state = self.state.lock(); // We ignore any possible error while submitting for now. Problems are handled below. diff --git a/vulkano/src/sync/future/semaphore_signal.rs b/vulkano/src/sync/future/semaphore_signal.rs index 5293b37236..c849df3802 100644 --- a/vulkano/src/sync/future/semaphore_signal.rs +++ b/vulkano/src/sync/future/semaphore_signal.rs @@ -25,6 +25,7 @@ use std::{ atomic::{AtomicBool, Ordering}, Arc, }, + thread, }; /// Builds a new semaphore signal future. @@ -250,6 +251,10 @@ where F: GpuFuture, { fn drop(&mut self) { + if thread::panicking() { + return; + } + unsafe { if !*self.finished.get_mut() { // TODO: handle errors? From 2adec7c16a7258e1f3211b50e65874bfe5f30607 Mon Sep 17 00:00:00 2001 From: marc0246 <40955683+marc0246@users.noreply.github.com> Date: Sat, 25 Mar 2023 11:00:56 +0100 Subject: [PATCH 2/3] Try to leak less --- vulkano/src/buffer/subbuffer.rs | 6 +----- vulkano/src/command_buffer/allocator.rs | 3 ++- vulkano/src/descriptor_set/allocator.rs | 3 ++- 3 files changed, 5 insertions(+), 7 deletions(-) diff --git a/vulkano/src/buffer/subbuffer.rs b/vulkano/src/buffer/subbuffer.rs index 3cb2fd8c70..b09784a09a 100644 --- a/vulkano/src/buffer/subbuffer.rs +++ b/vulkano/src/buffer/subbuffer.rs @@ -633,16 +633,12 @@ pub struct BufferWriteGuard<'a, T: ?Sized> { impl Drop for BufferWriteGuard<'_, T> { fn drop(&mut self) { - if thread::panicking() { - return; - } - let allocation = match self.subbuffer.buffer().memory() { BufferMemory::Normal(a) => a, BufferMemory::Sparse => unreachable!(), }; - if allocation.atom_size().is_some() { + if allocation.atom_size().is_some() && !thread::panicking() { unsafe { allocation.flush_range(self.range.clone()).unwrap() }; } diff --git a/vulkano/src/command_buffer/allocator.rs b/vulkano/src/command_buffer/allocator.rs index 1ee420a65a..a2ca514690 100644 --- a/vulkano/src/command_buffer/allocator.rs +++ b/vulkano/src/command_buffer/allocator.rs @@ -498,11 +498,12 @@ impl Pool { impl Drop for Pool { fn drop(&mut self) { + let inner = unsafe { ManuallyDrop::take(&mut self.inner) }; + if thread::panicking() { return; } - let inner = unsafe { ManuallyDrop::take(&mut self.inner) }; unsafe { inner.inner.reset(false) }.unwrap(); inner.primary_allocations.set(0); inner.secondary_allocations.set(0); diff --git a/vulkano/src/descriptor_set/allocator.rs b/vulkano/src/descriptor_set/allocator.rs index ed0c0b1720..954db3fbed 100644 --- a/vulkano/src/descriptor_set/allocator.rs +++ b/vulkano/src/descriptor_set/allocator.rs @@ -416,11 +416,12 @@ impl VariablePool { impl Drop for VariablePool { fn drop(&mut self) { + let inner = unsafe { ManuallyDrop::take(&mut self.inner) }; + if thread::panicking() { return; } - let inner = unsafe { ManuallyDrop::take(&mut self.inner) }; unsafe { inner.reset() }.unwrap(); // If there is not enough space in the reserve, we destroy the pool. The only way this can From f8cee2a0c7f8b77a650f564ab1d711363faa207c Mon Sep 17 00:00:00 2001 From: marc0246 <40955683+marc0246@users.noreply.github.com> Date: Sat, 25 Mar 2023 12:31:39 +0100 Subject: [PATCH 3/3] Cosmetics --- vulkano/src/command_buffer/traits.rs | 18 ++++++------------ vulkano/src/sync/future/semaphore_signal.rs | 18 ++++++------------ 2 files changed, 12 insertions(+), 24 deletions(-) diff --git a/vulkano/src/command_buffer/traits.rs b/vulkano/src/command_buffer/traits.rs index 25af867561..80469d8b9b 100644 --- a/vulkano/src/command_buffer/traits.rs +++ b/vulkano/src/command_buffer/traits.rs @@ -442,18 +442,12 @@ where F: GpuFuture, { fn drop(&mut self) { - if thread::panicking() { - return; - } - - unsafe { - if !*self.finished.get_mut() { - // TODO: handle errors? - self.flush().unwrap(); - // Block until the queue finished. - self.queue.with(|mut q| q.wait_idle()).unwrap(); - self.previous.signal_finished(); - } + if !*self.finished.get_mut() && !thread::panicking() { + // TODO: handle errors? + self.flush().unwrap(); + // Block until the queue finished. + self.queue.with(|mut q| q.wait_idle()).unwrap(); + unsafe { self.previous.signal_finished() }; } } } diff --git a/vulkano/src/sync/future/semaphore_signal.rs b/vulkano/src/sync/future/semaphore_signal.rs index c849df3802..f31d37ac5a 100644 --- a/vulkano/src/sync/future/semaphore_signal.rs +++ b/vulkano/src/sync/future/semaphore_signal.rs @@ -251,18 +251,12 @@ where F: GpuFuture, { fn drop(&mut self) { - if thread::panicking() { - return; - } - - unsafe { - if !*self.finished.get_mut() { - // TODO: handle errors? - self.flush().unwrap(); - // Block until the queue finished. - self.queue().unwrap().with(|mut q| q.wait_idle()).unwrap(); - self.previous.signal_finished(); - } + if !*self.finished.get_mut() && !thread::panicking() { + // TODO: handle errors? + self.flush().unwrap(); + // Block until the queue finished. + self.queue().unwrap().with(|mut q| q.wait_idle()).unwrap(); + unsafe { self.previous.signal_finished() }; } } }