fix(core): Add missing indirect draw validation for render bundles (gfx-rs#9871)

Author: andyleiserson

CHANGELOG.md

@@ -74,6 +74,10 @@ Bottom level categories:
 
 - Zero-initialize padding (if any) at the end of a buffer allocation. This was application-visible in rare cases on Vulkan when a shader read beyond the valid range of a vertex buffer. By @andyleiserson in [#9791](https://github.com/gfx-rs/wgpu/pull/9791).
 
+#### Validation
+
+- Validate that the arguments are within the indirect buffer when encoding an indirect draw to a render bundle. Moves some indirect draw errors from `RenderPassErrorInner` to `RenderCommandError`. By @andyleiserson in [#9871](https://github.com/gfx-rs/wgpu/pull/9871).
+
 #### GLES
 
 - Fixed signed integer `%` (and `%=`) returning the wrong result for negative operands in the GLSL (OpenGL/GLES) backend, e.g. `-1 % 768` yielding `255` instead of `-1`. GLSL's `%` is undefined when either operand is negative, so signed remainder is now lowered as `a - b * (a / b)`, matching the SPIR-V, HLSL, and Metal backends. By @mstampfli in [#9687](https://github.com/gfx-rs/wgpu/pull/9687).

cts_runner/fail.lst

@@ -32,8 +32,6 @@ webgpu:api,validation,createTexture:new_usages,* // https://github.com/denoland/
 webgpu:api,validation,createView:texture_state:* // 0%, https://github.com/gfx-rs/wgpu/issues/7881
 webgpu:api,validation,encoding,cmds,debug:* // 92%, https://github.com/gfx-rs/wgpu/issues/8039
 webgpu:api,validation,encoding,cmds,render,draw:max_draw_count:* // https://github.com/gfx-rs/wgpu/issues/8737
-webgpu:api,validation,encoding,cmds,render,indirect_draw:indirect_offset_alignment:* // render bundle
-webgpu:api,validation,encoding,cmds,render,indirect_draw:indirect_offset_oob:* // unwrap on None in wgpu-core/src/indirect_validation/draw.rs:432:18
 webgpu:api,validation,encoding,cmds,setBindGroup:dynamic_offsets_match_expectations_in_pass_encoder:* // deno unwrap
 webgpu:api,validation,encoding,cmds,setBindGroup:state_and_binding_index:encoderType="compute%20pass";state="destroyed";* // https://github.com/gfx-rs/wgpu/issues/7881
 webgpu:api,validation,encoding,cmds,setBindGroup:state_and_binding_index:encoderType="render%20bundle";state="destroyed";* // https://github.com/gfx-rs/wgpu/issues/7881

cts_runner/test.lst

@@ -174,9 +174,7 @@ webgpu:api,validation,encoding,cmds,render,draw:index_buffer_OOB:*
 webgpu:api,validation,encoding,cmds,render,draw:unused_buffer_bound:*
 webgpu:api,validation,encoding,cmds,render,draw:vertex_buffer_OOB:*
 webgpu:api,validation,encoding,cmds,render,dynamic_state:*
-webgpu:api,validation,encoding,cmds,render,indirect_draw:indirect_buffer_state:*
-webgpu:api,validation,encoding,cmds,render,indirect_draw:indirect_buffer_usage:*
-webgpu:api,validation,encoding,cmds,render,indirect_draw:indirect_buffer,device_mismatch:*
+webgpu:api,validation,encoding,cmds,render,indirect_draw:*
 webgpu:api,validation,encoding,cmds,render,setIndexBuffer:*
 webgpu:api,validation,encoding,cmds,render,setPipeline:*
 webgpu:api,validation,encoding,cmds,render,setVertexBuffer:*

wgpu-core/src/command/bundle.rs

@@ -1195,11 +1195,23 @@ fn multi_draw_indirect(
     buffer.same_device(&state.device)?;
     buffer.check_usage(wgt::BufferUsages::INDIRECT)?;
 
+    if !offset.is_multiple_of(4) {
+        return Err(RenderCommandError::UnalignedIndirectBufferOffset(offset).into());
+    }
+
     let stride = super::get_src_stride_of_indirect_args(family);
-    // TODO(https://github.com/gfx-rs/wgpu/issues/8051): It would be better to report this
-    // as a validation error, but it's pathological, so let's do the simpler thing for now
-    // and do the better thing as part of eliminating pass/bundle duplication.
-    assert!(offset <= wgt::BufferAddress::MAX - stride);
+    match offset.checked_add(stride) {
+        Some(end_offset) if end_offset <= buffer.size => {}
+        _ => {
+            return Err(RenderCommandError::IndirectBufferOverrun {
+                count: 1,
+                offset,
+                args_size: stride,
+                buffer_size: buffer.size,
+            }
+            .into());
+        }
+    }
     state
         .buffer_memory_init_actions
         .extend(buffer.initialization_status.read().create_action(

wgpu-core/src/command/draw.rs

@@ -128,6 +128,15 @@ pub enum RenderCommandError {
     InvalidViewportDepth(f32, f32),
     #[error("Scissor {0:?} is not contained in the render target {1:?}")]
     InvalidScissorRect(Rect<u32>, wgt::Extent3d),
+    #[error("Indirect buffer offset {0:?} is not a multiple of 4")]
+    UnalignedIndirectBufferOffset(wgt::BufferAddress),
+    #[error("Indirect draw arguments of {args_size} bytes (count = {count}) starting at {offset} would overrun buffer size of {buffer_size}")]
+    IndirectBufferOverrun {
+        count: u32,
+        offset: u64,
+        args_size: u64,
+        buffer_size: u64,
+    },
     #[error("Support for {0} is not implemented yet")]
     Unimplemented(&'static str),
 }
@@ -154,6 +163,8 @@ impl WebGpuError for RenderCommandError {
             | Self::InvalidViewportRectPosition { .. }
             | Self::InvalidViewportDepth(..)
             | Self::InvalidScissorRect(..)
+            | Self::UnalignedIndirectBufferOffset(..)
+            | Self::IndirectBufferOverrun { .. }
             | Self::Unimplemented(..) => ErrorType::Validation,
         }
     }

wgpu-core/src/command/render.rs

@@ -978,15 +978,6 @@ pub enum RenderPassErrorInner {
     MissingFeatures(#[from] MissingFeatures),
     #[error(transparent)]
     MissingDownlevelFlags(#[from] MissingDownlevelFlags),
-    #[error("Indirect buffer offset {0:?} is not a multiple of 4")]
-    UnalignedIndirectBufferOffset(BufferAddress),
-    #[error("Indirect draw arguments of {args_size} bytes (count = {count}) starting at {offset} would overrun buffer size of {buffer_size}")]
-    IndirectBufferOverrun {
-        count: u32,
-        offset: u64,
-        args_size: u64,
-        buffer_size: u64,
-    },
     #[error("Indirect draw count of {count_bytes} bytes starting at {begin_count_offset} would overrun buffer of size {count_buffer_size}")]
     IndirectCountBufferOverrun {
         count_bytes: u64,
@@ -1124,8 +1115,6 @@ impl WebGpuError for RenderPassError {
             | RenderPassErrorInner::MismatchedResolveTextureFormat { .. }
             | RenderPassErrorInner::InvalidDepthOps
             | RenderPassErrorInner::InvalidStencilOps
-            | RenderPassErrorInner::UnalignedIndirectBufferOffset(..)
-            | RenderPassErrorInner::IndirectBufferOverrun { .. }
             | RenderPassErrorInner::IndirectCountBufferOverrun { .. }
             | RenderPassErrorInner::ResourceUsageCompatibility(..)
             | RenderPassErrorInner::IncompatibleBundleReadOnlyDepthStencil { .. }
@@ -3317,19 +3306,20 @@ fn multi_draw_indirect(
     indirect_buffer.check_destroyed(state.pass.base.snatch_guard)?;
 
     if !offset.is_multiple_of(4) {
-        return Err(RenderPassErrorInner::UnalignedIndirectBufferOffset(offset));
+        return Err(RenderCommandError::UnalignedIndirectBufferOffset(offset).into());
     }
 
     let stride = get_src_stride_of_indirect_args(family);
     let args_size = match stride.checked_mul(u64::from(count)) {
         Some(sz) if sz <= indirect_buffer.size && indirect_buffer.size - sz >= offset => sz,
         args_size => {
-            return Err(RenderPassErrorInner::IndirectBufferOverrun {
+            return Err(RenderCommandError::IndirectBufferOverrun {
                 count,
                 offset,
                 args_size: args_size.unwrap_or(u64::MAX),
                 buffer_size: indirect_buffer.size,
-            });
+            }
+            .into());
         }
     };
 
@@ -3537,18 +3527,19 @@ fn multi_draw_indirect_count(
     let count_raw = count_buffer.try_raw(state.pass.base.snatch_guard)?;
 
     if !offset.is_multiple_of(4) {
-        return Err(RenderPassErrorInner::UnalignedIndirectBufferOffset(offset));
+        return Err(RenderCommandError::UnalignedIndirectBufferOffset(offset).into());
     }
 
     let args_size = match stride.checked_mul(u64::from(max_count)) {
         Some(sz) if sz <= indirect_buffer.size && indirect_buffer.size - sz >= offset => sz,
         args_size => {
-            return Err(RenderPassErrorInner::IndirectBufferOverrun {
+            return Err(RenderCommandError::IndirectBufferOverrun {
                 count: 1,
                 offset,
                 args_size: args_size.unwrap_or(u64::MAX),
                 buffer_size: indirect_buffer.size,
-            });
+            }
+            .into());
         }
     };