From ef428fcab8059e898b42542b6445bd94a9683e69 Mon Sep 17 00:00:00 2001 From: Andy Leiserson Date: Thu, 26 Jun 2025 17:45:07 -0700 Subject: [PATCH] Validate binding ranges against buffer size --- wgpu-core/src/binding_model.rs | 43 +++++++-- wgpu-core/src/command/bundle.rs | 81 ++++++++++++----- wgpu-core/src/command/draw.rs | 5 +- wgpu-core/src/command/render.rs | 51 +++++------ wgpu-core/src/command/render_command.rs | 21 +++++ wgpu-core/src/device/global.rs | 3 +- wgpu-core/src/device/resource.rs | 89 ++++++++----------- wgpu-core/src/indirect_validation/dispatch.rs | 14 ++- wgpu-core/src/indirect_validation/draw.rs | 14 ++- wgpu-core/src/resource.rs | 72 ++++++++++++++- wgpu-core/src/timestamp_normalization/mod.rs | 16 ++-- wgpu-hal/examples/halmark/main.rs | 22 +++-- wgpu-hal/examples/ray-traced-triangle/main.rs | 11 ++- wgpu-hal/src/gles/device.rs | 9 +- wgpu-hal/src/gles/mod.rs | 1 - wgpu-hal/src/lib.rs | 73 +++++++++++---- wgpu-hal/src/metal/command.rs | 12 +-- wgpu-hal/src/metal/device.rs | 16 +--- wgpu-hal/src/metal/mod.rs | 10 --- wgpu-hal/src/vulkan/device.rs | 4 +- wgpu/src/api/device.rs | 1 + wgpu/src/backend/wgpu_core.rs | 6 ++ 22 files changed, 362 insertions(+), 212 deletions(-) diff --git a/wgpu-core/src/binding_model.rs b/wgpu-core/src/binding_model.rs index 8075887ed..9b1c12fad 100644 --- a/wgpu-core/src/binding_model.rs +++ b/wgpu-core/src/binding_model.rs @@ -94,8 +94,39 @@ 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 { @@ -103,6 +134,8 @@ pub enum CreateBindGroupError { 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, - size: u64, - }, #[error("Binding size {actual} of {buffer} is less than minimum {min}")] BindingSizeTooSmall { buffer: ResourceErrorIdent, @@ -233,6 +260,7 @@ 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, @@ -240,7 +268,6 @@ impl WebGpuError for CreateBindGroupError { Self::BindingArrayPartialLengthMismatch { .. } | Self::BindingArrayLengthMismatch { .. } | Self::BindingArrayZeroLength - | Self::BindingRangeTooLarge { .. } | Self::BindingSizeTooSmall { .. } | Self::BindingsNumMismatch { .. } | Self::BindingZeroSize(_) diff --git a/wgpu-core/src/command/bundle.rs b/wgpu-core/src/command/bundle.rs index 7a64502b1..d924190db 100644 --- a/wgpu-core/src/command/bundle.rs +++ b/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>, @@ -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>, @@ -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, @@ -1152,13 +1161,24 @@ impl IndexState { /// Generate a `SetIndexBuffer` command to prepare for an indexed draw /// command, if needed. fn flush(&mut self) -> Option { + // 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, range: Range) -> 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 { + // 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 diff --git a/wgpu-core/src/command/draw.rs b/wgpu-core/src/command/draw.rs index 53a3f204f..7dadc8bfa 100644 --- a/wgpu-core/src/command/draw.rs +++ b/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 { .. } diff --git a/wgpu-core/src/command/render.rs b/wgpu-core/src/command/render.rs index 19129f891..d1596a5c4 100644 --- a/wgpu-core/src/command/render.rs +++ b/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, format: IndexFormat) { + fn update_buffer( + &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, @@ -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, @@ -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); diff --git a/wgpu-core/src/command/render_command.rs b/wgpu-core/src/command/render_command.rs index 6fc4cbf5c..606d3fe94 100644 --- a/wgpu-core/src/command/render_command.rs +++ b/wgpu-core/src/command/render_command.rs @@ -392,6 +392,17 @@ impl RenderCommand { } /// Equivalent to `RenderCommand` with the Ids resolved into resource Arcs. +/// +/// In a render pass, commands are stored in this format between when they are +/// added to the pass, and when the pass is `end()`ed and the commands are +/// replayed to the HAL encoder. Validation occurs when the pass is ended, which +/// means that parameters stored in an `ArcRenderCommand` for a pass operation +/// have generally not been validated. +/// +/// In a render bundle, commands are stored in this format between when the bundle +/// is `finish()`ed and when the bundle is executed. Validation occurs when the +/// bundle is finished, which means that parameters stored in an `ArcRenderCommand` +/// for a render bundle operation must have been validated. #[doc(hidden)] #[derive(Clone, Debug)] pub enum ArcRenderCommand { @@ -405,12 +416,22 @@ pub enum ArcRenderCommand { buffer: Arc, index_format: wgt::IndexFormat, offset: BufferAddress, + + // For a render pass, this reflects the argument passed by the + // application, which may be `None`. For a render bundle, this reflects + // the validated size of the binding, and will be populated even in the + // case that the application omitted the size. size: Option, }, SetVertexBuffer { slot: u32, buffer: Arc, offset: BufferAddress, + + // For a render pass, this reflects the argument passed by the + // application, which may be `None`. For a render bundle, this reflects + // the validated size of the binding, and will be populated even in the + // case that the application omitted the size. size: Option, }, SetBlendConstant(Color), diff --git a/wgpu-core/src/device/global.rs b/wgpu-core/src/device/global.rs index d61be9613..d05fb8c8c 100644 --- a/wgpu-core/src/device/global.rs +++ b/wgpu-core/src/device/global.rs @@ -383,6 +383,7 @@ impl Global { /// - `hal_buffer` must be created from `device_id` corresponding raw handle. /// - `hal_buffer` must be created respecting `desc` /// - `hal_buffer` must be initialized + /// - `hal_buffer` must not have zero size. pub unsafe fn create_buffer_from_hal( &self, hal_buffer: A::Buffer, @@ -404,7 +405,7 @@ impl Global { trace.add(trace::Action::CreateBuffer(fid.id(), desc.clone())); } - let (buffer, err) = device.create_buffer_from_hal(Box::new(hal_buffer), desc); + let (buffer, err) = unsafe { device.create_buffer_from_hal(Box::new(hal_buffer), desc) }; let id = fid.assign(buffer); api_log!("Device::create_buffer -> {id:?}"); diff --git a/wgpu-core/src/device/resource.rs b/wgpu-core/src/device/resource.rs index f68b8d693..a668f270a 100644 --- a/wgpu-core/src/device/resource.rs +++ b/wgpu-core/src/device/resource.rs @@ -702,7 +702,8 @@ impl Device { let buffer = unsafe { self.raw().create_buffer(&hal_desc) } .map_err(|e| self.handle_hal_error_with_nonfatal_oom(e))?; - let timestamp_normalization_bind_group = Snatchable::new( + let timestamp_normalization_bind_group = Snatchable::new(unsafe { + // SAFETY: The size passed here must not overflow the buffer. self.timestamp_normalizer .get() .unwrap() @@ -710,10 +711,10 @@ impl Device { self, &*buffer, desc.label.as_deref(), - desc.size, + wgt::BufferSize::new(hal_desc.size).unwrap(), desc.usage, - )?, - ); + ) + }?); let indirect_validation_bind_groups = self.create_indirect_validation_bind_groups(buffer.as_ref(), desc.size, desc.usage)?; @@ -809,28 +810,36 @@ impl Device { Ok(texture) } - pub(crate) fn create_buffer_from_hal( + /// # Safety + /// + /// - `hal_buffer` must have been created on this device. + /// - `hal_buffer` must have been created respecting `desc` (in particular, the size). + /// - `hal_buffer` must be initialized. + /// - `hal_buffer` must not have zero size. + pub(crate) unsafe fn create_buffer_from_hal( self: &Arc, hal_buffer: Box, desc: &resource::BufferDescriptor, ) -> (Fallible, Option) { - let timestamp_normalization_bind_group = match self - .timestamp_normalizer - .get() - .unwrap() - .create_normalization_bind_group( - self, - &*hal_buffer, - desc.label.as_deref(), - desc.size, - desc.usage, - ) { - Ok(bg) => Snatchable::new(bg), - Err(e) => { - return ( - Fallible::Invalid(Arc::new(desc.label.to_string())), - Some(e.into()), - ) + let timestamp_normalization_bind_group = unsafe { + match self + .timestamp_normalizer + .get() + .unwrap() + .create_normalization_bind_group( + self, + &*hal_buffer, + desc.label.as_deref(), + wgt::BufferSize::new(desc.size).unwrap(), + desc.usage, + ) { + Ok(bg) => Snatchable::new(bg), + Err(e) => { + return ( + Fallible::Invalid(Arc::new(desc.label.to_string())), + Some(e.into()), + ) + } } }; @@ -2187,31 +2196,9 @@ impl Device { buffer.same_device(self)?; buffer.check_usage(pub_usage)?; - let raw_buffer = buffer.try_raw(snatch_guard)?; - let (bind_size, bind_end) = match bb.size { - Some(size) => { - let end = bb.offset + size.get(); - if end > buffer.size { - return Err(Error::BindingRangeTooLarge { - buffer: buffer.error_ident(), - range: bb.offset..end, - size: buffer.size, - }); - } - (size.get(), end) - } - None => { - if buffer.size < bb.offset { - return Err(Error::BindingRangeTooLarge { - buffer: buffer.error_ident(), - range: bb.offset..bb.offset, - size: buffer.size, - }); - } - (buffer.size - bb.offset, buffer.size) - } - }; + let bb = buffer.binding(bb.offset, bb.size, snatch_guard)?; + let bind_size = bb.size.get(); if bind_size > range_limit as u64 { return Err(Error::BufferRangeTooLarge { @@ -2226,8 +2213,8 @@ impl Device { dynamic_binding_info.push(binding_model::BindGroupDynamicBindingData { binding_idx: binding, buffer_size: buffer.size, - binding_range: bb.offset..bind_end, - maximum_dynamic_offset: buffer.size - bind_end, + binding_range: bb.offset..bb.offset + bind_size, + maximum_dynamic_offset: buffer.size - bb.offset - bind_size, binding_type: binding_ty, }); } @@ -2265,11 +2252,7 @@ impl Device { MemoryInitKind::NeedsInitializedMemory, )); - Ok(hal::BufferBinding { - buffer: raw_buffer, - offset: bb.offset, - size: bb.size, - }) + Ok(bb) } fn create_sampler_binding<'a>( diff --git a/wgpu-core/src/indirect_validation/dispatch.rs b/wgpu-core/src/indirect_validation/dispatch.rs index 00e3798e9..e9fe4971b 100644 --- a/wgpu-core/src/indirect_validation/dispatch.rs +++ b/wgpu-core/src/indirect_validation/dispatch.rs @@ -232,10 +232,9 @@ impl Dispatch { resource_index: 0, count: 1, }], - buffers: &[hal::BufferBinding { - buffer: dst_buffer.as_ref(), - offset: 0, - size: Some(DST_BUFFER_SIZE), + buffers: &[unsafe { + // SAFETY: We just created the buffer with this size. + hal::BufferBinding::new_unchecked(dst_buffer.as_ref(), 0, DST_BUFFER_SIZE) }], samplers: &[], textures: &[], @@ -278,10 +277,9 @@ impl Dispatch { resource_index: 0, count: 1, }], - buffers: &[hal::BufferBinding { - buffer, - offset: 0, - size: Some(binding_size), + buffers: &[unsafe { + // SAFETY: We calculated the binding size to fit within the buffer. + hal::BufferBinding::new_unchecked(buffer, 0, binding_size) }], samplers: &[], textures: &[], diff --git a/wgpu-core/src/indirect_validation/draw.rs b/wgpu-core/src/indirect_validation/draw.rs index d88acb8d6..af0e1a2c5 100644 --- a/wgpu-core/src/indirect_validation/draw.rs +++ b/wgpu-core/src/indirect_validation/draw.rs @@ -135,10 +135,9 @@ impl Draw { resource_index: 0, count: 1, }], - buffers: &[hal::BufferBinding { - buffer, - offset: 0, - size: Some(binding_size), + buffers: &[unsafe { + // SAFETY: We calculated the binding size to fit within the buffer. + hal::BufferBinding::new_unchecked(buffer, 0, binding_size) }], samplers: &[], textures: &[], @@ -684,10 +683,9 @@ fn create_buffer_and_bind_group( resource_index: 0, count: 1, }], - buffers: &[hal::BufferBinding { - buffer: buffer.as_ref(), - offset: 0, - size: Some(BUFFER_SIZE), + buffers: &[unsafe { + // SAFETY: We just created the buffer with this size. + hal::BufferBinding::new_unchecked(buffer.as_ref(), 0, BUFFER_SIZE) }], samplers: &[], textures: &[], diff --git a/wgpu-core/src/resource.rs b/wgpu-core/src/resource.rs index 022b1ba59..df18ae83e 100644 --- a/wgpu-core/src/resource.rs +++ b/wgpu-core/src/resource.rs @@ -17,7 +17,7 @@ use wgt::{ #[cfg(feature = "trace")] use crate::device::trace; use crate::{ - binding_model::BindGroup, + binding_model::{BindGroup, BindingError}, device::{ queue, resource::DeferredDestroy, BufferMapPendingClosure, Device, DeviceError, DeviceMismatch, HostMap, MissingDownlevelFlags, MissingFeatures, @@ -485,6 +485,76 @@ impl Buffer { } } + /// Resolve the size of a binding for buffer with `offset` and `size`. + /// + /// If `size` is `None`, then the remainder of the buffer starting from + /// `offset` is used. + /// + /// If the binding would overflow the buffer or is empty (see + /// [`hal::BufferBinding`]), then an error is returned. + pub fn resolve_binding_size( + &self, + offset: wgt::BufferAddress, + binding_size: Option, + ) -> Result { + let buffer_size = self.size; + + match binding_size { + Some(binding_size) => { + match offset.checked_add(binding_size.get()) { + // `binding_size` is not zero which means `end == buffer_size` is ok. + Some(end) if end <= buffer_size => Ok(binding_size), + _ => Err(BindingError::BindingRangeTooLarge { + buffer: self.error_ident(), + offset, + binding_size: binding_size.get(), + buffer_size, + }), + } + } + None => { + // We require that `buffer_size - offset` converts to + // `BufferSize` (`NonZeroU64`) because bindings must not be + // empty. + buffer_size + .checked_sub(offset) + .and_then(wgt::BufferSize::new) + .ok_or_else(|| BindingError::BindingOffsetTooLarge { + buffer: self.error_ident(), + offset, + buffer_size, + }) + } + } + } + + /// Create a new [`hal::BufferBinding`] for the buffer with `offset` and + /// `size`. + /// + /// If `size` is `None`, then the remainder of the buffer starting from + /// `offset` is used. + /// + /// If the binding would overflow the buffer or is empty (see + /// [`hal::BufferBinding`]), then an error is returned. + pub fn binding<'a>( + &'a self, + offset: wgt::BufferAddress, + binding_size: Option, + snatch_guard: &'a SnatchGuard, + ) -> Result, BindingError> { + let buf_raw = self.try_raw(snatch_guard)?; + let resolved_size = self.resolve_binding_size(offset, binding_size)?; + unsafe { + // SAFETY: The offset and size passed to hal::BufferBinding::new_unchecked must + // define a binding contained within the buffer. + Ok(hal::BufferBinding::new_unchecked( + buf_raw, + offset, + resolved_size, + )) + } + } + /// Returns the mapping callback in case of error so that the callback can be fired outside /// of the locks that are held in this function. pub(crate) fn map_async( diff --git a/wgpu-core/src/timestamp_normalization/mod.rs b/wgpu-core/src/timestamp_normalization/mod.rs index dd4d46623..e5a9ef9a8 100644 --- a/wgpu-core/src/timestamp_normalization/mod.rs +++ b/wgpu-core/src/timestamp_normalization/mod.rs @@ -242,12 +242,16 @@ impl TimestampNormalizer { } } - pub fn create_normalization_bind_group( + /// Create a bind group for normalizing timestamps in `buffer`. + /// + /// This function is unsafe because it does not know that `buffer_size` is + /// the true size of the buffer. + pub unsafe fn create_normalization_bind_group( &self, device: &Device, buffer: &dyn hal::DynBuffer, buffer_label: Option<&str>, - buffer_size: u64, + buffer_size: wgt::BufferSize, buffer_usages: wgt::BufferUsages, ) -> Result { unsafe { @@ -263,7 +267,7 @@ impl TimestampNormalizer { // at once to normalize the timestamps, we can't use it. We force the buffer to fail // to allocate. The lowest max binding size is 128MB, and query sets must be small // (no more than 4096), so this should never be hit in practice by sane programs. - if buffer_size > device.adapter.limits().max_storage_buffer_binding_size as u64 { + if buffer_size.get() > device.adapter.limits().max_storage_buffer_binding_size as u64 { return Err(DeviceError::OutOfMemory); } @@ -282,11 +286,7 @@ impl TimestampNormalizer { .create_bind_group(&hal::BindGroupDescriptor { label: Some(label), layout: &*state.temporary_bind_group_layout, - buffers: &[hal::BufferBinding { - buffer, - offset: 0, - size: None, - }], + buffers: &[hal::BufferBinding::new_unchecked(buffer, 0, buffer_size)], samplers: &[], textures: &[], acceleration_structures: &[], diff --git a/wgpu-hal/examples/halmark/main.rs b/wgpu-hal/examples/halmark/main.rs index 75f3bc2fb..5641eb4de 100644 --- a/wgpu-hal/examples/halmark/main.rs +++ b/wgpu-hal/examples/halmark/main.rs @@ -445,10 +445,13 @@ impl Example { let texture_view = unsafe { device.create_texture_view(&texture, &view_desc).unwrap() }; let global_group = { - let global_buffer_binding = hal::BufferBinding { - buffer: &global_buffer, - offset: 0, - size: None, + let global_buffer_binding = unsafe { + // SAFETY: This is the same size that was specified for buffer creation. + hal::BufferBinding::new_unchecked( + &global_buffer, + 0, + global_buffer_desc.size.try_into().unwrap(), + ) }; let texture_binding = hal::TextureBinding { view: &texture_view, @@ -483,10 +486,13 @@ impl Example { }; let local_group = { - let local_buffer_binding = hal::BufferBinding { - buffer: &local_buffer, - offset: 0, - size: wgpu_types::BufferSize::new(size_of::() as _), + let local_buffer_binding = unsafe { + // SAFETY: The size must fit within the buffer. + hal::BufferBinding::new_unchecked( + &local_buffer, + 0, + wgpu_types::BufferSize::new(size_of::() as _).unwrap(), + ) }; let local_group_desc = hal::BindGroupDescriptor { label: Some("local"), diff --git a/wgpu-hal/examples/ray-traced-triangle/main.rs b/wgpu-hal/examples/ray-traced-triangle/main.rs index a8d3a77b9..93e687ff1 100644 --- a/wgpu-hal/examples/ray-traced-triangle/main.rs +++ b/wgpu-hal/examples/ray-traced-triangle/main.rs @@ -603,10 +603,13 @@ impl Example { let texture_view = unsafe { device.create_texture_view(&texture, &view_desc).unwrap() }; let bind_group = { - let buffer_binding = hal::BufferBinding { - buffer: &uniform_buffer, - offset: 0, - size: None, + let buffer_binding = unsafe { + // SAFETY: The size matches the buffer allocation. + hal::BufferBinding::new_unchecked( + &uniform_buffer, + 0, + wgpu_types::BufferSize::new_unchecked(uniforms_size as u64), + ) }; let texture_binding = hal::TextureBinding { view: &texture_view, diff --git a/wgpu-hal/src/gles/device.rs b/wgpu-hal/src/gles/device.rs index 0f36f734b..0b5718cf0 100644 --- a/wgpu-hal/src/gles/device.rs +++ b/wgpu-hal/src/gles/device.rs @@ -534,7 +534,6 @@ impl crate::Device for super::Device { return Ok(super::Buffer { raw: None, target, - size: desc.size, map_flags: 0, data: Some(Arc::new(MaybeMutex::new(vec![0; desc.size as usize]))), offset_of_current_mapping: Arc::new(MaybeMutex::new(0)), @@ -634,7 +633,6 @@ impl crate::Device for super::Device { Ok(super::Buffer { raw, target, - size: desc.size, map_flags, data, offset_of_current_mapping: Arc::new(MaybeMutex::new(0)), @@ -1265,11 +1263,8 @@ impl crate::Device for super::Device { let bb = &desc.buffers[entry.resource_index as usize]; super::RawBinding::Buffer { raw: bb.buffer.raw.unwrap(), - offset: bb.offset as i32, - size: match bb.size { - Some(s) => s.get() as i32, - None => (bb.buffer.size - bb.offset) as i32, - }, + offset: bb.offset.try_into().unwrap(), + size: bb.size.get().try_into().unwrap(), } } wgt::BindingType::Sampler { .. } => { diff --git a/wgpu-hal/src/gles/mod.rs b/wgpu-hal/src/gles/mod.rs index a6073b4ec..c1b226f8c 100644 --- a/wgpu-hal/src/gles/mod.rs +++ b/wgpu-hal/src/gles/mod.rs @@ -342,7 +342,6 @@ impl Drop for Queue { pub struct Buffer { raw: Option, target: BindTarget, - size: wgt::BufferAddress, map_flags: u32, data: Option>>>, offset_of_current_mapping: Arc>, diff --git a/wgpu-hal/src/lib.rs b/wgpu-hal/src/lib.rs index 6f05edbb1..65e42180d 100644 --- a/wgpu-hal/src/lib.rs +++ b/wgpu-hal/src/lib.rs @@ -1968,6 +1968,13 @@ pub struct PipelineLayoutDescriptor<'a, B: DynBindGroupLayout + ?Sized> { /// /// [`BindGroup`]: Api::BindGroup /// +/// ## Construction +/// +/// The recommended way to construct a `BufferBinding` is using the `binding` +/// method on a wgpu-core `Buffer`, which will validate the binding size +/// against the buffer size. An unsafe `new_unchecked` constructor is also +/// provided for cases where direct construction is necessary. +/// /// ## Accessible region /// /// `wgpu_hal` guarantees that shaders compiled with @@ -1992,39 +1999,48 @@ pub struct PipelineLayoutDescriptor<'a, B: DynBindGroupLayout + ?Sized> { /// parts of which buffers shaders might observe. This optimization is only /// sound if shader access is bounds-checked. /// +/// ## Zero-length bindings +/// +/// Some back ends cannot tolerate zero-length regions; for example, see +/// [VUID-VkDescriptorBufferInfo-offset-00340][340] and +/// [VUID-VkDescriptorBufferInfo-range-00341][341], or the +/// documentation for GLES's [glBindBufferRange][bbr]. For this reason, a valid +/// `BufferBinding` must have `offset` strictly less than the size of the +/// buffer. +/// +/// WebGPU allows zero-length bindings, and there is not currently a mechanism +/// in place +/// /// [`buffer`]: BufferBinding::buffer /// [`offset`]: BufferBinding::offset /// [`size`]: BufferBinding::size /// [`Storage`]: wgt::BufferBindingType::Storage /// [`Uniform`]: wgt::BufferBindingType::Uniform +/// [340]: https://registry.khronos.org/vulkan/specs/1.3-extensions/html/vkspec.html#VUID-VkDescriptorBufferInfo-offset-00340 +/// [341]: https://registry.khronos.org/vulkan/specs/1.3-extensions/html/vkspec.html#VUID-VkDescriptorBufferInfo-range-00341 +/// [bbr]: https://registry.khronos.org/OpenGL-Refpages/es3.0/html/glBindBufferRange.xhtml /// [woob]: https://gpuweb.github.io/gpuweb/wgsl/#out-of-bounds-access-sec #[derive(Debug)] pub struct BufferBinding<'a, B: DynBuffer + ?Sized> { /// The buffer being bound. - pub buffer: &'a B, + /// + /// This is not fully `pub` to prevent direct construction of + /// `BufferBinding`s, while still allowing public read access to the `offset` + /// and `size` properties. + pub(crate) buffer: &'a B, /// The offset at which the bound region starts. /// - /// This must be less than the size of the buffer. Some back ends - /// cannot tolerate zero-length regions; for example, see - /// [VUID-VkDescriptorBufferInfo-offset-00340][340] and - /// [VUID-VkDescriptorBufferInfo-range-00341][341], or the - /// documentation for GLES's [glBindBufferRange][bbr]. - /// - /// [340]: https://registry.khronos.org/vulkan/specs/1.3-extensions/html/vkspec.html#VUID-VkDescriptorBufferInfo-offset-00340 - /// [341]: https://registry.khronos.org/vulkan/specs/1.3-extensions/html/vkspec.html#VUID-VkDescriptorBufferInfo-range-00341 - /// [bbr]: https://registry.khronos.org/OpenGL-Refpages/es3.0/html/glBindBufferRange.xhtml + /// Because zero-length bindings are not permitted (see above), this must be + /// strictly less than the size of the buffer. pub offset: wgt::BufferAddress, /// The size of the region bound, in bytes. - /// - /// If `None`, the region extends from `offset` to the end of the - /// buffer. Given the restrictions on `offset`, this means that - /// the size is always greater than zero. - pub size: Option, + pub size: wgt::BufferSize, } -impl<'a, T: DynBuffer + ?Sized> Clone for BufferBinding<'a, T> { +// We must implement this manually because `B` is not necessarily `Clone`. +impl Clone for BufferBinding<'_, B> { fn clone(&self) -> Self { BufferBinding { buffer: self.buffer, @@ -2034,6 +2050,31 @@ impl<'a, T: DynBuffer + ?Sized> Clone for BufferBinding<'a, T> { } } +impl<'a, B: DynBuffer + ?Sized> BufferBinding<'a, B> { + /// Construct a `BufferBinding` with the given contents. + /// + /// When possible, use the `binding` method on a wgpu-core `Buffer` instead + /// of this method. `Buffer::binding` validates the size of the binding + /// against the size of the buffer. + /// + /// It is more difficult to provide a validating constructor here, due to + /// not having direct access to the size of a `DynBuffer`. + /// + /// SAFETY: The caller is responsible for ensuring that a binding of `size` + /// bytes starting at `offset` is contained within the buffer. + pub unsafe fn new_unchecked( + buffer: &'a B, + offset: wgt::BufferAddress, + size: wgt::BufferSize, + ) -> Self { + Self { + buffer, + offset, + size, + } + } +} + #[derive(Debug)] pub struct TextureBinding<'a, T: DynTextureView + ?Sized> { pub view: &'a T, diff --git a/wgpu-hal/src/metal/command.rs b/wgpu-hal/src/metal/command.rs index 72a799a02..4fc1987ce 100644 --- a/wgpu-hal/src/metal/command.rs +++ b/wgpu-hal/src/metal/command.rs @@ -977,15 +977,9 @@ impl crate::CommandEncoder for super::CommandEncoder { let encoder = self.state.render.as_ref().unwrap(); encoder.set_vertex_buffer(buffer_index, Some(&binding.buffer.raw), binding.offset); - let buffer_size = binding.resolve_size(); - if buffer_size > 0 { - self.state.vertex_buffer_size_map.insert( - buffer_index, - core::num::NonZeroU64::new(buffer_size).unwrap(), - ); - } else { - self.state.vertex_buffer_size_map.remove(&buffer_index); - } + self.state + .vertex_buffer_size_map + .insert(buffer_index, binding.size); if let Some((index, sizes)) = self .state diff --git a/wgpu-hal/src/metal/device.rs b/wgpu-hal/src/metal/device.rs index ef8e7c83a..3835fd022 100644 --- a/wgpu-hal/src/metal/device.rs +++ b/wgpu-hal/src/metal/device.rs @@ -340,10 +340,6 @@ impl super::Device { } } - pub unsafe fn buffer_from_raw(raw: metal::Buffer, size: wgt::BufferAddress) -> super::Buffer { - super::Buffer { raw, size } - } - pub fn raw_device(&self) -> &Mutex { &self.shared.device } @@ -373,10 +369,7 @@ impl crate::Device for super::Device { raw.set_label(label); } self.counters.buffers.add(1); - Ok(super::Buffer { - raw, - size: desc.size, - }) + Ok(super::Buffer { raw }) }) } unsafe fn destroy_buffer(&self, _buffer: super::Buffer) { @@ -935,14 +928,9 @@ impl crate::Device for super::Device { let end = start + 1; bg.buffers .extend(desc.buffers[start..end].iter().map(|source| { - // Given the restrictions on `BufferBinding::offset`, - // this should never be `None`. - let remaining_size = wgt::BufferSize::new( - source.buffer.size - source.offset, - ); let binding_size = match ty { wgt::BufferBindingType::Storage { .. } => { - source.size.or(remaining_size) + Some(source.size) } _ => None, }; diff --git a/wgpu-hal/src/metal/mod.rs b/wgpu-hal/src/metal/mod.rs index b5ae1dd5d..30af14a33 100644 --- a/wgpu-hal/src/metal/mod.rs +++ b/wgpu-hal/src/metal/mod.rs @@ -502,7 +502,6 @@ impl crate::Queue for Queue { #[derive(Debug)] pub struct Buffer { raw: metal::Buffer, - size: wgt::BufferAddress, } unsafe impl Send for Buffer {} @@ -516,15 +515,6 @@ impl Buffer { } } -impl crate::BufferBinding<'_, Buffer> { - fn resolve_size(&self) -> wgt::BufferAddress { - match self.size { - Some(size) => size.get(), - None => self.buffer.size - self.offset, - } - } -} - #[derive(Debug)] pub struct Texture { raw: metal::Texture, diff --git a/wgpu-hal/src/vulkan/device.rs b/wgpu-hal/src/vulkan/device.rs index 81f523929..1c9ad8d93 100644 --- a/wgpu-hal/src/vulkan/device.rs +++ b/wgpu-hal/src/vulkan/device.rs @@ -1802,9 +1802,7 @@ impl crate::Device for super::Device { vk::DescriptorBufferInfo::default() .buffer(binding.buffer.raw) .offset(binding.offset) - .range( - binding.size.map_or(vk::WHOLE_SIZE, wgt::BufferSize::get), - ) + .range(binding.size.get()) }, )); write.buffer_info(local_buffer_infos) diff --git a/wgpu/src/api/device.rs b/wgpu/src/api/device.rs index 99ed5071d..224c688fd 100644 --- a/wgpu/src/api/device.rs +++ b/wgpu/src/api/device.rs @@ -322,6 +322,7 @@ impl Device { /// - `hal_buffer` must be created from this device internal handle /// - `hal_buffer` must be created respecting `desc` /// - `hal_buffer` must be initialized + /// - `hal_buffer` must not have zero size #[cfg(wgpu_core)] #[must_use] pub unsafe fn create_buffer_from_hal( diff --git a/wgpu/src/backend/wgpu_core.rs b/wgpu/src/backend/wgpu_core.rs index 6a86d44b2..87573dae3 100644 --- a/wgpu/src/backend/wgpu_core.rs +++ b/wgpu/src/backend/wgpu_core.rs @@ -170,6 +170,12 @@ impl ContextWgpuCore { } } + /// # Safety + /// + /// - `hal_buffer` must be created from `device`. + /// - `hal_buffer` must be created respecting `desc` + /// - `hal_buffer` must be initialized + /// - `hal_buffer` must not have zero size. pub unsafe fn create_buffer_from_hal( &self, hal_buffer: A::Buffer,