WebGPU Pad kernel int64 / int32 truncation can read to oob read

[guschmue]

WebGPU Pad kernel int64 / int32 truncation can read to oob read

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

[hariharans29]

Review — PR #28721 (WebGPU Pad: int64→int32 truncation OOB + wrap negative-mod)

Verdict: approve. Two real bugs fixed cleanly in one small PR, with tests for both.

What it fixes

1. Silent int64→int32 truncation on lower_pad. Pad's pads input is int64 per ONNX spec, but the WebGPU shader path stores it as i32 (lower_pads[i] = static_cast<int32_t>(lower_pad)). Out-of-range values silently wrapped, then input_index = output_index - lower_pads[i] aliased into a wrong (often OOB) input element. The new ORT_RETURN_IF_NOT rejects with a clear message before dispatch.
2. Negative in_coord under WRAP mode. WGSL's % follows C semantics — (-1) % 3 == -1, not 2. The previous shader did in_coord = output_index - lower_pads; followed by an unguarded in_coord % data_shape, which read OOB whenever output_index < lower_pads (i.e., any output position inside the lower-pad region) or whenever lower_pad > data_shape. The new line in_coord = ((in_coord % data_shape) + data_shape) % data_shape; is the standard idiom for true positive mod.

What's right

• The ORT_RETURN_IF_NOT check is at the correct layer (host code, before launch) and includes the offending axis and value in the error message — great for debuggability.
• SafeInt<int64_t>(lower_pad) + upper_pad correctly guards the output_dims[i] += accumulation against int64 overflow. Cheap and right.
• The #include <limits> is added explicitly, not relied on transitively.
• Tests cover both code paths:
  • Pad_Wrap_WebGpu_LowerPadExceedsInt32Fails uses 2147483648LL (INT32_MAX + 1) — exactly the right boundary.
  • Pad_Wrap_WebGpu_PadGreaterThanInputDimension uses pads {5, 0} on a 3-element input — exercises the negative-mod path with lower_pad > data_shape, which would have read OOB before the shader fix.
• The fix is properly gated #if pad_mode == PAD_MODE_WRAP in the WGSL template, so non-wrap modes pay zero cost.

Minor comments

1. The bounds check covers lower_pad but not upper_pad. That's correct as written — upper_pad is only used to compute output_dims[i] and is never cast to i32 or sent to the shader. Worth a one-line comment next to the ORT_RETURN_IF_NOT saying "only lower_pad is consumed by the shader as i32; upper_pad is i64 throughout host code", so the next reader doesn't add a redundant check or wonder why upper is unguarded.

2. Negative lower_pad boundary not exercised. The failure test only covers the upper boundary (INT32_MAX + 1). A symmetric test for INT32_MIN - 1 would lock in the lower_pad >= std::numeric_limits<int32_t>::min() half of the check. Trivial to add — duplicate the test with pads {-2147483649LL, 0} and the same expected-failure message. Not blocking.

3. The wrap fix ((x % n) + n) % n assumes n > 0. data_shape dimensions are always positive at runtime (zero-dim tensors would have been short-circuited earlier), so this is fine. Worth a one-line comment in the WGSL template noting the invariant: // data_shape > 0 by tensor-shape invariant; two-step mod handles negative in_coord under WRAP.

4. Test naming nit. Pad_Wrap_WebGpu_LowerPadExceedsInt32Fails is descriptive; Pad_Wrap_WebGpu_PadGreaterThanInputDimension reads more like a behavior than a bug-regression name. Consider Pad_Wrap_WebGpu_LowerPadExceedsDataShape_WrapsCorrectly so the regression intent is clear. Bikeshed.

5. The two new tests share substantial setup boilerplate (DefaultWebGpuExecutionProvider, eps.push_back, etc.). Not worth a fixture for two tests; mention only if you grow it further.

Things to verify before merge

• Confirm data_shape cannot be zero on entry to the shader (#if pad_mode == PAD_MODE_WRAP block divides by it). The CPU Pad op has a zero-dim rejection upstream; verify the WebGPU EP inherits that path. If not, an ORT_RETURN_IF on zero dims in Pad::ComputeInternal before shader dispatch would close the gap.
• The Copilot reviewer first errored, then ran without new comments — that's clean, but it means no automated coverage of the shader template change. The two new gtests are the real signal here.

Bottom line

Two narrow, real bugs; correct fixes; tested. Approve. 86/86 CI green. The minor comments above are documentation polish and a symmetric negative-bound test — nice to have, not blockers.