Validate binding ranges against buffer size

cts_runner/test.lst

@@ -27,6 +27,10 @@ webgpu:api,validation,encoding,cmds,copyTextureToTexture:sample_count:*
 //FAIL: webgpu:api,validation,encoding,cmds,copyTextureToTexture:copy_ranges_with_compressed_texture_formats:*
 webgpu:api,validation,encoding,cmds,index_access:*
 //FAIL: webgpu:api,validation,encoding,cmds,render,draw:*
+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,setIndexBuffer:index_buffer_state:*
+webgpu:api,validation,encoding,cmds,render,setVertexBuffer:vertex_buffer_state:*
 webgpu:api,validation,encoding,encoder_state:*
 webgpu:api,validation,encoding,encoder_open_state:non_pass_commands:*
 webgpu:api,validation,encoding,encoder_open_state:render_pass_commands:*

deno_webgpu/device.rs

@@ -680,29 +680,26 @@ impl GPUDevice {
                     .buffers
                     .into_iter()
                     .map(|b| {
-                        let layout = b.into_option().ok_or_else(|| {
-                            JsErrorBox::type_error(
-                                "Nullable GPUVertexBufferLayouts are currently not supported",
-                            )
-                        })?;
-
-                        Ok(wgpu_core::pipeline::VertexBufferLayout {
-                            array_stride: layout.array_stride,
-                            step_mode: layout.step_mode.into(),
-                            attributes: Cow::Owned(
-                                layout
-                                    .attributes
-                                    .into_iter()
-                                    .map(|attr| wgpu_types::VertexAttribute {
-                                        format: attr.format.into(),
-                                        offset: attr.offset,
-                                        shader_location: attr.shader_location,
-                                    })
-                                    .collect(),
-                            ),
-                        })
+                        b.into_option().map_or_else(
+                            wgpu_core::pipeline::VertexBufferLayout::default,
+                            |layout| wgpu_core::pipeline::VertexBufferLayout {
+                                array_stride: layout.array_stride,
+                                step_mode: layout.step_mode.into(),
+                                attributes: Cow::Owned(
+                                    layout
+                                        .attributes
+                                        .into_iter()
+                                        .map(|attr| wgpu_types::VertexAttribute {
+                                            format: attr.format.into(),
+                                            offset: attr.offset,
+                                            shader_location: attr.shader_location,
+                                        })
+                                        .collect(),
+                                ),
+                            },
+                        )
                     })
-                    .collect::<Result<_, JsErrorBox>>()?,
+                    .collect(),
             ),
         };
 

wgpu-core/src/binding_model.rs

@@ -94,15 +94,48 @@ impl WebGpuError for CreateBindGroupLayoutError {
     }
 }
 
-//TODO: refactor this to move out `enum BindingError`.
+#[derive(Clone, Debug, Error)]
+#[non_exhaustive]
+pub enum BindingError {
+    #[error(transparent)]
+    DestroyedResource(#[from] DestroyedResourceError),
+    #[error("Buffer {buffer}: Binding with size {binding_size} at offset {offset} would overflow buffer size of {buffer_size}")]
+    BindingRangeTooLarge {
+        buffer: ResourceErrorIdent,
+        offset: wgt::BufferAddress,
+        binding_size: u64,
+        buffer_size: u64,
+    },
+    #[error("Buffer {buffer}: Binding offset {offset} is greater than buffer size {buffer_size}")]
+    BindingOffsetTooLarge {
+        buffer: ResourceErrorIdent,
+        offset: wgt::BufferAddress,
+        buffer_size: u64,
+    },
+}
 
+impl WebGpuError for BindingError {
+    fn webgpu_error_type(&self) -> ErrorType {
+        match self {
+            Self::DestroyedResource(e) => e.webgpu_error_type(),
+            Self::BindingRangeTooLarge { .. } | Self::BindingOffsetTooLarge { .. } => {
+                ErrorType::Validation
+            }
+        }
+    }
+}
+
+// TODO: there may be additional variants here that can be extracted into
+// `BindingError`.
 #[derive(Clone, Debug, Error)]
 #[non_exhaustive]
 pub enum CreateBindGroupError {
     #[error(transparent)]
     Device(#[from] DeviceError),
     #[error(transparent)]
     DestroyedResource(#[from] DestroyedResourceError),
+    #[error(transparent)]
+    BindingError(#[from] BindingError),
     #[error(
         "Binding count declared with at most {expected} items, but {actual} items were provided"
     )]
@@ -113,12 +146,6 @@ pub enum CreateBindGroupError {
     BindingArrayLengthMismatch { actual: usize, expected: usize },
     #[error("Array binding provided zero elements")]
     BindingArrayZeroLength,
-    #[error("The bound range {range:?} of {buffer} overflows its size ({size})")]
-    BindingRangeTooLarge {
-        buffer: ResourceErrorIdent,
-        range: Range<wgt::BufferAddress>,
-        size: u64,
-    },
     #[error("Binding size {actual} of {buffer} is less than minimum {min}")]
     BindingSizeTooSmall {
         buffer: ResourceErrorIdent,
@@ -233,14 +260,14 @@ impl WebGpuError for CreateBindGroupError {
         let e: &dyn WebGpuError = match self {
             Self::Device(e) => e,
             Self::DestroyedResource(e) => e,
+            Self::BindingError(e) => e,
             Self::MissingBufferUsage(e) => e,
             Self::MissingTextureUsage(e) => e,
             Self::ResourceUsageCompatibility(e) => e,
             Self::InvalidResource(e) => e,
             Self::BindingArrayPartialLengthMismatch { .. }
             | Self::BindingArrayLengthMismatch { .. }
             | Self::BindingArrayZeroLength
-            | Self::BindingRangeTooLarge { .. }
             | Self::BindingSizeTooSmall { .. }
             | Self::BindingsNumMismatch { .. }
             | Self::BindingZeroSize(_)

wgpu-core/src/command/bundle.rs

@@ -93,6 +93,7 @@ use core::{
 use arrayvec::ArrayVec;
 use thiserror::Error;
 
+use wgpu_hal::ShouldBeNonZeroExt;
 use wgt::error::{ErrorType, WebGpuError};
 
 use crate::{
@@ -602,6 +603,7 @@ fn set_pipeline(
     Ok(())
 }
 
+// This function is duplicative of `render::set_index_buffer`.
 fn set_index_buffer(
     state: &mut State,
     buffer_guard: &crate::storage::Storage<Fallible<Buffer>>,
@@ -620,21 +622,20 @@ fn set_index_buffer(
     buffer.same_device(&state.device)?;
     buffer.check_usage(wgt::BufferUsages::INDEX)?;
 
-    let end = match size {
-        Some(s) => offset + s.get(),
-        None => buffer.size,
-    };
+    let end = offset + buffer.resolve_binding_size(offset, size)?;
+
     state
         .buffer_memory_init_actions
         .extend(buffer.initialization_status.read().create_action(
             &buffer,
-            offset..end,
+            offset..end.get(),
             MemoryInitKind::NeedsInitializedMemory,
         ));
-    state.set_index_buffer(buffer, index_format, offset..end);
+    state.set_index_buffer(buffer, index_format, offset..end.get());
     Ok(())
 }
 
+// This function is duplicative of `render::set_vertex_buffer`.
 fn set_vertex_buffer(
     state: &mut State,
     buffer_guard: &crate::storage::Storage<Fallible<Buffer>>,
@@ -662,18 +663,16 @@ fn set_vertex_buffer(
     buffer.same_device(&state.device)?;
     buffer.check_usage(wgt::BufferUsages::VERTEX)?;
 
-    let end = match size {
-        Some(s) => offset + s.get(),
-        None => buffer.size,
-    };
+    let end = offset + buffer.resolve_binding_size(offset, size)?;
+
     state
         .buffer_memory_init_actions
         .extend(buffer.initialization_status.read().create_action(
             &buffer,
-            offset..end,
+            offset..end.get(),
             MemoryInitKind::NeedsInitializedMemory,
         ));
-    state.vertex[slot as usize] = Some(VertexState::new(buffer, offset..end));
+    state.vertex[slot as usize] = Some(VertexState::new(buffer, offset..end.get()));
     Ok(())
 }
 
@@ -965,11 +964,9 @@ impl RenderBundle {
                     size,
                 } => {
                     let buffer = buffer.try_raw(snatch_guard)?;
-                    let bb = hal::BufferBinding {
-                        buffer,
-                        offset: *offset,
-                        size: *size,
-                    };
+                    // SAFETY: The binding size was checked against the buffer size
+                    // in `set_index_buffer` and again in `IndexState::flush`.
+                    let bb = hal::BufferBinding::new_unchecked(buffer, *offset, *size);
                     unsafe { raw.set_index_buffer(bb, *index_format) };
                 }
                 Cmd::SetVertexBuffer {
@@ -979,11 +976,9 @@ impl RenderBundle {
                     size,
                 } => {
                     let buffer = buffer.try_raw(snatch_guard)?;
-                    let bb = hal::BufferBinding {
-                        buffer,
-                        offset: *offset,
-                        size: *size,
-                    };
+                    // SAFETY: The binding size was checked against the buffer size
+                    // in `set_vertex_buffer` and again in `VertexState::flush`.
+                    let bb = hal::BufferBinding::new_unchecked(buffer, *offset, *size);
                     unsafe { raw.set_vertex_buffer(*slot, bb) };
                 }
                 Cmd::SetPushConstant {
@@ -1131,6 +1126,9 @@ crate::impl_trackable!(RenderBundle);
 /// [`RenderBundleEncoder::finish`] records the currently set index buffer here,
 /// and calls [`State::flush_index`] before any indexed draw command to produce
 /// a `SetIndexBuffer` command if one is necessary.
+///
+/// Binding ranges must be validated against the size of the buffer before
+/// being stored in `IndexState`.
 #[derive(Debug)]
 struct IndexState {
     buffer: Arc<Buffer>,
@@ -1152,13 +1150,21 @@ impl IndexState {
     /// Generate a `SetIndexBuffer` command to prepare for an indexed draw
     /// command, if needed.
     fn flush(&mut self) -> Option<ArcRenderCommand> {
+        // This was all checked before, but let's check again just in case.
+        let binding_size = self
+            .range
+            .end
+            .checked_sub(self.range.start)
+            .filter(|_| self.range.end <= self.buffer.size)
+            .expect("index range must be contained in buffer");
+
         if self.is_dirty {
             self.is_dirty = false;
             Some(ArcRenderCommand::SetIndexBuffer {
                 buffer: self.buffer.clone(),
                 index_format: self.format,
                 offset: self.range.start,
-                size: wgt::BufferSize::new(self.range.end - self.range.start),
+                size: NonZeroU64::new(binding_size),
             })
         } else {
             None
@@ -1174,6 +1180,9 @@ impl IndexState {
 /// calls this type's [`flush`] method just before any draw command to
 /// produce a `SetVertexBuffer` commands if one is necessary.
 ///
+/// Binding ranges must be validated against the size of the buffer before
+/// being stored in `VertexState`.
+///
 /// [`flush`]: IndexState::flush
 #[derive(Debug)]
 struct VertexState {
@@ -1183,6 +1192,9 @@ struct VertexState {
 }
 
 impl VertexState {
+    /// Create a new `VertexState`.
+    ///
+    /// The `range` must be contained within `buffer`.
     fn new(buffer: Arc<Buffer>, range: Range<wgt::BufferAddress>) -> Self {
         Self {
             buffer,
@@ -1195,13 +1207,20 @@ impl VertexState {
     ///
     /// `slot` is the index of the vertex buffer slot that `self` tracks.
     fn flush(&mut self, slot: u32) -> Option<ArcRenderCommand> {
+        let binding_size = self
+            .range
+            .end
+            .checked_sub(self.range.start)
+            .filter(|_| self.range.end <= self.buffer.size)
+            .expect("vertex range must be contained in buffer");
+
         if self.is_dirty {
             self.is_dirty = false;
             Some(ArcRenderCommand::SetVertexBuffer {
                 slot,
                 buffer: self.buffer.clone(),
                 offset: self.range.start,
-                size: wgt::BufferSize::new(self.range.end - self.range.start),
+                size: NonZeroU64::new(binding_size),
             })
         } else {
             None

wgpu-core/src/command/draw.rs

@@ -7,7 +7,7 @@ use wgt::error::{ErrorType, WebGpuError};
 use super::bind::BinderError;
 use crate::command::pass;
 use crate::{
-    binding_model::{LateMinBufferBindingSizeMismatch, PushConstantUploadError},
+    binding_model::{BindingError, LateMinBufferBindingSizeMismatch, PushConstantUploadError},
     resource::{
         DestroyedResourceError, MissingBufferUsageError, MissingTextureUsageError,
         ResourceErrorIdent,
@@ -89,6 +89,8 @@ pub enum RenderCommandError {
     MissingTextureUsage(#[from] MissingTextureUsageError),
     #[error(transparent)]
     PushConstants(#[from] PushConstantUploadError),
+    #[error(transparent)]
+    BindingError(#[from] BindingError),
     #[error("Viewport size {{ w: {w}, h: {h} }} greater than device's requested `max_texture_dimension_2d` limit {max}, or less than zero")]
     InvalidViewportRectSize { w: f32, h: f32, max: u32 },
     #[error("Viewport has invalid rect {rect:?} for device's requested `max_texture_dimension_2d` limit; Origin less than -2 * `max_texture_dimension_2d` ({min}), or rect extends past 2 * `max_texture_dimension_2d` - 1 ({max})")]
@@ -110,6 +112,7 @@ impl WebGpuError for RenderCommandError {
             Self::MissingBufferUsage(e) => e,
             Self::MissingTextureUsage(e) => e,
             Self::PushConstants(e) => e,
+            Self::BindingError(e) => e,
 
             Self::BindGroupIndexOutOfRange { .. }
             | Self::VertexBufferIndexOutOfRange { .. }

wgpu-core/src/command/mod.rs

@@ -31,6 +31,7 @@ pub use timestamp_writes::PassTimestampWrites;
 
 use self::memory_init::CommandBufferTextureMemoryActions;
 
+use crate::binding_model::BindingError;
 use crate::command::transition_resources::TransitionResourcesError;
 use crate::device::queue::TempResource;
 use crate::device::{Device, DeviceError, MissingFeatures};
@@ -1060,6 +1061,18 @@ impl CommandEncoderError {
                     inner: RenderPassErrorInner::DestroyedResource(_),
                     ..
                 })
+                | Self::RenderPass(RenderPassError {
+                    inner: RenderPassErrorInner::RenderCommand(
+                        RenderCommandError::DestroyedResource(_)
+                    ),
+                    ..
+                })
+                | Self::RenderPass(RenderPassError {
+                    inner: RenderPassErrorInner::RenderCommand(RenderCommandError::BindingError(
+                        BindingError::DestroyedResource(_)
+                    )),
+                    ..
+                })
         )
     }
 }