From ba74217c89d3bf52ebe457c8de7f059379409941 Mon Sep 17 00:00:00 2001 From: Nicolas Silva Date: Fri, 19 Jan 2024 18:28:03 +0100 Subject: [PATCH] d3d12: Null check the out ComPtr of a few creation functions (#5096) My understanding is that we shouldn't need to (The d3d12 docs aren't very specific about that), but we have evidence that these functions sometimes leave the resource pointer set to null without returning an error. --- wgpu-hal/src/dx12/descriptor.rs | 7 ++++ wgpu-hal/src/dx12/device.rs | 63 ++++++++++++++++++------------ wgpu-hal/src/dx12/mod.rs | 12 ++++++ wgpu-hal/src/dx12/suballocation.rs | 10 +++++ 4 files changed, 68 insertions(+), 24 deletions(-) diff --git a/wgpu-hal/src/dx12/descriptor.rs b/wgpu-hal/src/dx12/descriptor.rs index 8833c1adf4..2d4d2c1673 100644 --- a/wgpu-hal/src/dx12/descriptor.rs +++ b/wgpu-hal/src/dx12/descriptor.rs @@ -1,3 +1,4 @@ +use super::null_comptr_check; use crate::auxil::dxgi::result::HResult as _; use bit_set::BitSet; use parking_lot::Mutex; @@ -53,6 +54,8 @@ impl GeneralHeap { .into_device_result("Descriptor heap creation")? }; + null_comptr_check(&raw)?; + Ok(Self { raw: raw.clone(), ty, @@ -130,6 +133,8 @@ impl FixedSizeHeap { ) .into_device_result("Descriptor heap creation")?; + null_comptr_check(&heap)?; + Ok(Self { handle_size: device.get_descriptor_increment_size(ty) as _, availability: !0, // all free! @@ -254,6 +259,8 @@ impl CpuHeap { .create_descriptor_heap(total, ty, d3d12::DescriptorHeapFlags::empty(), 0) .into_device_result("CPU descriptor heap creation")?; + null_comptr_check(&raw)?; + Ok(Self { inner: Mutex::new(CpuHeapInner { _raw: raw.clone(), diff --git a/wgpu-hal/src/dx12/device.rs b/wgpu-hal/src/dx12/device.rs index 0c1977203f..bb128b2a6d 100644 --- a/wgpu-hal/src/dx12/device.rs +++ b/wgpu-hal/src/dx12/device.rs @@ -1,9 +1,11 @@ use crate::{ auxil::{self, dxgi::result::HResult as _}, dx12::shader_compilation, + DeviceError, }; +use d3d12::ComPtr; -use super::{conv, descriptor, view}; +use super::{conv, descriptor, null_comptr_check, view}; use parking_lot::Mutex; use std::{ ffi, mem, @@ -29,7 +31,7 @@ impl super::Device { private_caps: super::PrivateCapabilities, library: &Arc, dxc_container: Option>, - ) -> Result { + ) -> Result { let mem_allocator = if private_caps.suballocation_supported { super::suballocation::create_allocator_wrapper(&raw)? } else { @@ -48,6 +50,8 @@ impl super::Device { }; hr.into_device_result("Idle fence creation")?; + null_comptr_check(&idle_fence)?; + let mut zero_buffer = d3d12::Resource::null(); unsafe { let raw_desc = d3d12_ty::D3D12_RESOURCE_DESC { @@ -89,6 +93,8 @@ impl super::Device { ) .into_device_result("Zero buffer creation")?; + null_comptr_check(&zero_buffer)?; + // Note: without `D3D12_HEAP_FLAG_CREATE_NOT_ZEROED` // this resource is zeroed by default. }; @@ -142,7 +148,7 @@ impl super::Device { // A null pResource is used to initialize a null descriptor, // which guarantees D3D11-like null binding behavior (reading 0s, writes are discarded) raw.create_render_target_view( - d3d12::ComPtr::null(), + ComPtr::null(), &d3d12::RenderTargetViewDesc::texture_2d( winapi::shared::dxgiformat::DXGI_FORMAT_R8G8B8A8_UNORM, 0, @@ -185,10 +191,10 @@ impl super::Device { // Blocks until the dedicated present queue is finished with all of its work. // // Once this method completes, the surface is able to be resized or deleted. - pub(super) unsafe fn wait_for_present_queue_idle(&self) -> Result<(), crate::DeviceError> { + pub(super) unsafe fn wait_for_present_queue_idle(&self) -> Result<(), DeviceError> { let cur_value = self.idler.fence.get_value(); if cur_value == !0 { - return Err(crate::DeviceError::Lost); + return Err(DeviceError::Lost); } let value = cur_value + 1; @@ -326,7 +332,7 @@ impl crate::Device for super::Device { unsafe fn create_buffer( &self, desc: &crate::BufferDescriptor, - ) -> Result { + ) -> Result { let mut resource = d3d12::Resource::null(); let mut size = desc.size; if desc.usage.contains(crate::BufferUses::UNIFORM) { @@ -381,11 +387,12 @@ impl crate::Device for super::Device { &self, buffer: &super::Buffer, range: crate::MemoryRange, - ) -> Result { + ) -> Result { let mut ptr = ptr::null_mut(); // TODO: 0 for subresource should be fine here until map and unmap buffer is subresource aware? let hr = unsafe { (*buffer.resource).Map(0, ptr::null(), &mut ptr) }; hr.into_device_result("Map buffer")?; + Ok(crate::BufferMapping { ptr: ptr::NonNull::new(unsafe { ptr.offset(range.start as isize).cast::() }) .unwrap(), @@ -395,7 +402,7 @@ impl crate::Device for super::Device { }) } - unsafe fn unmap_buffer(&self, buffer: &super::Buffer) -> Result<(), crate::DeviceError> { + unsafe fn unmap_buffer(&self, buffer: &super::Buffer) -> Result<(), DeviceError> { unsafe { (*buffer.resource).Unmap(0, ptr::null()) }; Ok(()) } @@ -406,7 +413,7 @@ impl crate::Device for super::Device { unsafe fn create_texture( &self, desc: &crate::TextureDescriptor, - ) -> Result { + ) -> Result { use super::suballocation::create_texture_resource; let mut resource = d3d12::Resource::null(); @@ -465,7 +472,7 @@ impl crate::Device for super::Device { &self, texture: &super::Texture, desc: &crate::TextureViewDescriptor, - ) -> Result { + ) -> Result { let view_desc = desc.to_internal(texture); Ok(super::TextureView { @@ -591,7 +598,7 @@ impl crate::Device for super::Device { unsafe fn create_sampler( &self, desc: &crate::SamplerDescriptor, - ) -> Result { + ) -> Result { let handle = self.sampler_pool.lock().alloc_handle()?; let reduction = match desc.compare { @@ -633,7 +640,7 @@ impl crate::Device for super::Device { unsafe fn create_command_encoder( &self, desc: &crate::CommandEncoderDescriptor, - ) -> Result { + ) -> Result { let allocator = self .raw .create_command_allocator(d3d12::CmdListType::Direct) @@ -665,7 +672,7 @@ impl crate::Device for super::Device { unsafe fn create_bind_group_layout( &self, desc: &crate::BindGroupLayoutDescriptor, - ) -> Result { + ) -> Result { let (mut num_buffer_views, mut num_samplers, mut num_texture_views) = (0, 0, 0); for entry in desc.entries.iter() { let count = entry.count.map_or(1, NonZeroU32::get); @@ -714,7 +721,7 @@ impl crate::Device for super::Device { unsafe fn create_pipeline_layout( &self, desc: &crate::PipelineLayoutDescriptor, - ) -> Result { + ) -> Result { use naga::back::hlsl; // Pipeline layouts are implemented as RootSignature for D3D12. // @@ -1024,7 +1031,7 @@ impl crate::Device for super::Device { ) .map_err(|e| { log::error!("Unable to find serialization function: {:?}", e); - crate::DeviceError::Lost + DeviceError::Lost })? .into_device_result("Root signature serialization")?; @@ -1033,7 +1040,7 @@ impl crate::Device for super::Device { "Root signature serialization error: {:?}", unsafe { error.as_c_str() }.to_str().unwrap() ); - return Err(crate::DeviceError::Lost); + return Err(DeviceError::Lost); } let raw = self @@ -1076,7 +1083,7 @@ impl crate::Device for super::Device { unsafe fn create_bind_group( &self, desc: &crate::BindGroupDescriptor, - ) -> Result { + ) -> Result { let mut cpu_views = desc .layout .cpu_heap_views @@ -1437,6 +1444,8 @@ impl crate::Device for super::Device { hr.into_result() .map_err(|err| crate::PipelineError::Linkage(shader_stages, err.into_owned()))?; + null_comptr_check(&raw)?; + if let Some(name) = desc.label { let cwstr = conv::map_label(name); unsafe { raw.SetName(cwstr.as_ptr()) }; @@ -1474,6 +1483,8 @@ impl crate::Device for super::Device { crate::PipelineError::Linkage(wgt::ShaderStages::COMPUTE, err.into_owned()) })?; + null_comptr_check(&raw)?; + if let Some(name) = desc.label { let cwstr = conv::map_label(name); unsafe { raw.SetName(cwstr.as_ptr()) }; @@ -1489,7 +1500,7 @@ impl crate::Device for super::Device { unsafe fn create_query_set( &self, desc: &wgt::QuerySetDescriptor, - ) -> Result { + ) -> Result { let (heap_ty, raw_ty) = match desc.ty { wgt::QueryType::Occlusion => ( d3d12::QueryHeapType::Occlusion, @@ -1510,6 +1521,8 @@ impl crate::Device for super::Device { .create_query_heap(heap_ty, desc.count, 0) .into_device_result("Query heap creation")?; + null_comptr_check(&raw)?; + if let Some(label) = desc.label { let cwstr = conv::map_label(label); unsafe { raw.SetName(cwstr.as_ptr()) }; @@ -1519,7 +1532,7 @@ impl crate::Device for super::Device { } unsafe fn destroy_query_set(&self, _set: super::QuerySet) {} - unsafe fn create_fence(&self) -> Result { + unsafe fn create_fence(&self) -> Result { let mut raw = d3d12::Fence::null(); let hr = unsafe { self.raw.CreateFence( @@ -1530,13 +1543,15 @@ impl crate::Device for super::Device { ) }; hr.into_device_result("Fence creation")?; + null_comptr_check(&raw)?; + Ok(super::Fence { raw }) } unsafe fn destroy_fence(&self, _fence: super::Fence) {} unsafe fn get_fence_value( &self, fence: &super::Fence, - ) -> Result { + ) -> Result { Ok(unsafe { fence.raw.GetCompletedValue() }) } unsafe fn wait( @@ -1544,7 +1559,7 @@ impl crate::Device for super::Device { fence: &super::Fence, value: crate::FenceValue, timeout_ms: u32, - ) -> Result { + ) -> Result { let timeout_duration = Duration::from_millis(timeout_ms as u64); // We first check if the fence has already reached the value we're waiting for. @@ -1602,7 +1617,7 @@ impl crate::Device for super::Device { winbase::WAIT_OBJECT_0 => {} winbase::WAIT_ABANDONED | winbase::WAIT_FAILED => { log::error!("Wait failed!"); - break Err(crate::DeviceError::Lost); + break Err(DeviceError::Lost); } winerror::WAIT_TIMEOUT => { log::trace!("Wait timed out!"); @@ -1610,7 +1625,7 @@ impl crate::Device for super::Device { } other => { log::error!("Unexpected wait status: 0x{:x}", other); - break Err(crate::DeviceError::Lost); + break Err(DeviceError::Lost); } }; @@ -1664,7 +1679,7 @@ impl crate::Device for super::Device { unsafe fn create_acceleration_structure( &self, _desc: &crate::AccelerationStructureDescriptor, - ) -> Result { + ) -> Result { // Create a D3D12 resource as per-usual. todo!() } diff --git a/wgpu-hal/src/dx12/mod.rs b/wgpu-hal/src/dx12/mod.rs index af8d5a8c01..d56e5efa08 100644 --- a/wgpu-hal/src/dx12/mod.rs +++ b/wgpu-hal/src/dx12/mod.rs @@ -938,3 +938,15 @@ impl crate::Queue for Queue { (1_000_000_000.0 / frequency as f64) as f32 } } + +/// A shorthand for producing a `ResourceCreationFailed` error if a ComPtr is null. +#[inline] +pub fn null_comptr_check( + ptr: &d3d12::ComPtr, +) -> Result<(), crate::DeviceError> { + if d3d12::ComPtr::is_null(ptr) { + return Err(crate::DeviceError::ResourceCreationFailed); + } + + Ok(()) +} diff --git a/wgpu-hal/src/dx12/suballocation.rs b/wgpu-hal/src/dx12/suballocation.rs index 3b9696e455..47a398be53 100644 --- a/wgpu-hal/src/dx12/suballocation.rs +++ b/wgpu-hal/src/dx12/suballocation.rs @@ -16,6 +16,7 @@ use placed as allocation; // This is the fast path using gpu_allocator to suballocate buffers and textures. #[cfg(feature = "windows_rs")] mod placed { + use crate::dx12::null_comptr_check; use d3d12::ComPtr; use parking_lot::Mutex; use std::ptr; @@ -115,6 +116,8 @@ mod placed { ) }; + null_comptr_check(resource)?; + Ok((hr, Some(AllocationWrapper { allocation }))) } @@ -162,6 +165,8 @@ mod placed { ) }; + null_comptr_check(resource)?; + Ok((hr, Some(AllocationWrapper { allocation }))) } @@ -223,6 +228,7 @@ mod placed { // This is the older, slower path where it doesn't suballocate buffers. // Tracking issue for when it can be removed: https://github.com/gfx-rs/wgpu/issues/3207 mod committed { + use crate::dx12::null_comptr_check; use d3d12::ComPtr; use parking_lot::Mutex; use std::ptr; @@ -296,6 +302,8 @@ mod committed { ) }; + null_comptr_check(resource)?; + Ok((hr, None)) } @@ -332,6 +340,8 @@ mod committed { ) }; + null_comptr_check(resource)?; + Ok((hr, None)) }