BACKPORT to 0.19: fix: don't depend on BG{,L} layout entry order in HAL

#5421 (#5455) Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
gfx-rs · Apr 17, 2024 · 95dc7e5 · 95dc7e5
1 parent 2516bc2
commit 95dc7e5
Show file tree

Hide file tree

Showing 6 changed files with 171 additions and 7 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -60,6 +60,19 @@ Bottom level categories:
         - `step`
         - `tan`
         - `tanh`
+### Bug Fixes
+
+#### GLES
+
+- Don't depend on bind group and bind group layout entry order in HAL. This caused incorrect severely incorrect command execution and, in some cases, crashes. By @ErichDonGubler in [#5421](https://github.com/gfx-rs/wgpu/pull/5421).
+
+#### Metal
+
+- Don't depend on bind group and bind group layout entry order in HAL. This caused incorrect severely incorrect command execution and, in some cases, crashes. By @ErichDonGubler in [#5421](https://github.com/gfx-rs/wgpu/pull/5421).
+
+#### DX12
+
+- Don't depend on bind group and bind group layout entry order in HAL. This caused incorrect severely incorrect command execution and, in some cases, crashes. By @ErichDonGubler in [#5421](https://github.com/gfx-rs/wgpu/pull/5421).
 
 ## v0.19.3 (2024-03-01)
 

diff --git a/Cargo.lock b/Cargo.lock
diff --git a/tests/tests/device.rs b/tests/tests/device.rs
@@ -615,3 +615,125 @@ static DROPPED_GLOBAL_THEN_DEVICE_LOST: GpuTestConfiguration = GpuTestConfigurat
             "Device lost callback should have been called."
         );
     });
+
+#[gpu_test]
+static DIFFERENT_BGL_ORDER_BW_SHADER_AND_API: GpuTestConfiguration = GpuTestConfiguration::new()
+    .parameters(TestParameters::default())
+    .run_sync(|ctx| {
+        // This test addresses a bug found in multiple backends where `wgpu_core` and `wgpu_hal`
+        // backends made different assumptions about the element order of vectors of bind group
+        // layout entries and bind group resource bindings.
+        //
+        // Said bug was exposed originally by:
+        //
+        // 1. Shader-declared bindings having a different order than resource bindings provided to
+        //    `Device::create_bind_group`.
+        // 2. Having more of one type of resource in the bind group than another.
+        //
+        // …such that internals would accidentally attempt to use an out-of-bounds index (of one
+        // resource type) in the wrong list of a different resource type. Let's reproduce that
+        // here.
+
+        let trivial_shaders_with_some_reversed_bindings = "\
+@group(0) @binding(3) var myTexture2: texture_2d<f32>;
+@group(0) @binding(2) var myTexture1: texture_2d<f32>;
+@group(0) @binding(1) var mySampler: sampler;
+
+@fragment
+fn fs_main(@builtin(position) pos: vec4<f32>) -> @location(0) vec4f {
+  return textureSample(myTexture1, mySampler, pos.xy) + textureSample(myTexture2, mySampler, pos.xy);
+}
+
+@vertex
+fn vs_main() -> @builtin(position) vec4<f32> {
+  return vec4<f32>(0.0, 0.0, 0.0, 1.0);
+}
+";
+
+        let trivial_shaders_with_some_reversed_bindings =
+            ctx.device
+                .create_shader_module(wgpu::ShaderModuleDescriptor {
+                    label: None,
+                    source: wgpu::ShaderSource::Wgsl(
+                        trivial_shaders_with_some_reversed_bindings.into(),
+                    ),
+                });
+
+        let my_texture = ctx.device.create_texture(&wgt::TextureDescriptor {
+            label: None,
+            size: wgt::Extent3d {
+                width: 1024,
+                height: 512,
+                depth_or_array_layers: 1,
+            },
+            mip_level_count: 1,
+            sample_count: 1,
+            dimension: wgt::TextureDimension::D2,
+            format: wgt::TextureFormat::Rgba8Unorm,
+            usage: wgt::TextureUsages::RENDER_ATTACHMENT | wgt::TextureUsages::TEXTURE_BINDING,
+            view_formats: &[],
+        });
+
+        let my_texture_view = my_texture.create_view(&wgpu::TextureViewDescriptor {
+            label: None,
+            format: None,
+            dimension: None,
+            aspect: wgt::TextureAspect::All,
+            base_mip_level: 0,
+            mip_level_count: None,
+            base_array_layer: 0,
+            array_layer_count: None,
+        });
+
+        let my_sampler = ctx
+            .device
+            .create_sampler(&wgpu::SamplerDescriptor::default());
+
+        let render_pipeline = ctx
+            .device
+            .create_render_pipeline(&wgpu::RenderPipelineDescriptor {
+                fragment: Some(wgpu::FragmentState {
+                    module: &trivial_shaders_with_some_reversed_bindings,
+                    entry_point: "fs_main",
+                    targets: &[Some(wgt::ColorTargetState {
+                        format: wgt::TextureFormat::Bgra8Unorm,
+                        blend: None,
+                        write_mask: wgt::ColorWrites::ALL,
+                    })],
+                }),
+                layout: None,
+
+                // Other fields below aren't interesting for this text.
+                label: None,
+                vertex: wgpu::VertexState {
+                    module: &trivial_shaders_with_some_reversed_bindings,
+                    entry_point: "vs_main",
+                    buffers: &[],
+                },
+                primitive: wgt::PrimitiveState::default(),
+                depth_stencil: None,
+                multisample: wgt::MultisampleState::default(),
+                multiview: None,
+            });
+
+        // fail(&ctx.device, || {
+        // }, "");
+        ctx.device.create_bind_group(&wgpu::BindGroupDescriptor {
+            label: None,
+            layout: &render_pipeline.get_bind_group_layout(0),
+            entries: &[
+                wgpu::BindGroupEntry {
+                    binding: 1,
+                    resource: wgpu::BindingResource::Sampler(&my_sampler),
+                },
+                wgpu::BindGroupEntry {
+                    binding: 2,
+                    resource: wgpu::BindingResource::TextureView(&my_texture_view),
+                },
+                wgpu::BindGroupEntry {
+                    binding: 3,
+                    resource: wgpu::BindingResource::TextureView(&my_texture_view),
+                },
+            ],
+        });
+    });
diff --git a/wgpu-hal/src/dx12/device.rs b/wgpu-hal/src/dx12/device.rs
@@ -1098,7 +1098,16 @@ impl crate::Device<super::Api> for super::Device {
         }
         let mut dynamic_buffers = Vec::new();
 
-        for (layout, entry) in desc.layout.entries.iter().zip(desc.entries.iter()) {
+        let layout_and_entry_iter = desc.entries.iter().map(|entry| {
+            let layout = desc
+                .layout
+                .entries
+                .iter()
+                .find(|layout_entry| layout_entry.binding == entry.binding)
+                .expect("internal error: no layout entry found with binding slot");
+            (layout, entry)
+        });
+        for (layout, entry) in layout_and_entry_iter {
             match layout.ty {
                 wgt::BindingType::Buffer {
                     has_dynamic_offset: true,

diff --git a/wgpu-hal/src/gles/device.rs b/wgpu-hal/src/gles/device.rs
@@ -1123,8 +1123,10 @@ impl crate::Device<super::Api> for super::Device {
                 !0;
                 bg_layout
                     .entries
-                    .last()
-                    .map_or(0, |b| b.binding as usize + 1)
+                    .iter()
+                    .map(|b| b.binding)
+                    .max()
+                    .map_or(0, |idx| idx as usize + 1)
             ]
             .into_boxed_slice();
 
@@ -1177,7 +1179,16 @@ impl crate::Device<super::Api> for super::Device {
     ) -> Result<super::BindGroup, crate::DeviceError> {
         let mut contents = Vec::new();
 
-        for (entry, layout) in desc.entries.iter().zip(desc.layout.entries.iter()) {
+        let layout_and_entry_iter = desc.entries.iter().map(|entry| {
+            let layout = desc
+                .layout
+                .entries
+                .iter()
+                .find(|layout_entry| layout_entry.binding == entry.binding)
+                .expect("internal error: no layout entry found with binding slot");
+            (entry, layout)
+        });
+        for (entry, layout) in layout_and_entry_iter {
             let binding = match layout.ty {
                 wgt::BindingType::Buffer { .. } => {
                     let bb = &desc.buffers[entry.resource_index as usize];

diff --git a/wgpu-hal/src/metal/device.rs b/wgpu-hal/src/metal/device.rs
@@ -706,7 +706,16 @@ impl crate::Device<super::Api> for super::Device {
         for (&stage, counter) in super::NAGA_STAGES.iter().zip(bg.counters.iter_mut()) {
             let stage_bit = map_naga_stage(stage);
             let mut dynamic_offsets_count = 0u32;
-            for (entry, layout) in desc.entries.iter().zip(desc.layout.entries.iter()) {
+            let layout_and_entry_iter = desc.entries.iter().map(|entry| {
+                let layout = desc
+                    .layout
+                    .entries
+                    .iter()
+                    .find(|layout_entry| layout_entry.binding == entry.binding)
+                    .expect("internal error: no layout entry found with binding slot");
+                (entry, layout)
+            });
+            for (entry, layout) in layout_and_entry_iter {
                 let size = layout.count.map_or(1, |c| c.get());
                 if let wgt::BindingType::Buffer {
                     has_dynamic_offset: true,