Skip to content

Address resize shortcomings - #28779

Merged
yuslepukhin merged 6 commits into
mainfrom
yuslepukhin/address_resize_shortcomings
Jun 5, 2026
Merged

Address resize shortcomings#28779
yuslepukhin merged 6 commits into
mainfrom
yuslepukhin/address_resize_shortcomings

Conversation

@yuslepukhin

@yuslepukhin yuslepukhin commented Jun 3, 2026

Copy link
Copy Markdown
Contributor

This pull request strengthens input validation and error handling for the ONNX Resize and Upsample operators, particularly around the axes, scales, and roi attributes. It ensures compliance with the ONNX specification and prevents invalid or ambiguous input from causing incorrect behavior or crashes. The changes also introduce comprehensive unit tests to verify these new validation paths.

Key improvements include:

Validation and Error Handling

  • Axes Attribute Validation:
    The axes attribute is validated after negative-axis normalization once the input rank is known — at construction when the rank is statically available, otherwise on the first Compute(). This rejects duplicates that collide only after canonicalization (e.g. {-1, rank-1}) as well as out-of-range entries, with clear error messages. The normalized axes are cached to keep the inference hot path allocation-free.
  • Scales Validation:
    All scale values are checked for finiteness (no NaN or Inf allowed), and for non-Resize modes, scale values must be ≥ 1. This prevents invalid scaling operations.

ROI and Sizes Validation

  • ROI Length Check:
    When axes are provided, the roi input length must be either 2 * len(axes) (per-axis ROI) or 2 * rank (default ROI). Anything else is rejected. The default-ROI path no longer scatters spurious zeros into the canonical [0..0, 1..1] buffer when axes is partial.
  • Sizes/Axes Count Consistency:
    If axes are given, the number of elements in the sizes input must match the number of axes, ensuring correct output shape computation.

Error Propagation and Refactoring

  • Status Return for ROI Calculation:
    The ComputeROIWithAxes function now returns a Status object, and all its callers (CPU, CUDA, WebGPU EPs) are updated to propagate errors instead of assuming success.

Testing

  • Comprehensive Unit Tests:
    New tests cover negative and out-of-range axes, post-normalization duplicate axes (including {-1, rank-1}), mismatched sizes/axes counts, non-finite scale values, and invalid ROI length. These tests ensure that invalid input is consistently rejected across all relevant code paths and work correctly in builds without exceptions.

Miscellaneous

  • Test File Improvements:
    The test file now includes <limits> to support testing of NaN and Inf values in scales.

These changes collectively make the Resize and Upsample operators more robust and standards-compliant, and help prevent subtle bugs due to invalid input.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR tightens ONNX Resize/Upsample validation and error propagation around opset-18 axes/scales/roi, and adds regression tests to ensure invalid inputs are rejected consistently across providers.

Changes:

  • Add validation for duplicate axes, non-finite scales, sizes length vs axes, and roi length vs axes (opset 18+ paths).
  • Refactor ComputeROIWithAxes to return Status and propagate errors in CPU/CUDA/WebGPU call sites.
  • Add unit tests covering new validation failures (NaN/Inf scales, negative/out-of-range axes, sizes/axes mismatch, ROI too short, duplicate axes).

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
onnxruntime/test/providers/cpu/tensor/resize_op_test.cc Adds new negative tests (and one positive test) for opset-18 Resize validation around axes/scales/roi/sizes.
onnxruntime/core/providers/cpu/tensor/upsamplebase.h Implements new validation checks and changes ComputeROIWithAxes to return Status.
onnxruntime/core/providers/cpu/tensor/upsample.cc Propagates ComputeROIWithAxes failures via ORT_RETURN_IF_ERROR.
onnxruntime/core/providers/cuda/tensor/upsample.cc Propagates ComputeROIWithAxes failures via ORT_RETURN_IF_ERROR.
onnxruntime/core/providers/webgpu/tensor/upsample.cc Propagates ComputeROIWithAxes failures via ORT_RETURN_IF_ERROR.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread onnxruntime/core/providers/cpu/tensor/upsamplebase.h Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

@tianleiwu tianleiwu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

Solid input-validation hardening for Resize/Upsample. The changes are well-scoped, propagate errors via Status instead of throwing/UB, and are backed by comprehensive tests covering negative/out-of-range axes, post-normalization duplicate axes, sizes/axes count mismatch, non-finite scales, and short ROI. I verified the new ROI-length check does not regress valid models: on the default-ROI path roi_array is sized 2 * rank (>= 2 * len(axes)), and on the cached/tf_crop_and_resize path the user ROI is 2 * len(axes), so both satisfy the new guard.

Verdict: COMMENT — no blocking issues. A few minor, non-blocking notes below.

What's well done

  • ComputeROIWithAxes now returns Status and all three EP callers (CPU/CUDA/WebGPU) propagate it with ORT_RETURN_IF_ERROR — no silent OOB on malformed axes/roi.
  • ValidateAndNormalizeAxes correctly enforces uniqueness after negative-axis normalization, so {-1, rank-1} collisions are caught; the error message matches the existing HandleNegativeAxis wording for consistency.
  • The std::isfinite check in ScalesValidation closes a real gap: +Inf previously passed the scale > 0 check and would have flowed into output-dim computation.

Minor suggestions (non-blocking)

  1. Redundant per-inference validation. ValidateAndNormalizeAxes allocates a seen bitmap plus a normalized_axes vector and runs on every Compute() from both ComputeROIWithAxes and ParseScalesData/ParseSizesData. Since axes_ is fixed at construction, the range/uniqueness portion only depends on rank, which is usually known. Consider validating once (e.g., at construction when input rank is available, or memoizing the normalized axes) to keep the inference hot path allocation-free. Functionally correct as-is; this is purely an efficiency/cleanliness note.
  2. Pre-existing, out of scope: on the default-ROI path with a partial axes set, ComputeROIWithAxes reads the per-axis end value from roi_array[axes_.size() + i], which indexes into the start half of the default [0..0, 1..1] buffer. This is benign today because ROI only affects coordinate computation under tf_crop_and_resize (where a real ROI is supplied), so output is unaffected — just flagging for awareness since this PR touches the function.
  3. The PR description says axes uniqueness is enforced "during operator construction," but it is actually enforced lazily at Compute() time (it needs the input rank). The behavior is correct since ComputeROIWithAxes always runs, but the description wording could be aligned to avoid confusion.

xadupre
xadupre previously approved these changes Jun 4, 2026

@tianleiwu tianleiwu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary (re-review of follow-up commit)

Thanks for the follow-up. The three minor notes from the previous review round are all addressed, and the validation hardening for Resize/Upsample is correct, well-scoped, and well-tested. Verdict: APPROVE — no remaining findings.

Prior notes — resolved

  1. Per-inference validation cost → resolved. The constructor now pre-populates normalized_axes_ / normalized_axes_rank_ when the input rank is statically known, and ValidateAndNormalizeAxes short-circuits to a copy on cache hit, keeping the inference hot path allocation-free for the common case.
  2. Default-ROI partial-axes read → resolved. ComputeROIWithAxes now distinguishes per-axis ROI (size == 2*len(axes)) from the default full-rank ROI (size == 2*rank) and only scatters in the per-axis case, so the default [0..0, 1..1] buffer is no longer indexed as per-axis start/end pairs.
  3. Description wording → resolved. Axes are now genuinely validated at construction when the rank is statically available (otherwise lazily on first Compute()), matching the description.

Correctness checks on the current head

  • Caching is sound and thread-safe. normalized_axes_ / normalized_axes_rank_ are written once at construction; the const hot path only reads them and writes to a caller-local TensorShapeVector. The cache key is rank, and since axes_ is immutable the normalized form is fully determined by rank; a runtime rank that differs from the static rank simply misses the cache and recomputes — safe.
  • ROI guard does not regress valid models. The default-ROI path yields exactly 2*rank and the tf_crop_and_resize/cached path supplies 2*len(axes), both of which satisfy per_axis_roi || size == 2*rank. A full-rank ROI with partial axes is accepted and applied directly (no scatter), which is the correct full-rank semantics.
  • OOB read fixed. The previous per-axis path read roi_array[axes_.size()+i]; with a short ROI that was an out-of-bounds read, now rejected by the length guard (Roi_TooShortForAxes_18).
  • std::isfinite gap closed. +Inf/NaN previously passed scale > 0 and flowed into output-dim computation; now rejected up front.
  • All three EP callers (CPU/CUDA/WebGPU) propagate the new Status via ORT_RETURN_IF_ERROR; no other callers exist.

Nice, thorough test coverage including the no-exception build path.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants