fix bound check on webgpu/slice

[guschmue]

Fix missing axis bounds check leads to heap OOB write

[hariharans29]

Review — PR #28704 fix bound check on webgpu/slice

Verdict: good fix, accept with one small addition. Tight, targeted, has tests. The OOB-write classification is correct — without the check, axes[i] flows into output_dims[axes[i]] = ... a few lines later, which is a heap write at an attacker-controlled index.

What's good

• Bound check happens before static_cast<uint32_t>(val), so a negative-and-still-negative-after-+= input_rank value can't wrap to a huge positive index. Correct ordering.
• Duplicate check via InlinedHashSet<uint32_t> is the right container choice per project conventions.
• The axes_raw.size() == starts_raw.size() check fixes a second latent OOB (the main loop iterates starts_raw.size() and indexes axes[i], which previously could be short).
• Tests are placed in the shared slice_op.test.cc so they run against every EP, locking in consistent rejection behavior rather than only validating WebGPU.

One thing missing

The main loop reads ends_raw[i] and steps_raw[i] for i < starts_raw.size(). The PR adds:

ORT_RETURN_IF_NOT(axes_raw.size() == starts_raw.size(), "axes and starts must have the same size");
ORT_RETURN_IF_NOT(steps_raw.size() == starts_raw.size(), "steps and starts must have the same size");

…but no equivalent check for ends_raw. A model with len(ends) < len(starts) still OOB-reads ends_raw[i]. Please add:

ORT_RETURN_IF_NOT(ends_raw.size() == starts_raw.size(), "ends and starts must have the same size");

This is the same class of bug as the one you're fixing; worth closing in the same PR.

Nits

• The Slice ONNX spec says "axes and steps are optional with default 0..r-1 and 1s." When defaults are injected (axes_default, steps_default) the sizes are constructed from starts_raw.size(), so the new equality checks are trivially true for those branches and only meaningful when the user supplies the input. Fine, but a one-line comment noting that would help future readers.
• axes_fixed and axes end up holding the same values now. Consider just keeping one (looks like axes_fixed is only used in the missing-dimension insertion loop, where axes would do equally well). Minor cleanup, not blocking.
• Layering nit, not for this PR: the WebGPU slice has its own bounds/clamp logic separate from SliceBase::PrepareForCompute. The reason this bug existed only in WebGPU is the duplicated path. Worth a follow-up to consolidate, but out of scope here.
• The new tests use OpTester("Slice", 10) only. Consider adding one at opset 13 too (where axes/steps are inputs rather than attributes) so the input-tensor code path is also exercised — though the existing test already exercises the input-tensor path since it uses AddInput<int64_t>("axes", ...). Fine as-is.
• Test name InvalidAxesDuplicates — error message check "'axes' has duplicates" is exact-match against the new string. If anyone re-words the message, the test silently moves to substring-match behavior. Not a real issue, just be aware.

Bottom line

Solid security fix. Add the ends_raw size check and this is ready to merge.

[hariharans29]

Re-review: Approve

Both follow-up commits land my prior feedback. Specifically:

Commit d1ff228 — "review feedback"

The missing ends_raw size check is now in. The PR now has the full triplet:

ORT_RETURN_IF_NOT(axes_raw.size() == starts_raw.size(),  "axes and starts must have the same size");
ORT_RETURN_IF_NOT(steps_raw.size() == starts_raw.size(), "steps and starts must have the same size");
ORT_RETURN_IF_NOT(ends_raw.size() == starts_raw.size(),  "ends and starts must have the same size");

That closes the second latent OOB read I flagged — the main loop reads all three (starts_raw[i], ends_raw[i], steps_raw[i]) for i < starts_raw.size(), so all three sizes need to match. The order of the three checks doesn't matter semantically; ORT_RETURN_IF_NOT returns on the first failure, so each error message is reachable. ✓

Commit 996bce1 — "test not supported by dml"

testv10.Run(OpTester::ExpectResult::kExpectFailure,
            "axis outside of the tensor dimension count",
            {kTensorrtExecutionProvider, kDmlExecutionProvider});

Excluding DML for the negative tests is the right call — the DML EP routes Slice through its own kernel that emits a different (or no) error string, so a cross-EP expect-failure test would mismatch. Same exclusion applied to both InvalidAxesOutOfBounds and InvalidAxesDuplicates. Consistent.

One observation while you're here: if there's an existing slice EP-exclusion list elsewhere (other negative slice tests), worth checking they all exclude DML consistently. Not blocking — purely a "tests stay consistent" hygiene thing.

Nits from the prior review (unchanged, still non-blocking)

• axes_fixed and axes still hold the same values. Fine as-is; pure cleanup.
• The layering concern (WebGPU slice has its own bounds/clamp logic separate from SliceBase::PrepareForCompute) is the underlying reason this bug existed only here. Still worth a follow-up issue for consolidation, but explicitly out of scope for this PR.

---

Bottom line

The substantive ask was addressed, the test EP-exclusion is correct, and auto-merge is reasonable. Approve as-is.