Validate binding ranges against buffer size

Author: andyleiserson

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 or equal to 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

@@ -602,6 +602,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 +621,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 = 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 +662,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 = 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,10 +963,14 @@ impl RenderBundle {
                     size,
                 } => {
                     let buffer = buffer.try_raw(snatch_guard)?;
-                    let bb = hal::BufferBinding {
-                        buffer,
-                        offset: *offset,
-                        size: *size,
+                    let bb = unsafe {
+                        // SAFETY: The binding size was checked against the buffer size
+                        // in `set_index_buffer` and again in `IndexState::flush`.
+                        hal::BufferBinding::new_unchecked(
+                            buffer,
+                            *offset,
+                            size.expect("size was resolved in `RenderBundleEncoder::finish`"),
+                        )
                     };
                     unsafe { raw.set_index_buffer(bb, *index_format) };
                 }
@@ -979,10 +981,14 @@ impl RenderBundle {
                     size,
                 } => {
                     let buffer = buffer.try_raw(snatch_guard)?;
-                    let bb = hal::BufferBinding {
-                        buffer,
-                        offset: *offset,
-                        size: *size,
+                    let bb = unsafe {
+                        // SAFETY: The binding size was checked against the buffer size
+                        // in `set_vertex_buffer` and again in `VertexState::flush`.
+                        hal::BufferBinding::new_unchecked(
+                            buffer,
+                            *offset,
+                            size.expect("size was resolved in `RenderBundleEncoder::finish`"),
+                        )
                     };
                     unsafe { raw.set_vertex_buffer(*slot, bb) };
                 }
@@ -1131,6 +1137,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 +1161,24 @@ 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)
+            .and_then(wgt::BufferSize::new);
+        assert!(
+            self.range.end <= self.buffer.size && binding_size.is_some(),
+            "index buffer range must have non-zero size and 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: binding_size,
             })
         } else {
             None
@@ -1174,6 +1194,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 +1206,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 +1221,24 @@ impl VertexState {
     ///
     /// `slot` is the index of the vertex buffer slot that `self` tracks.
     fn flush(&mut self, slot: u32) -> 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)
+            .and_then(wgt::BufferSize::new);
+        assert!(
+            self.range.end <= self.buffer.size && binding_size.is_some(),
+            "vertex buffer range must have non-zero size and 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: 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/render.rs

@@ -1,5 +1,5 @@
 use alloc::{borrow::Cow, sync::Arc, vec::Vec};
-use core::{fmt, num::NonZeroU32, ops::Range, str};
+use core::{fmt, num::NonZeroU32, str};
 
 use arrayvec::ArrayVec;
 use thiserror::Error;
@@ -356,13 +356,17 @@ struct IndexState {
 }
 
 impl IndexState {
-    fn update_buffer(&mut self, range: Range<BufferAddress>, format: IndexFormat) {
+    fn update_buffer<B: hal::DynBuffer + ?Sized>(
+        &mut self,
+        binding: &hal::BufferBinding<'_, B>,
+        format: IndexFormat,
+    ) {
         self.buffer_format = Some(format);
         let shift = match format {
             IndexFormat::Uint16 => 1,
             IndexFormat::Uint32 => 2,
         };
-        self.limit = (range.end - range.start) >> shift;
+        self.limit = binding.size.get() >> shift;
     }
 
     fn reset(&mut self) {
@@ -2322,6 +2326,7 @@ fn set_pipeline(
     Ok(())
 }
 
+// This function is duplicative of `bundle::set_index_buffer`.
 fn set_index_buffer(
     state: &mut State,
     cmd_buf: &Arc<CommandBuffer>,
@@ -2341,33 +2346,27 @@ fn set_index_buffer(
     buffer.same_device_as(cmd_buf.as_ref())?;
 
     buffer.check_usage(BufferUsages::INDEX)?;
-    let buf_raw = buffer.try_raw(state.general.snatch_guard)?;
 
-    let end = match size {
-        Some(s) => offset + s.get(),
-        None => buffer.size,
-    };
-    state.index.update_buffer(offset..end, index_format);
+    let binding = buffer
+        .binding(offset, size, state.general.snatch_guard)
+        .map_err(RenderCommandError::from)?;
+    state.index.update_buffer(&binding, index_format);
 
     state.general.buffer_memory_init_actions.extend(
         buffer.initialization_status.read().create_action(
             &buffer,
-            offset..end,
+            offset..(offset + binding.size.get()),
             MemoryInitKind::NeedsInitializedMemory,
         ),
     );
 
-    let bb = hal::BufferBinding {
-        buffer: buf_raw,
-        offset,
-        size,
-    };
     unsafe {
-        hal::DynCommandEncoder::set_index_buffer(state.general.raw_encoder, bb, index_format);
+        hal::DynCommandEncoder::set_index_buffer(state.general.raw_encoder, binding, index_format);
     }
     Ok(())
 }
 
+// This function is duplicative of `render::set_vertex_buffer`.
 fn set_vertex_buffer(
     state: &mut State,
     cmd_buf: &Arc<CommandBuffer>,
@@ -2399,30 +2398,22 @@ fn set_vertex_buffer(
     }
 
     buffer.check_usage(BufferUsages::VERTEX)?;
-    let buf_raw = buffer.try_raw(state.general.snatch_guard)?;
 
-    //TODO: where are we checking that the offset is in bound?
-    let buffer_size = match size {
-        Some(s) => s.get(),
-        None => buffer.size - offset,
-    };
-    state.vertex.buffer_sizes[slot as usize] = Some(buffer_size);
+    let binding = buffer
+        .binding(offset, size, state.general.snatch_guard)
+        .map_err(RenderCommandError::from)?;
+    state.vertex.buffer_sizes[slot as usize] = Some(binding.size.get());
 
     state.general.buffer_memory_init_actions.extend(
         buffer.initialization_status.read().create_action(
             &buffer,
-            offset..(offset + buffer_size),
+            offset..(offset + binding.size.get()),
             MemoryInitKind::NeedsInitializedMemory,
         ),
     );
 
-    let bb = hal::BufferBinding {
-        buffer: buf_raw,
-        offset,
-        size,
-    };
     unsafe {
-        hal::DynCommandEncoder::set_vertex_buffer(state.general.raw_encoder, slot, bb);
+        hal::DynCommandEncoder::set_vertex_buffer(state.general.raw_encoder, slot, binding);
     }
     if let Some(pipeline) = state.pipeline.as_ref() {
         state.vertex.update_limits(&pipeline.vertex_steps);