From b4f2c029415954b684dfa1f603d23793eff9e273 Mon Sep 17 00:00:00 2001 From: Connor Fitzgerald Date: Tue, 30 Jun 2020 12:35:49 -0400 Subject: [PATCH] Validate dynamic bindings are in-bounds --- wgpu-core/src/binding_model.rs | 67 +++++++++++++++++++++++++++++++- wgpu-core/src/command/bind.rs | 1 - wgpu-core/src/command/bundle.rs | 2 +- wgpu-core/src/command/compute.rs | 14 +------ wgpu-core/src/command/render.rs | 13 +------ wgpu-core/src/device/mod.rs | 28 +++++++++---- 6 files changed, 92 insertions(+), 33 deletions(-) diff --git a/wgpu-core/src/binding_model.rs b/wgpu-core/src/binding_model.rs index 6c5b939d5f..1e41de370f 100644 --- a/wgpu-core/src/binding_model.rs +++ b/wgpu-core/src/binding_model.rs @@ -114,6 +114,23 @@ pub struct BindGroupDescriptor<'a> { pub bindings: &'a [BindGroupEntry<'a>], } +#[derive(Debug)] +pub enum BindError { + /// Number of dynamic offsets doesn't match the number of dynamic bindings + /// in the bind group layout. + MismatchedDynamicOffsetCount { actual: usize, expected: usize }, + /// Expected dynamic binding alignment was not met. + UnalignedDynamicBinding { idx: usize }, + /// Dynamic offset would cause buffer overrun. + DynamicBindingOutOfBounds { idx: usize }, +} + +#[derive(Debug)] +pub struct BindGroupDynamicBindingData { + /// The maximum value the dynamic offset can have before running off the end of the buffer. + pub(crate) maximum_dynamic_offset: wgt::BufferAddress, +} + #[derive(Debug)] pub struct BindGroup { pub(crate) raw: DescriptorSet, @@ -121,7 +138,55 @@ pub struct BindGroup { pub(crate) layout_id: BindGroupLayoutId, pub(crate) life_guard: LifeGuard, pub(crate) used: TrackerSet, - pub(crate) dynamic_count: usize, + pub(crate) dynamic_binding_info: Vec, +} + +impl BindGroup { + pub(crate) fn validate_dynamic_bindings( + &self, + offsets: &[wgt::DynamicOffset], + ) -> Result<(), BindError> { + if self.dynamic_binding_info.len() != offsets.len() { + log::error!( + "BindGroup has {} dynamic bindings, but {} dynamic offsets were provided", + self.dynamic_binding_info.len(), + offsets.len() + ); + return Err(BindError::MismatchedDynamicOffsetCount { + expected: self.dynamic_binding_info.len(), + actual: offsets.len(), + }); + } + + for (idx, (info, &offset)) in self + .dynamic_binding_info + .iter() + .zip(offsets.iter()) + .enumerate() + { + if offset as wgt::BufferAddress % wgt::BIND_BUFFER_ALIGNMENT != 0 { + log::error!( + "Dynamic buffer offset index {}: {} needs to be aligned as a multiple of {}", + idx, + offset, + wgt::BIND_BUFFER_ALIGNMENT + ); + return Err(BindError::UnalignedDynamicBinding { idx }); + } + + if offset as wgt::BufferAddress > info.maximum_dynamic_offset { + log::error!( + "Dynamic offset index {} with value {} overruns underlying buffer. Dynamic offset must be no more than {}", + idx, + offset, + info.maximum_dynamic_offset, + ); + return Err(BindError::DynamicBindingOutOfBounds { idx }); + } + } + + Ok(()) + } } impl Borrow for BindGroup { diff --git a/wgpu-core/src/command/bind.rs b/wgpu-core/src/command/bind.rs index 375f0b78b3..31a45afe1b 100644 --- a/wgpu-core/src/command/bind.rs +++ b/wgpu-core/src/command/bind.rs @@ -84,7 +84,6 @@ impl BindGroupEntry { ref_count: bind_group.life_guard.add_ref(), }, }); - //TODO: validate the count of dynamic offsets to match the layout self.dynamic_offsets.clear(); self.dynamic_offsets.extend_from_slice(offsets); diff --git a/wgpu-core/src/command/bundle.rs b/wgpu-core/src/command/bundle.rs index 0987155181..cfb7cc4a20 100644 --- a/wgpu-core/src/command/bundle.rs +++ b/wgpu-core/src/command/bundle.rs @@ -546,7 +546,7 @@ impl Global { .bind_groups .use_extend(&*bind_group_guard, bind_group_id, (), ()) .unwrap(); - assert_eq!(bind_group.dynamic_count, offsets.len()); + assert_eq!(bind_group.dynamic_binding_info.len(), offsets.len()); state.set_bind_group(index, bind_group_id, bind_group.layout_id, offsets); state.trackers.merge_extend(&bind_group.used); diff --git a/wgpu-core/src/command/compute.rs b/wgpu-core/src/command/compute.rs index 0465ee11d2..83ab55451c 100644 --- a/wgpu-core/src/command/compute.rs +++ b/wgpu-core/src/command/compute.rs @@ -15,7 +15,7 @@ use crate::{ }; use hal::command::CommandBuffer as _; -use wgt::{BufferAddress, BufferUsage, BIND_BUFFER_ALIGNMENT}; +use wgt::{BufferAddress, BufferUsage}; use std::{fmt, iter, str}; @@ -160,22 +160,12 @@ impl Global { let offsets = &base.dynamic_offsets[..num_dynamic_offsets as usize]; base.dynamic_offsets = &base.dynamic_offsets[num_dynamic_offsets as usize..]; - for off in offsets { - assert_eq!( - *off as BufferAddress % BIND_BUFFER_ALIGNMENT, - 0, - "Misaligned dynamic buffer offset: {} does not align with {}", - off, - BIND_BUFFER_ALIGNMENT - ); - } - let bind_group = cmb .trackers .bind_groups .use_extend(&*bind_group_guard, bind_group_id, (), ()) .unwrap(); - assert_eq!(bind_group.dynamic_count, offsets.len()); + bind_group.validate_dynamic_bindings(offsets).unwrap(); log::trace!( "Encoding barriers on binding of {:?} to {:?}", diff --git a/wgpu-core/src/command/render.rs b/wgpu-core/src/command/render.rs index a7c080e21c..23314e2a94 100644 --- a/wgpu-core/src/command/render.rs +++ b/wgpu-core/src/command/render.rs @@ -26,7 +26,7 @@ use hal::command::CommandBuffer as _; use wgt::{ BufferAddress, BufferSize, BufferUsage, Color, IndexFormat, InputStepMode, LoadOp, RenderPassColorAttachmentDescriptorBase, RenderPassDepthStencilAttachmentDescriptorBase, - StoreOp, TextureUsage, BIND_BUFFER_ALIGNMENT, + StoreOp, TextureUsage, }; use std::{borrow::Borrow, collections::hash_map::Entry, fmt, iter, ops::Range, str}; @@ -877,21 +877,12 @@ impl Global { } => { let offsets = &base.dynamic_offsets[..num_dynamic_offsets as usize]; base.dynamic_offsets = &base.dynamic_offsets[num_dynamic_offsets as usize..]; - for off in offsets { - assert_eq!( - *off as BufferAddress % BIND_BUFFER_ALIGNMENT, - 0, - "Misaligned dynamic buffer offset: {} does not align with {}", - off, - BIND_BUFFER_ALIGNMENT - ); - } let bind_group = trackers .bind_groups .use_extend(&*bind_group_guard, bind_group_id, (), ()) .unwrap(); - assert_eq!(bind_group.dynamic_count, offsets.len()); + bind_group.validate_dynamic_bindings(offsets).unwrap(); trackers.merge_extend(&bind_group.used); diff --git a/wgpu-core/src/device/mod.rs b/wgpu-core/src/device/mod.rs index 17da787a96..b4b9834686 100644 --- a/wgpu-core/src/device/mod.rs +++ b/wgpu-core/src/device/mod.rs @@ -1477,6 +1477,10 @@ impl Global { // Rebind `desc_set` as immutable let desc_set = desc_set; + // TODO: arrayvec/smallvec + // Record binding info for dynamic offset validation + let mut dynamic_binding_info = Vec::new(); + // fill out the descriptors let mut used = TrackerSet::new(B::VARIANT); { @@ -1496,17 +1500,18 @@ impl Global { .ok_or(BindGroupError::MissingBindingDeclaration(binding))?; let descriptors: SmallVec<[_; 1]> = match b.resource { Br::Buffer(ref bb) => { - let (pub_usage, internal_use, min_size) = match decl.ty { + let (pub_usage, internal_use, min_size, dynamic) = match decl.ty { wgt::BindingType::UniformBuffer { - dynamic: _, + dynamic, min_binding_size, } => ( wgt::BufferUsage::UNIFORM, resource::BufferUse::UNIFORM, min_binding_size, + dynamic, ), wgt::BindingType::StorageBuffer { - dynamic: _, + dynamic, min_binding_size, readonly, } => ( @@ -1517,6 +1522,7 @@ impl Global { resource::BufferUse::STORAGE_LOAD }, min_binding_size, + dynamic, ), _ => { return Err(BindGroupError::WrongBindingType { @@ -1545,7 +1551,7 @@ impl Global { buffer.usage, pub_usage ); - let bind_size = match bb.size { + let (bind_size, bind_end) = match bb.size { Some(size) => { let end = bb.offset + size.get(); assert!( @@ -1554,10 +1560,18 @@ impl Global { bb.offset..end, buffer.size ); - size.get() + (size.get(), end) } - None => buffer.size - bb.offset, + None => (buffer.size - bb.offset, buffer.size), }; + + // Record binding info for validating dynamic offsets + if dynamic { + dynamic_binding_info.push(binding_model::BindGroupDynamicBindingData { + maximum_dynamic_offset: buffer.size - bind_end, + }); + } + match min_size { Some(non_zero) if non_zero.get() > bind_size => panic!( "Minimum buffer binding size {} is not respected with size {}", @@ -1750,7 +1764,7 @@ impl Global { layout_id: desc.layout, life_guard: LifeGuard::new(), used, - dynamic_count: bind_group_layout.dynamic_count, + dynamic_binding_info, }; let ref_count = bind_group.life_guard.add_ref();