Skip to content

Commit 35d3602

Browse files
lunadogbotbartlomiejucrowlKats
authored
fix(ext/webgpu): bounds-check + view-aware setBindGroup Uint32Array fast path (#33980)
The `Uint32Array` fast path of `setBindGroup` in `compute_pass.rs`, `render_pass.rs` and `render_bundle.rs` had two related bugs (#33956): 1. `&data[start..(start + len)]` sliced unchecked. An out-of-range length panicked inside the op's `extern "C"` callback, and the panic crossed the C ABI as a process abort — six lines of JS could kill a running runtime. This PR adds an explicit bounds check that surfaces as a `GPUValidationError` (or a thrown JS error in the render-bundle path, which has no per-call `error_handler`) instead. 2. `uint_32.buffer(scope).data()` + `ab.byte_length() / 4` walked the backing `ArrayBuffer` and ignored the typed-array view's `byteOffset` and `length`. A `Uint32Array(buf, byteOffset > 0, …)` fed `wgpu_core` the wrong window with no error surfaced — silently incorrect rendering / compute results. The slice is now constructed from `byte_offset()` + `length()`. Adds a regression test (gated on `!isCIWithoutGPU`, like the other webgpu tests) that: - calls `setBindGroup(…, new Uint32Array(4), 0, 1_000_000)` against both a `ComputePass` (expects a validation error in the device error scope) and a `RenderBundleEncoder` (expects a thrown JS error), and - confirms a well-formed call still works. `deno fmt --check` and `deno lint` are clean on the changed files; the Rust changes will get their fmt/clippy from CI. Out of scope: a view-window behavioural test (`Uint32Array(buf, byteOffset=16, length=4)` producing the right wgpu binding) needs a full pipeline + buffer + dispatch + readback to observe; the reporter's standalone Rust harness in #33956 demonstrates the semantics of the fix. Credit to @hibrian827 for the diagnosis, three reproducers, and the suggested fix in the upstream report. Fixes #33956. @bartlomieju --- 🤖 Filed by `agor` (worker `work2-2`, bot `lunadogbot`). Tracking claim: bartlomieju/orchid-inbox#24. (Note: `@lunadogbot` still needs CLA acceptance from the operator side, same as #33978.) --------- Signed-off-by: lunadogbot <lunadogbot@users.noreply.github.com> Co-authored-by: lunadogbot <lunadogbot@users.noreply.github.com> Co-authored-by: Bartek Iwańczuk <biwanczuk@gmail.com> Co-authored-by: Leo Kettmeir <crowlkats@toaxl.com>
1 parent 5fe5b77 commit 35d3602

4 files changed

Lines changed: 144 additions & 16 deletions

File tree

ext/webgpu/compute_pass.rs

Lines changed: 30 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ use deno_core::webidl::WebIdlConverter;
1414
use deno_core::webidl::WebIdlError;
1515

1616
use crate::Instance;
17+
use crate::error::GPUError;
1718
use crate::error::GPUGenericError;
1819

1920
pub struct GPUComputePassEncoder {
@@ -186,15 +187,39 @@ impl GPUComputePassEncoder {
186187
},
187188
)? as usize;
188189

190+
let view_byte_offset = uint_32.byte_offset();
191+
let view_len = uint_32.length();
192+
193+
// Validate `start..start+len` against the **view** length, not the
194+
// backing buffer's length. Without this check, `&data[start..end]`
195+
// below would panic on out-of-range input; the panic crosses the
196+
// op's `extern "C"` boundary and aborts the process. See #33956.
197+
let Some(end) = start.checked_add(len).filter(|end| *end <= view_len)
198+
else {
199+
self.error_handler.push_error(Some(GPUError::Validation(format!(
200+
"{PREFIX}: dynamicOffsetsDataStart + dynamicOffsetsDataLength ({start} + {len}) is outside the bounds of dynamicOffsetsData (length {view_len})",
201+
))));
202+
return Ok(());
203+
};
204+
189205
let ab = uint_32.buffer(scope).unwrap();
190206
let ptr = ab.data().unwrap();
191-
let ab_len = ab.byte_length() / 4;
192207

193-
// SAFETY: compute_pass_set_bind_group internally calls extend_from_slice with this slice
194-
let data =
195-
unsafe { std::slice::from_raw_parts(ptr.as_ptr() as _, ab_len) };
208+
// SAFETY: `ptr` is the start of the backing ArrayBuffer's data; the
209+
// Uint32Array constructor guarantees `byte_offset + view_len * 4`
210+
// fits within `byte_length`, so the resulting slice covers exactly
211+
// the view's window. `data` is dropped at the end of this call;
212+
// `ab` keeps the backing buffer alive for that duration.
213+
// compute_pass_set_bind_group internally calls extend_from_slice
214+
// with this slice.
215+
let data = unsafe {
216+
std::slice::from_raw_parts(
217+
(ptr.as_ptr() as *const u8).add(view_byte_offset) as *const u32,
218+
view_len,
219+
)
220+
};
196221

197-
let offsets = &data[start..(start + len)];
222+
let offsets = &data[start..end];
198223

199224
self
200225
.instance

ext/webgpu/render_bundle.rs

Lines changed: 27 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -185,15 +185,36 @@ impl GPURenderBundleEncoder {
185185
},
186186
)? as usize;
187187

188+
let view_byte_offset = uint_32.byte_offset();
189+
let view_len = uint_32.length();
190+
191+
// Validate `start..start+len` against the **view** length, not the
192+
// backing buffer's length. Without this check, `&data[start..end]`
193+
// below would panic on out-of-range input; the panic crosses the
194+
// op's `extern "C"` boundary and aborts the process. See #33956.
195+
let Some(end) = start.checked_add(len).filter(|end| *end <= view_len)
196+
else {
197+
return Err(JsErrorBox::generic(format!(
198+
"{PREFIX}: dynamicOffsetsDataStart + dynamicOffsetsDataLength ({start} + {len}) is outside the bounds of dynamicOffsetsData (length {view_len})",
199+
)).into());
200+
};
201+
188202
let ab = uint_32.buffer(scope).unwrap();
189203
let ptr = ab.data().unwrap();
190-
let ab_len = ab.byte_length() / 4;
191-
192-
// SAFETY: created from an array buffer, slice is dropped at end of function call
193-
let data =
194-
unsafe { std::slice::from_raw_parts(ptr.as_ptr() as _, ab_len) };
195204

196-
let offsets = &data[start..(start + len)];
205+
// SAFETY: `ptr` is the start of the backing ArrayBuffer's data; the
206+
// Uint32Array constructor guarantees `byte_offset + view_len * 4`
207+
// fits within `byte_length`, so the resulting slice covers exactly
208+
// the view's window. `data` is dropped at the end of this call;
209+
// `ab` keeps the backing buffer alive for that duration.
210+
let data = unsafe {
211+
std::slice::from_raw_parts(
212+
(ptr.as_ptr() as *const u8).add(view_byte_offset) as *const u32,
213+
view_len,
214+
)
215+
};
216+
217+
let offsets = &data[start..end];
197218

198219
// SAFETY: wgpu FFI call
199220
unsafe {

ext/webgpu/render_pass.rs

Lines changed: 28 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ use deno_core::webidl::WebIdlError;
1919

2020
use crate::Instance;
2121
use crate::buffer::GPUBuffer;
22+
use crate::error::GPUError;
2223
use crate::error::GPUGenericError;
2324
use crate::render_bundle::GPURenderBundle;
2425
use crate::texture::GPUTexture;
@@ -261,15 +262,37 @@ impl GPURenderPassEncoder {
261262
},
262263
)? as usize;
263264

265+
let view_byte_offset = uint_32.byte_offset();
266+
let view_len = uint_32.length();
267+
268+
// Validate `start..start+len` against the **view** length, not the
269+
// backing buffer's length. Without this check, `&data[start..end]`
270+
// below would panic on out-of-range input; the panic crosses the
271+
// op's `extern "C"` boundary and aborts the process. See #33956.
272+
let Some(end) = start.checked_add(len).filter(|end| *end <= view_len)
273+
else {
274+
self.error_handler.push_error(Some(GPUError::Validation(format!(
275+
"{PREFIX}: dynamicOffsetsDataStart + dynamicOffsetsDataLength ({start} + {len}) is outside the bounds of dynamicOffsetsData (length {view_len})",
276+
))));
277+
return Ok(());
278+
};
279+
264280
let ab = uint_32.buffer(scope).unwrap();
265281
let ptr = ab.data().unwrap();
266-
let ab_len = ab.byte_length() / 4;
267282

268-
// SAFETY: created from an array buffer, slice is dropped at end of function call
269-
let data =
270-
unsafe { std::slice::from_raw_parts(ptr.as_ptr() as _, ab_len) };
283+
// SAFETY: `ptr` is the start of the backing ArrayBuffer's data; the
284+
// Uint32Array constructor guarantees `byte_offset + view_len * 4`
285+
// fits within `byte_length`, so the resulting slice covers exactly
286+
// the view's window. `data` is dropped at the end of this call;
287+
// `ab` keeps the backing buffer alive for that duration.
288+
let data = unsafe {
289+
std::slice::from_raw_parts(
290+
(ptr.as_ptr() as *const u8).add(view_byte_offset) as *const u32,
291+
view_len,
292+
)
293+
};
271294

272-
let offsets = &data[start..(start + len)];
295+
let offsets = &data[start..end];
273296

274297
self
275298
.instance

tests/unit/webgpu_test.ts

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -810,6 +810,65 @@ Deno.test({
810810
device.destroy();
811811
});
812812

813+
// Regression test for https://github.com/denoland/deno/issues/33956.
814+
// Before the fix, the Uint32Array fast path of setBindGroup sliced the
815+
// backing ArrayBuffer with an unchecked `&data[start..start+len]`. An
816+
// out-of-range length panicked inside the op's `extern "C"` callback,
817+
// which crosses the C ABI as a process abort. After the fix, the
818+
// bounds-check surfaces a GPUValidationError instead.
819+
Deno.test({
820+
permissions: { read: true, env: true },
821+
ignore: isWsl || isCIWithoutGPU,
822+
}, async function webgpuSetBindGroupBoundsCheck() {
823+
const adapter = await navigator.gpu.requestAdapter();
824+
assert(adapter);
825+
const device = await adapter.requestDevice();
826+
827+
const layout = device.createBindGroupLayout({ entries: [] });
828+
const bg = device.createBindGroup({ layout, entries: [] });
829+
830+
// ComputePass.setBindGroup: out-of-range len pushes a validation
831+
// error instead of aborting the process.
832+
{
833+
const encoder = device.createCommandEncoder();
834+
const pass = encoder.beginComputePass();
835+
device.pushErrorScope("validation");
836+
pass.setBindGroup(0, bg, new Uint32Array(4), 0, 1_000_000);
837+
pass.end();
838+
const err = await device.popErrorScope();
839+
assert(err, "expected GPUValidationError on out-of-range setBindGroup");
840+
}
841+
842+
// RenderBundleEncoder.setBindGroup: same input shape, different code
843+
// path (returns the validation as a thrown JS error, since there is
844+
// no error_handler.push_error at this site).
845+
{
846+
const bundleEncoder = device.createRenderBundleEncoder({
847+
colorFormats: ["rgba8unorm"],
848+
});
849+
let threw = false;
850+
try {
851+
bundleEncoder.setBindGroup(0, bg, new Uint32Array(4), 0, 1_000_000);
852+
} catch (_) {
853+
threw = true;
854+
}
855+
assert(
856+
threw,
857+
"expected setBindGroup on a RenderBundleEncoder to throw on out-of-range args",
858+
);
859+
}
860+
861+
// Sanity: valid args still work (no regression on the happy path).
862+
{
863+
const encoder = device.createCommandEncoder();
864+
const pass = encoder.beginComputePass();
865+
pass.setBindGroup(0, bg, new Uint32Array(4), 0, 0);
866+
pass.end();
867+
}
868+
869+
device.destroy();
870+
});
871+
813872
async function checkIsWsl() {
814873
return Deno.build.os === "linux" && await hasMicrosoftProcVersion();
815874

0 commit comments

Comments
 (0)