Handle non-finite ROI coordinates in CropAndResize

[titaiwangms]

Description

The CropAndResize CPU contrib op (com.microsoft::CropAndResize) computes bilinear/nearest interpolation indices from the ROI box coordinates. The bounds guards were written as if (in_y < 0 || in_y > height - 1) / if (in_x < 0 || in_x > width - 1). Because every comparison involving a NaN is false, a non-finite (NaN or ±inf) ROI coordinate slipped past both comparisons, skipped the extrapolation continue, and reached the integer index computation ((int)floorf(NaN)) with an invalid value.

This is a robustness/correctness gap: ROI coordinates are a runtime input, and non-finite values are not handled gracefully.

Fix

• NaN-safe bounds guards. Rewrite both guards into negated-conjunction form: if (!(in_y >= 0 && in_y <= height - 1)) / if (!(in_x >= 0 && in_x <= width - 1)). This is logically identical for all finite coordinates, but is true for NaN/±inf, so non-finite coordinates now take the extrapolation branch and are filled with extrapolation_value (matching the documented behavior for out-of-range coordinates).
• crop_size validation. Add a Status-based ORT_RETURN_IF_NOT(crop_height > 0 && crop_width > 0, ...) check in Compute so non-positive crop sizes are rejected with a clear error rather than producing degenerate work.
• Index arithmetic hardening. Use SafeInt<int64_t> for the interpolation index computation, which allows the now-unnecessary MSVC 26451 (arithmetic-overflow) suppression pragma to be removed.

No behavior change for valid finite ROIs. CPU-only — there is no CUDA CropAndResize kernel.

Tests

Added to onnxruntime/test/contrib_ops/crop_and_resize_op_test.cc:

• NaN ROI coordinate → extrapolation (bilinear height, bilinear width, and nearest).
• ±inf ROI coordinate → extrapolation.
• Finite boundary values (exact [0, 1] identity crop and just-outside 1.0001) — no regression.
• Non-positive crop_size ({0, 2} and {-1, 2}) rejected.
• Out-of-range batch index rejected.

Run with:

onnxruntime_provider_test --gtest_filter='CropAndResizeTest.*'

All 10 CropAndResize tests pass (5 new + 5 pre-existing, no regression). An AddressSanitizer build can additionally corroborate the index handling in CI.

Motivation and Context

Handles non-finite ROI coordinates gracefully and makes the CropAndResize index arithmetic robust, consistent with how out-of-range coordinates are already treated (extrapolation).

[Copilot]

Pull request overview

This PR hardens the com.microsoft::CropAndResize CPU contrib op against non-finite ROI coordinates (NaN/±inf) by ensuring they take the extrapolation path instead of reaching integer index computations, and adds input validation for invalid crop sizes. It improves robustness for runtime-provided ROI inputs and reduces the risk of invalid indexing/overflow in the inner interpolation math.

Changes:

• Make ROI bounds checks NaN/inf-safe by using a negated-conjunction guard so non-finite coordinates always extrapolate.
• Reject non-positive crop_size values early with a clear Status error message.
• Harden interpolation index arithmetic with SafeInt<int64_t> and remove the MSVC overflow-warning suppression.

Reviewed changes

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

File	Description
onnxruntime/contrib_ops/cpu/crop_and_resize.cc	Adds NaN/inf-safe bounds guards, validates crop_size positivity, and uses SafeInt for index/offset arithmetic.
onnxruntime/test/contrib_ops/crop_and_resize_op_test.cc	Adds coverage for NaN/±inf ROI extrapolation, finite boundary non-regression, invalid crop_size, and out-of-range batch indices.

[titaiwangms]

Review — multi-agent pass (readability, correctness, adversarial, spec/deep, integration)

Verdict: LGTM. No Critical or Major in-scope blockers. The fix is correct, spec-faithful, and well-tested. Two reviewers independently confirmed the negated-conjunction guard is numerically identical to the old < / > form for all finite inputs (the int64_t bound was already converted to float in the old comparison, so static_cast<T>(height-1) reproduces it exactly — verified at height = 2^24+3). Routing out-of-range coords to extrapolation_value matches the TF CropAndResize reference this kernel is ported from; the NaN/inf hole exists in TF upstream too, so this is more defensive than the reference. Confirmed CPU is the only com.microsoft::CropAndResize implementation, so no sibling kernel needs the same change. New tests are wired in via the contrib_ops/*.cc glob and the crop_size>0 check enforces the schema's documented "both crop_height and crop_width need to be positive" contract that shape inference cannot.

Non-blocking follow-ups (out of scope for this PR)

• Latent OOB for image dimensions > 2^24. For T=float, static_cast<float>(height-1) can round up to height when height exceeds 2^24 (~16.7M px), so a boundary coordinate could still pass the guard and index one row/col past the image. This is pre-existing (not a regression) — the old guard had the same float conversion — so it doesn't block. A follow-up could keep the coordinate/bounds math in double before narrowing to the integer index. (via critical-reviewer)
• Sibling RoiAlign has the same NaN hole. core/providers/cpu/object_detection/roialign.cc:111 (and the CUDA roialign_impl.cu) use the same if (y < -1 || y > height ...) pattern that NaN slips through. Worth the same NaN-safe rewrite in a follow-up. (via integration-reviewer)

Minor / nits (optional)

• Consider erroring on non-finite ROI coords instead of silently extrapolating, to match this PR's own crop_size/batch_index validation philosophy — or add a one-line schema note that extrapolation_value also covers non-finite boxes. Neither the schema nor TF specifies NaN behavior, so the graceful-fill choice is defensible; just confirm it's intended. (via deep-reviewer)
• Readability: extract the repeated static_cast<int64_t>((SafeInt<int64_t>(roi_batch_ind) * channels + c) * height * width) into a named batch_channel_offset local; the nested SafeInt + outer static_cast is hard to parse. (via readability-reviewer)
• The CropAndResize_finite_boundary_no_regression comment says "only the out-of-range row/column" but 3 of 4 pixels extrapolate; tighten the comment. (via readability-reviewer)
• CropAndResize_rejects_out_of_range_batch_index exercises pre-existing CheckROIAlignValidInput bounds checking rather than this PR's new code — fine as a regression test, worth a comment noting that. (via code-reviewer)

Reviewed by a 5-agent team (Claude / GPT / Gemini families). Findings deduplicated and prioritized by the lead; the float-precision and RoiAlign items were confirmed pre-existing and thus filed as follow-ups rather than blockers.

[apsonawane]

Can we create a follow-up PR for RoIAlign as well as mentioned in the above comment

[titaiwangms]

Can we create a follow-up PR for RoIAlign as well as mentioned in the above comment

Will do