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

@@ -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(),

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<d3d12::D3D12Lib>,
         dxc_container: Option<Arc<shader_compilation::DxcContainer>>,
-    ) -> Result<Self, crate::DeviceError> {
+    ) -> Result<Self, DeviceError> {
         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<super::Api> for super::Device {
     unsafe fn create_buffer(
         &self,
         desc: &crate::BufferDescriptor,
-    ) -> Result<super::Buffer, crate::DeviceError> {
+    ) -> Result<super::Buffer, DeviceError> {
         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<super::Api> for super::Device {
         &self,
         buffer: &super::Buffer,
         range: crate::MemoryRange,
-    ) -> Result<crate::BufferMapping, crate::DeviceError> {
+    ) -> Result<crate::BufferMapping, DeviceError> {
         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::<u8>() })
                 .unwrap(),
@@ -395,7 +402,7 @@ impl crate::Device<super::Api> 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<super::Api> for super::Device {
     unsafe fn create_texture(
         &self,
         desc: &crate::TextureDescriptor,
-    ) -> Result<super::Texture, crate::DeviceError> {
+    ) -> Result<super::Texture, DeviceError> {
         use super::suballocation::create_texture_resource;
 
         let mut resource = d3d12::Resource::null();
@@ -465,7 +472,7 @@ impl crate::Device<super::Api> for super::Device {
         &self,
         texture: &super::Texture,
         desc: &crate::TextureViewDescriptor,
-    ) -> Result<super::TextureView, crate::DeviceError> {
+    ) -> Result<super::TextureView, DeviceError> {
         let view_desc = desc.to_internal(texture);
 
         Ok(super::TextureView {
@@ -591,7 +598,7 @@ impl crate::Device<super::Api> for super::Device {
     unsafe fn create_sampler(
         &self,
         desc: &crate::SamplerDescriptor,
-    ) -> Result<super::Sampler, crate::DeviceError> {
+    ) -> Result<super::Sampler, DeviceError> {
         let handle = self.sampler_pool.lock().alloc_handle()?;
 
         let reduction = match desc.compare {
@@ -633,7 +640,7 @@ impl crate::Device<super::Api> for super::Device {
     unsafe fn create_command_encoder(
         &self,
         desc: &crate::CommandEncoderDescriptor<super::Api>,
-    ) -> Result<super::CommandEncoder, crate::DeviceError> {
+    ) -> Result<super::CommandEncoder, DeviceError> {
         let allocator = self
             .raw
             .create_command_allocator(d3d12::CmdListType::Direct)
@@ -665,7 +672,7 @@ impl crate::Device<super::Api> for super::Device {
     unsafe fn create_bind_group_layout(
         &self,
         desc: &crate::BindGroupLayoutDescriptor,
-    ) -> Result<super::BindGroupLayout, crate::DeviceError> {
+    ) -> Result<super::BindGroupLayout, DeviceError> {
         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<super::Api> for super::Device {
     unsafe fn create_pipeline_layout(
         &self,
         desc: &crate::PipelineLayoutDescriptor<super::Api>,
-    ) -> Result<super::PipelineLayout, crate::DeviceError> {
+    ) -> Result<super::PipelineLayout, DeviceError> {
         use naga::back::hlsl;
         // Pipeline layouts are implemented as RootSignature for D3D12.
         //
@@ -1024,7 +1031,7 @@ impl crate::Device<super::Api> 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<super::Api> 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<super::Api> for super::Device {
     unsafe fn create_bind_group(
         &self,
         desc: &crate::BindGroupDescriptor<super::Api>,
-    ) -> Result<super::BindGroup, crate::DeviceError> {
+    ) -> Result<super::BindGroup, DeviceError> {
         let mut cpu_views = desc
             .layout
             .cpu_heap_views
@@ -1437,6 +1444,8 @@ impl crate::Device<super::Api> 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<super::Api> 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<super::Api> for super::Device {
     unsafe fn create_query_set(
         &self,
         desc: &wgt::QuerySetDescriptor<crate::Label>,
-    ) -> Result<super::QuerySet, crate::DeviceError> {
+    ) -> Result<super::QuerySet, DeviceError> {
         let (heap_ty, raw_ty) = match desc.ty {
             wgt::QueryType::Occlusion => (
                 d3d12::QueryHeapType::Occlusion,
@@ -1510,6 +1521,8 @@ impl crate::Device<super::Api> 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<super::Api> for super::Device {
     }
     unsafe fn destroy_query_set(&self, _set: super::QuerySet) {}
 
-    unsafe fn create_fence(&self) -> Result<super::Fence, crate::DeviceError> {
+    unsafe fn create_fence(&self) -> Result<super::Fence, DeviceError> {
         let mut raw = d3d12::Fence::null();
         let hr = unsafe {
             self.raw.CreateFence(
@@ -1530,21 +1543,23 @@ impl crate::Device<super::Api> 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<crate::FenceValue, crate::DeviceError> {
+    ) -> Result<crate::FenceValue, DeviceError> {
         Ok(unsafe { fence.raw.GetCompletedValue() })
     }
     unsafe fn wait(
         &self,
         fence: &super::Fence,
         value: crate::FenceValue,
         timeout_ms: u32,
-    ) -> Result<bool, crate::DeviceError> {
+    ) -> Result<bool, DeviceError> {
         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,15 +1617,15 @@ impl crate::Device<super::Api> 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!");
                     break Ok(false);
                 }
                 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<super::Api> for super::Device {
     unsafe fn create_acceleration_structure(
         &self,
         _desc: &crate::AccelerationStructureDescriptor,
-    ) -> Result<super::AccelerationStructure, crate::DeviceError> {
+    ) -> Result<super::AccelerationStructure, DeviceError> {
         // Create a D3D12 resource as per-usual.
         todo!()
     }

wgpu-hal/src/dx12/mod.rs

@@ -938,3 +938,15 @@ impl crate::Queue<Api> 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<T: winapi::Interface>(
+    ptr: &d3d12::ComPtr<T>,
+) -> Result<(), crate::DeviceError> {
+    if d3d12::ComPtr::is_null(ptr) {
+        return Err(crate::DeviceError::ResourceCreationFailed);
+    }
+
+    Ok(())
+}

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))
     }