From b648c9039c49cf9664fe2f9a00e770090173b575 Mon Sep 17 00:00:00 2001
From: Antonio Maiorano <amaiorano@google.com>
Date: Sat, 30 May 2026 08:34:25 -0700
Subject: [PATCH] [native] Fix IndirectDrawValidationEncoder merging non-matching passes

If two passes had different kDuplicateBaseVertexInstance values, the
encoder would erroneously merge them.

Fixed by making sure to take this flag into account. To help avoid this
happening in the future should we add or modify more fields to the
config, I added a constructor to force all fields to be passed in. The
comparison code now inline constructs the config object and compares it
use the equality operator.

Fixes: 513948465
Change-Id: I9e22feb9d6f407126c604a872ddd5c45aab4ef04
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/311995
Reviewed-by: Brandon Jones <bajones@chromium.org>
Commit-Queue: Antonio Maiorano <amaiorano@google.com>
---

diff --git a/src/dawn/native/IndirectDrawMetadata.cpp b/src/dawn/native/IndirectDrawMetadata.cpp
index 2353dd6..8073170 100644
--- a/src/dawn/native/IndirectDrawMetadata.cpp
+++ b/src/dawn/native/IndirectDrawMetadata.cpp
@@ -261,8 +261,8 @@
             DAWN_UNREACHABLE();
     }
 
-    const IndexedIndirectConfig config = {reinterpret_cast<uintptr_t>(indirectBuffer),
-                                          duplicateBaseVertexInstance, DrawType::Indexed};
+    const IndexedIndirectConfig config{reinterpret_cast<uintptr_t>(indirectBuffer),
+                                       duplicateBaseVertexInstance, DrawType::Indexed};
     auto it = mIndexedIndirectBufferValidationInfo.find(config);
     if (it == mIndexedIndirectBufferValidationInfo.end()) {
         auto result = mIndexedIndirectBufferValidationInfo.emplace(
@@ -282,8 +282,8 @@
                                            uint64_t indirectOffset,
                                            bool duplicateBaseVertexInstance,
                                            DrawIndirectCmd* cmd) {
-    const IndexedIndirectConfig config = {reinterpret_cast<uintptr_t>(indirectBuffer),
-                                          duplicateBaseVertexInstance, DrawType::NonIndexed};
+    const IndexedIndirectConfig config{reinterpret_cast<uintptr_t>(indirectBuffer),
+                                       duplicateBaseVertexInstance, DrawType::NonIndexed};
     auto it = mIndexedIndirectBufferValidationInfo.find(config);
     if (it == mIndexedIndirectBufferValidationInfo.end()) {
         auto result = mIndexedIndirectBufferValidationInfo.emplace(
diff --git a/src/dawn/native/IndirectDrawMetadata.h b/src/dawn/native/IndirectDrawMetadata.h
index b130bf3..77070d4 100644
--- a/src/dawn/native/IndirectDrawMetadata.h
+++ b/src/dawn/native/IndirectDrawMetadata.h
@@ -151,9 +151,16 @@
     };
 
     struct IndexedIndirectConfig {
-        uintptr_t inputIndirectBufferPtr = 0;
-        bool duplicateBaseVertexInstance = false;
-        DrawType drawType = DrawType::NonIndexed;
+        const uintptr_t inputIndirectBufferPtr;
+        const bool duplicateBaseVertexInstance;
+        const DrawType drawType;
+
+        IndexedIndirectConfig(uintptr_t inputIndirectBufferPtr,
+                              bool duplicateBaseVertexInstance,
+                              DrawType drawType)
+            : inputIndirectBufferPtr(inputIndirectBufferPtr),
+              duplicateBaseVertexInstance(duplicateBaseVertexInstance),
+              drawType(drawType) {}
 
         bool operator<(const IndexedIndirectConfig& other) const;
         bool operator==(const IndexedIndirectConfig& other) const = default;
diff --git a/src/dawn/native/IndirectDrawValidationEncoder.cpp b/src/dawn/native/IndirectDrawValidationEncoder.cpp
index 67eb215..15b8ba0 100644
--- a/src/dawn/native/IndirectDrawValidationEncoder.cpp
+++ b/src/dawn/native/IndirectDrawValidationEncoder.cpp
@@ -528,9 +528,10 @@
 
             Pass* currentPass = passes.empty() ? nullptr : &passes.back();
             if (currentPass &&
-                reinterpret_cast<uintptr_t>(currentPass->inputIndirectBuffer.get()) ==
-                    config.inputIndirectBufferPtr &&
-                currentPass->drawType == config.drawType) {
+                IndirectDrawMetadata::IndexedIndirectConfig{
+                    reinterpret_cast<uintptr_t>(currentPass->inputIndirectBuffer.get()),
+                    bool(currentPass->flags & kDuplicateBaseVertexInstance),
+                    currentPass->drawType} == config) {
                 uint64_t nextBatchDataOffset =
                     Align(currentPass->batchDataSize, minStorageBufferOffsetAlignment);
                 uint64_t newPassBatchDataSize = nextBatchDataOffset + newBatch.dataSize;
diff --git a/src/dawn/tests/end2end/DrawIndexedIndirectTests.cpp b/src/dawn/tests/end2end/DrawIndexedIndirectTests.cpp
index e029b05..34e6d1c 100644
--- a/src/dawn/tests/end2end/DrawIndexedIndirectTests.cpp
+++ b/src/dawn/tests/end2end/DrawIndexedIndirectTests.cpp
@@ -722,5 +722,114 @@
                       VulkanBackend(),
                       WebGPUBackend());
 
+class DrawIndexedIndirectTest_IndirectFirstInstance : public DawnTest {
+    std::vector<wgpu::FeatureName> GetRequiredFeatures() override {
+        return {wgpu::FeatureName::IndirectFirstInstance};
+    }
+};
+
+// Test that makes sure indirect-draw validation merges passes correctly.
+// A bug was found in D3D12 where it would merge passes despite kDuplicateBaseVertexInstance being
+// different between the passes. Note that the test did not reproduce a failure, which is likely due
+// to GPU driver robustness; however, the logic was indeed incorrect. Presumably on GPUs that do not
+// implement such robustness, an OOB read would be possible.
+TEST_P(DrawIndexedIndirectTest_IndirectFirstInstance, IndirectDrawValidationMergesMatchingPasses) {
+    DAWN_ASSERT(device.HasFeature(wgpu::FeatureName::IndirectFirstInstance));
+
+    // Test expects validation to be enabled
+    DAWN_TEST_UNSUPPORTED_IF(HasToggleEnabled("skip_validation"));
+
+    // Create pNo pipeline (no vertex_index)
+    wgpu::ShaderModule modNo = utils::CreateShaderModule(device, R"(
+        @vertex fn vs() -> @builtin(position) vec4f {
+            // No vertex_index / instance_index builtin -> dup=false on D3D12.
+            return vec4f(0.0, 0.0, 0.0, 1.0);
+        }
+        @fragment fn fs() -> @location(0) vec4f {
+            return vec4f(0.0, 0.0, 0.0, 1.0);
+        }
+    )");
+
+    // Create pDup pipeline (uses vertex_index)
+    wgpu::ShaderModule modDup = utils::CreateShaderModule(device, R"(
+        struct VOut {
+            @builtin(position) pos : vec4f,
+            @location(0) @interpolate(flat) vidx : u32,
+        };
+        @vertex fn vs(@builtin(vertex_index) vi : u32) -> VOut {
+            // Reading @builtin(vertex_index) sets usesVertexIndex=true in the
+            // compiled D3D12 shader, so this pipeline gets the 7-u32
+            // ExecuteIndirect signature. After the OOB index fetch, vi carries the
+            // OOB-read index value (SV_VertexID) -- exfiltratable here.
+            var o : VOut;
+            o.pos = vec4f(0.0, 0.0, 0.0, 1.0);
+            o.vidx = vi;
+            return o;
+        }
+        @fragment fn fs(in : VOut) -> @location(0) vec4f {
+            return vec4f(f32(in.vidx & 255u) / 255.0, 0.0, 0.0, 1.0);
+        }
+    )");
+
+    utils::BasicRenderPass renderPass = utils::CreateBasicRenderPass(device, kRTSize, kRTSize);
+
+    utils::ComboRenderPipelineDescriptor descNo;
+    descNo.vertex.module = modNo;
+    descNo.cFragment.module = modNo;
+    descNo.primitive.topology = wgpu::PrimitiveTopology::PointList;
+    descNo.cTargets[0].format = renderPass.colorFormat;
+    wgpu::RenderPipeline pNo = device.CreateRenderPipeline(&descNo);
+
+    utils::ComboRenderPipelineDescriptor descDup;
+    descDup.vertex.module = modDup;
+    descDup.cFragment.module = modDup;
+    descDup.primitive.topology = wgpu::PrimitiveTopology::PointList;
+    descDup.cTargets[0].format = renderPass.colorFormat;
+    wgpu::RenderPipeline pDup = device.CreateRenderPipeline(&descDup);
+
+    // Index buffer: 1 MiB uint16 -> numIndexBufferElements = 524288.
+    const uint32_t NUM_IDX = 524288;
+    std::vector<uint16_t> idxData(NUM_IDX);
+    for (size_t i = 0; i < NUM_IDX; ++i) {
+        idxData[i] = static_cast<uint16_t>(i);
+    }
+    wgpu::Buffer idxBuf = utils::CreateBufferFromData(
+        device, idxData.data(), idxData.size() * sizeof(uint16_t), wgpu::BufferUsage::Index);
+
+    // Indirect buffer with three 5-u32 DrawIndexedIndirect records.
+    wgpu::Buffer indBuf = utils::CreateBufferFromData<uint32_t>(
+        device, wgpu::BufferUsage::Indirect | wgpu::BufferUsage::CopyDst,
+        {0, 0, 0, 0, 0,                 // record 0: harmless no-op for pNo
+         0, 1, NUM_IDX, 1, 0xFFFFFFFF,  // record 1: dup=true config (merged)
+         0, 0, 0, 0, 0});               // record 2: zeros
+
+    wgpu::CommandEncoder enc = device.CreateCommandEncoder();
+    wgpu::RenderPassEncoder pass = enc.BeginRenderPass(&renderPass.renderPassInfo);
+    pass.SetIndexBuffer(idxBuf, wgpu::IndexFormat::Uint16);
+
+    pass.SetPipeline(pNo);
+    pass.DrawIndexedIndirect(indBuf, 0);
+
+    pass.SetPipeline(pDup);
+    pass.DrawIndexedIndirect(indBuf, 20);
+    pass.DrawIndexedIndirect(indBuf, 40);
+
+    pass.End();
+    wgpu::CommandBuffer commands = enc.Finish();
+    queue.Submit(1, &commands);
+
+    // If the bug triggers, the driver will read 7-u32 records and execute a draw with a large
+    // number of vertices, overdrawing at the center of the viewport. Since we expect no pixels to
+    // be drawn (indexCount = 0 in record 1), the center pixel should remain clear.
+    EXPECT_PIXEL_RGBA8_EQ(utils::RGBA8(0, 0, 0, 0), renderPass.color, 2, 2);
+}
+
+DAWN_INSTANTIATE_TEST(DrawIndexedIndirectTest_IndirectFirstInstance,

diff --git a/src/dawn/tests/end2end/DrawIndexedIndirectTests.cpp b/src/dawn/tests/end2end/DrawIndexedIndirectTests.cpp
index e029b05..34e6d1c 100644
--- a/src/dawn/tests/end2end/DrawIndexedIndirectTests.cpp
+++ b/src/dawn/tests/end2end/DrawIndexedIndirectTests.cpp
@@ -722,5 +722,114 @@
                       VulkanBackend(),
                       WebGPUBackend());
 
+class DrawIndexedIndirectTest_IndirectFirstInstance : public DawnTest {
+    std::vector<wgpu::FeatureName> GetRequiredFeatures() override {
+        return {wgpu::FeatureName::IndirectFirstInstance};
+    }
+};
+
+// Test that makes sure indirect-draw validation merges passes correctly.
+// A bug was found in D3D12 where it would merge passes despite kDuplicateBaseVertexInstance being
+// different between the passes. Note that the test did not reproduce a failure, which is likely due
+// to GPU driver robustness; however, the logic was indeed incorrect. Presumably on GPUs that do not
+// implement such robustness, an OOB read would be possible.
+TEST_P(DrawIndexedIndirectTest_IndirectFirstInstance, IndirectDrawValidationMergesMatchingPasses) {
+    DAWN_ASSERT(device.HasFeature(wgpu::FeatureName::IndirectFirstInstance));
+
+    // Test expects validation to be enabled
+    DAWN_TEST_UNSUPPORTED_IF(HasToggleEnabled("skip_validation"));
+
+    // Create pNo pipeline (no vertex_index)
+    wgpu::ShaderModule modNo = utils::CreateShaderModule(device, R"(
+        @vertex fn vs() -> @builtin(position) vec4f {
+            // No vertex_index / instance_index builtin -> dup=false on D3D12.
+            return vec4f(0.0, 0.0, 0.0, 1.0);
+        }
+        @fragment fn fs() -> @location(0) vec4f {
+            return vec4f(0.0, 0.0, 0.0, 1.0);
+        }
+    )");
+
+    // Create pDup pipeline (uses vertex_index)
+    wgpu::ShaderModule modDup = utils::CreateShaderModule(device, R"(
+        struct VOut {
+            @builtin(position) pos : vec4f,
+            @location(0) @interpolate(flat) vidx : u32,
+        };
+        @vertex fn vs(@builtin(vertex_index) vi : u32) -> VOut {
+            // Reading @builtin(vertex_index) sets usesVertexIndex=true in the
+            // compiled D3D12 shader, so this pipeline gets the 7-u32
+            // ExecuteIndirect signature. After the OOB index fetch, vi carries the
+            // OOB-read index value (SV_VertexID) -- exfiltratable here.
+            var o : VOut;
+            o.pos = vec4f(0.0, 0.0, 0.0, 1.0);
+            o.vidx = vi;
+            return o;
+        }
+        @fragment fn fs(in : VOut) -> @location(0) vec4f {
+            return vec4f(f32(in.vidx & 255u) / 255.0, 0.0, 0.0, 1.0);
+        }
+    )");
+
+    utils::BasicRenderPass renderPass = utils::CreateBasicRenderPass(device, kRTSize, kRTSize);
+
+    utils::ComboRenderPipelineDescriptor descNo;
+    descNo.vertex.module = modNo;
+    descNo.cFragment.module = modNo;
+    descNo.primitive.topology = wgpu::PrimitiveTopology::PointList;
+    descNo.cTargets[0].format = renderPass.colorFormat;
+    wgpu::RenderPipeline pNo = device.CreateRenderPipeline(&descNo);
+
+    utils::ComboRenderPipelineDescriptor descDup;
+    descDup.vertex.module = modDup;
+    descDup.cFragment.module = modDup;
+    descDup.primitive.topology = wgpu::PrimitiveTopology::PointList;
+    descDup.cTargets[0].format = renderPass.colorFormat;
+    wgpu::RenderPipeline pDup = device.CreateRenderPipeline(&descDup);
+
+    // Index buffer: 1 MiB uint16 -> numIndexBufferElements = 524288.
+    const uint32_t NUM_IDX = 524288;
+    std::vector<uint16_t> idxData(NUM_IDX);
+    for (size_t i = 0; i < NUM_IDX; ++i) {
+        idxData[i] = static_cast<uint16_t>(i);
+    }
+    wgpu::Buffer idxBuf = utils::CreateBufferFromData(
+        device, idxData.data(), idxData.size() * sizeof(uint16_t), wgpu::BufferUsage::Index);
+
+    // Indirect buffer with three 5-u32 DrawIndexedIndirect records.
+    wgpu::Buffer indBuf = utils::CreateBufferFromData<uint32_t>(
+        device, wgpu::BufferUsage::Indirect | wgpu::BufferUsage::CopyDst,
+        {0, 0, 0, 0, 0,                 // record 0: harmless no-op for pNo
+         0, 1, NUM_IDX, 1, 0xFFFFFFFF,  // record 1: dup=true config (merged)
+         0, 0, 0, 0, 0});               // record 2: zeros
+
+    wgpu::CommandEncoder enc = device.CreateCommandEncoder();
+    wgpu::RenderPassEncoder pass = enc.BeginRenderPass(&renderPass.renderPassInfo);
+    pass.SetIndexBuffer(idxBuf, wgpu::IndexFormat::Uint16);
+
+    pass.SetPipeline(pNo);
+    pass.DrawIndexedIndirect(indBuf, 0);
+
+    pass.SetPipeline(pDup);
+    pass.DrawIndexedIndirect(indBuf, 20);
+    pass.DrawIndexedIndirect(indBuf, 40);
+
+    pass.End();
+    wgpu::CommandBuffer commands = enc.Finish();
+    queue.Submit(1, &commands);
+
+    // If the bug triggers, the driver will read 7-u32 records and execute a draw with a large
+    // number of vertices, overdrawing at the center of the viewport. Since we expect no pixels to
+    // be drawn (indexCount = 0 in record 1), the center pixel should remain clear.
+    EXPECT_PIXEL_RGBA8_EQ(utils::RGBA8(0, 0, 0, 0), renderPass.color, 2, 2);
+}
+
+DAWN_INSTANTIATE_TEST(DrawIndexedIndirectTest_IndirectFirstInstance,
+                      D3D12Backend(),
+                      MetalBackend(),
+                      OpenGLBackend(),
+                      VulkanBackend(),
+                      WebGPUBackend());
+
 }  // anonymous namespace
 }  // namespace dawn