Fix the validation of vertex/index/instance ranges in render bundles

wgpu-core/src/command/bundle.rs

@@ -96,7 +96,7 @@ use crate::{
     hub::Hub,
     id::{self, RenderBundleId},
     init_tracker::{BufferInitTrackerAction, MemoryInitKind, TextureInitTrackerAction},
-    pipeline::{self, PipelineFlags, RenderPipeline},
+    pipeline::{PipelineFlags, RenderPipeline, VertexStep},
     resource::{Resource, ResourceInfo, ResourceType},
     resource_log,
     track::RenderBundleScope,
@@ -110,6 +110,91 @@ use thiserror::Error;
 
 use hal::CommandEncoder as _;
 
+/// https://gpuweb.github.io/gpuweb/#dom-gpurendercommandsmixin-draw
+fn validate_draw(
+    vertex: &[Option<VertexState>],
+    step: &[VertexStep],
+    first_vertex: u32,
+    vertex_count: u32,
+    first_instance: u32,
+    instance_count: u32,
+) -> Result<(), DrawError> {
+    let vertices_end = first_vertex as u64 + vertex_count as u64;
+    let instances_end = first_instance as u64 + instance_count as u64;
+
+    for (idx, (vbs, step)) in vertex.iter().zip(step).enumerate() {
+        let Some(vbs) = vbs else {
+            continue;
+        };
+
+        let stride_count = match step.mode {
+            wgt::VertexStepMode::Vertex => vertices_end,
+            wgt::VertexStepMode::Instance => instances_end,
+        };
+
+        if stride_count == 0 {
+            continue;
+        }
+
+        let offset = (stride_count - 1) * step.stride + step.last_stride;
+        let limit = vbs.range.end - vbs.range.start;
+        if offset > limit {
+            return Err(DrawError::VertexOutOfBounds {
+                step_mode: step.mode,
+                offset,
+                limit,
+                slot: idx as u32,
+            });
+        }
+    }
+
+    Ok(())
+}
+
+// See https://gpuweb.github.io/gpuweb/#dom-gpurendercommandsmixin-drawindexed
+fn validate_indexed_draw(
+    vertex: &[Option<VertexState>],
+    step: &[VertexStep],
+    index_state: &IndexState,
+    first_index: u32,
+    index_count: u32,
+    first_instance: u32,
+    instance_count: u32,
+) -> Result<(), DrawError> {
+    let last_index = first_index as u64 + index_count as u64;
+    let index_limit = index_state.limit();
+    if last_index <= index_limit {
+        return Err(DrawError::IndexBeyondLimit {
+            last_index,
+            index_limit,
+        });
+    }
+
+    let stride_count = first_instance as u64 + instance_count as u64;
+    for (idx, (vbs, step)) in vertex.iter().zip(step).enumerate() {
+        let Some(vbs) = vbs else {
+            continue;
+        };
+
+        if stride_count == 0 || step.mode != wgt::VertexStepMode::Instance {
+            continue;
+        }
+
+        let offset = (stride_count - 1) * step.stride + step.last_stride;
+        let limit = vbs.range.end - vbs.range.start;
+        if offset > limit {
+            return Err(DrawError::VertexOutOfBounds {
+                step_mode: step.mode,
+                offset,
+                limit,
+                slot: idx as u32,
+            });
+        }
+    }
+
+    Ok(())
+}
+
 /// Describes a [`RenderBundleEncoder`].
 #[derive(Clone, Debug, Default, PartialEq, Eq, Hash)]
 #[cfg_attr(feature = "trace", derive(serde::Serialize))]
@@ -495,25 +580,15 @@ impl RenderBundleEncoder {
                     };
                     let pipeline = state.pipeline(scope)?;
                     let used_bind_groups = pipeline.used_bind_groups;
-                    let vertex_limits = state.vertex_limits(pipeline);
-                    let last_vertex = first_vertex as u64 + vertex_count as u64;
-                    if last_vertex > vertex_limits.vertex_limit {
-                        return Err(DrawError::VertexBeyondLimit {
-                            last_vertex,
-                            vertex_limit: vertex_limits.vertex_limit,
-                            slot: vertex_limits.vertex_limit_slot,
-                        })
-                        .map_pass_err(scope);
-                    }
-                    let last_instance = first_instance as u64 + instance_count as u64;
-                    if last_instance > vertex_limits.instance_limit {
-                        return Err(DrawError::InstanceBeyondLimit {
-                            last_instance,
-                            instance_limit: vertex_limits.instance_limit,
-                            slot: vertex_limits.instance_limit_slot,
-                        })
-                        .map_pass_err(scope);
-                    }
+
+                    validate_draw(
+                        &state.vertex[..],
+                        &pipeline.steps,
+                        first_vertex,
+                        vertex_count,
+                        first_instance,
+                        instance_count,
+                    ).map_pass_err(scope)?;
 
                     if instance_count > 0 && vertex_count > 0 {
                         commands.extend(state.flush_vertices());
@@ -539,26 +614,16 @@ impl RenderBundleEncoder {
                         Some(ref index) => index,
                         None => return Err(DrawError::MissingIndexBuffer).map_pass_err(scope),
                     };
-                    //TODO: validate that base_vertex + max_index() is within the provided range
-                    let vertex_limits = state.vertex_limits(pipeline);
-                    let index_limit = index.limit();
-                    let last_index = first_index as u64 + index_count as u64;
-                    if last_index > index_limit {
-                        return Err(DrawError::IndexBeyondLimit {
-                            last_index,
-                            index_limit,
-                        })
-                        .map_pass_err(scope);
-                    }
-                    let last_instance = first_instance as u64 + instance_count as u64;
-                    if last_instance > vertex_limits.instance_limit {
-                        return Err(DrawError::InstanceBeyondLimit {
-                            last_instance,
-                            instance_limit: vertex_limits.instance_limit,
-                            slot: vertex_limits.instance_limit_slot,
-                        })
-                        .map_pass_err(scope);
-                    }
+
+                    validate_indexed_draw(
+                        &state.vertex[..],
+                        &pipeline.steps,
+                        index,
+                        first_index,
+                        index_count,
+                        first_instance,
+                        instance_count,
+                    ).map_pass_err(scope)?;
 
                     if instance_count > 0 && index_count > 0 {
                         commands.extend(state.flush_index());
@@ -1119,26 +1184,14 @@ struct BindState<A: HalApi> {
     is_dirty: bool,
 }
 
-#[derive(Debug)]
-struct VertexLimitState {
-    /// Length of the shortest vertex rate vertex buffer
-    vertex_limit: u64,
-    /// Buffer slot which the shortest vertex rate vertex buffer is bound to
-    vertex_limit_slot: u32,
-    /// Length of the shortest instance rate vertex buffer
-    instance_limit: u64,
-    /// Buffer slot which the shortest instance rate vertex buffer is bound to
-    instance_limit_slot: u32,
-}
-
 /// The bundle's current pipeline, and some cached information needed for validation.
 struct PipelineState<A: HalApi> {
     /// The pipeline
     pipeline: Arc<RenderPipeline<A>>,
 
     /// How this pipeline's vertex shader traverses each vertex buffer, indexed
     /// by vertex buffer slot number.
-    steps: Vec<pipeline::VertexStep>,
+    steps: Vec<VertexStep>,
 
     /// Ranges of push constants this pipeline uses, copied from the pipeline
     /// layout.
@@ -1223,35 +1276,6 @@ struct State<A: HalApi> {
 }
 
 impl<A: HalApi> State<A> {
-    fn vertex_limits(&self, pipeline: &PipelineState<A>) -> VertexLimitState {
-        let mut vert_state = VertexLimitState {
-            vertex_limit: u32::MAX as u64,
-            vertex_limit_slot: 0,
-            instance_limit: u32::MAX as u64,
-            instance_limit_slot: 0,
-        };
-        for (idx, (vbs, step)) in self.vertex.iter().zip(&pipeline.steps).enumerate() {
-            if let Some(ref vbs) = *vbs {
-                let limit = (vbs.range.end - vbs.range.start) / step.stride;
-                match step.mode {
-                    wgt::VertexStepMode::Vertex => {
-                        if limit < vert_state.vertex_limit {
-                            vert_state.vertex_limit = limit;
-                            vert_state.vertex_limit_slot = idx as _;
-                        }
-                    }
-                    wgt::VertexStepMode::Instance => {
-                        if limit < vert_state.instance_limit {
-                            vert_state.instance_limit = limit;
-                            vert_state.instance_limit_slot = idx as _;
-                        }
-                    }
-                }
-            }
-        }
-        vert_state
-    }
-
     /// Return the id of the current pipeline, if any.
     fn pipeline_id(&self) -> Option<id::RenderPipelineId> {
         self.pipeline.as_ref().map(|p| p.pipeline.as_info().id())

wgpu-core/src/command/draw.rs

@@ -8,7 +8,7 @@ use crate::{
     track::UsageConflict,
     validation::{MissingBufferUsageError, MissingTextureUsageError},
 };
-use wgt::{BufferAddress, BufferSize, Color};
+use wgt::{BufferAddress, BufferSize, Color, VertexStepMode};
 
 use std::num::NonZeroU32;
 use thiserror::Error;
@@ -33,6 +33,13 @@ pub enum DrawError {
         vertex_limit: u64,
         slot: u32,
     },
+    #[error("{step_mode:?} buffer out of bounds at slot {slot}. Offset {offset} beyond limit {limit}. Did you bind the correct `Vertex` step-rate vertex buffer?")]
+    VertexOutOfBounds {
+        step_mode: VertexStepMode,
+        offset: u64,
+        limit: u64,
+        slot: u32,
+    },
     #[error("Instance {last_instance} extends beyond limit {instance_limit} imposed by the buffer in slot {slot}. Did you bind the correct `Instance` step-rate vertex buffer?")]
     InstanceBeyondLimit {
         last_instance: u64,

wgpu-core/src/command/render.rs

@@ -361,6 +361,7 @@ impl VertexBufferState {
         total_size: 0,
         step: pipeline::VertexStep {
             stride: 0,
+            last_stride: 0,
             mode: VertexStepMode::Vertex,
         },
         bound: false,
@@ -384,11 +385,13 @@ struct VertexState {
 
 impl VertexState {
     fn update_limits(&mut self) {
-        // Ensure that the limits are always smaller than u32::MAX so that
-        // interger overlows can be prevented via saturating additions.
-        let max = u32::MAX as u64;
-        self.vertex_limit = max;
-        self.instance_limit = max;
+        // TODO: This isn't entirely spec-compliant.
+        // We currently require that the buffer range can fit `stride` * count bytes.
+        // The spec, however, lets a buffer be a bit smaller as long as the size of the
+        // last element fits in it (the last element can be smaller than the stride between
+        // elements).
+        self.vertex_limit = u32::MAX as u64;
+        self.instance_limit = u32::MAX as u64;
         for (idx, vbs) in self.inputs.iter().enumerate() {
             if vbs.step.stride == 0 || !vbs.bound {
                 continue;

wgpu-core/src/device/resource.rs

@@ -2717,8 +2717,13 @@ impl<A: HalApi> Device<A> {
         let mut shader_expects_dual_source_blending = false;
         let mut pipeline_expects_dual_source_blending = false;
         for (i, vb_state) in desc.vertex.buffers.iter().enumerate() {
+            let mut last_stride = 0;
+            for attribute in vb_state.attributes.iter() {
+                last_stride = last_stride.max(attribute.offset + attribute.format.size());
+            }
             vertex_steps.push(pipeline::VertexStep {
                 stride: vb_state.array_stride,
+                last_stride,
                 mode: vb_state.step_mode,
             });
             if vb_state.attributes.is_empty() {

wgpu-core/src/pipeline.rs

@@ -493,6 +493,9 @@ pub struct VertexStep {
     /// The byte stride in the buffer between one attribute value and the next.
     pub stride: wgt::BufferAddress,
 
+    /// The byte size required to fit the last vertex in the stream.
+    pub last_stride: wgt::BufferAddress,
+
     /// Whether the buffer is indexed by vertex number or instance number.
     pub mode: wgt::VertexStepMode,
 }
@@ -501,6 +504,7 @@ impl Default for VertexStep {
     fn default() -> Self {
         Self {
             stride: 0,
+            last_stride: 0,
             mode: wgt::VertexStepMode::Vertex,
         }
     }