Merge #757

757: Validate Dynamic Bindings are In-Bounds r=kvark a=cwfitzgerald

**Connections**

Closes #756 

**Description**

Needed to store some basic info about all dynamic bindings in the bind group.

I wasn't exactly sure where to put the validation function, so I stuck it as an inherent function on the BindGroup. Can be moved if this isn't ideal.

I also moved offset alignment validation into the validation function to help reduce duplication between render/compute.

**Testing**

None of the examples use dynamic indexing, so it's hard to test this as is.

@FlorianUekermann could you possibly test this patch with your repro case?

Co-authored-by: Connor Fitzgerald <connorwadefitzgerald@gmail.com>

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<B: hal::Backend> {
     pub(crate) raw: DescriptorSet<B>,
@@ -121,7 +138,55 @@ pub struct BindGroup<B: hal::Backend> {
     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<BindGroupDynamicBindingData>,
+}
+
+impl<B: hal::Backend> BindGroup<B> {
+    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<B: hal::Backend> Borrow<RefCount> for BindGroup<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);
 

wgpu-core/src/command/bundle.rs

@@ -546,7 +546,7 @@ impl<G: GlobalIdentityHandlerFactory> Global<G> {
                             .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);

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<G: GlobalIdentityHandlerFactory> Global<G> {
                     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 {:?}",

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<G: GlobalIdentityHandlerFactory> Global<G> {
                 } => {
                     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);
 

wgpu-core/src/device/mod.rs

@@ -1477,6 +1477,10 @@ impl<G: GlobalIdentityHandlerFactory> Global<G> {
         // 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<G: GlobalIdentityHandlerFactory> Global<G> {
                     .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<G: GlobalIdentityHandlerFactory> Global<G> {
                                     resource::BufferUse::STORAGE_LOAD
                                 },
                                 min_binding_size,
+                                dynamic,
                             ),
                             _ => {
                                 return Err(BindGroupError::WrongBindingType {
@@ -1545,7 +1551,7 @@ impl<G: GlobalIdentityHandlerFactory> Global<G> {
                             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<G: GlobalIdentityHandlerFactory> Global<G> {
                                     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<G: GlobalIdentityHandlerFactory> Global<G> {
             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();