Skip to content

Commit bcc9d30

Browse files
titaiwangmsCopilot
andauthored
Handle non-finite ROI coordinates in CropAndResize (#29605)
### 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). Signed-off-by: Tita Wang <titaiwang@microsoft.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent 682936c commit bcc9d30

2 files changed

Lines changed: 139 additions & 14 deletions

File tree

onnxruntime/contrib_ops/cpu/crop_and_resize.cc

Lines changed: 20 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -17,16 +17,12 @@ limitations under the License.
1717
#include "contrib_ops/cpu/crop_and_resize.h"
1818

1919
#include <cmath>
20+
#include "core/common/safeint.h"
2021
#include "core/util/math_cpuonly.h"
2122
#include "core/common/common.h"
2223
#include "core/framework/tensor.h"
2324
#include "core/platform/threadpool.h"
2425
#include "core/providers/cpu/object_detection/roialign.h"
25-
// TODO: fix the warnings
26-
#if defined(_MSC_VER) && !defined(__clang__)
27-
// Chance of arithmetic overflow could be reduced
28-
#pragma warning(disable : 26451)
29-
#endif
3026
using namespace onnxruntime::concurrency;
3127

3228
namespace onnxruntime {
@@ -97,7 +93,11 @@ void CropAndResizeForward(const TensorShape& output_shape,
9793
? roi_start_h * (height - 1)
9894
: 0.5 * (roi_start_h + roi_end_h) * (height - 1));
9995
}
100-
if (in_y < 0 || in_y > height - 1) {
96+
// Route non-finite (NaN/inf) or out-of-image coordinates to the extrapolation
97+
// branch. The negated-conjunction form is NaN-safe: every comparison with NaN is
98+
// false, so !(in_y >= 0 && in_y <= height - 1) is true and NaN cannot reach the
99+
// integer index math below. For finite values this is identical to the < / > form.
100+
if (!(in_y >= 0 && in_y <= static_cast<T>(height - 1))) {
101101
for (int64_t pw = 0; pw < pooled_width; pw++) {
102102
for (int64_t c = 0; c < channels; c++) {
103103
int64_t index_n_c = index_n + c * pooled_width * pooled_height;
@@ -126,7 +126,8 @@ void CropAndResizeForward(const TensorShape& output_shape,
126126
? roi_start_w * (width - 1)
127127
: 0.5 * (roi_start_w + roi_end_w) * (width - 1));
128128
}
129-
if (in_x < 0 || in_x > width - 1) {
129+
// NaN-safe bounds guard (see the in_y guard above for rationale).
130+
if (!(in_x >= 0 && in_x <= static_cast<T>(width - 1))) {
130131
for (int64_t c = 0; c < channels; c++) {
131132
int64_t index_n_c = index_n + c * pooled_width * pooled_height;
132133
int64_t index = index_n_c + ph * pooled_width + pw;
@@ -140,16 +141,17 @@ void CropAndResizeForward(const TensorShape& output_shape,
140141
const int left_x_index = static_cast<int>(floorf(static_cast<float>(in_x)));
141142
const int right_x_index = static_cast<int>(ceilf(static_cast<float>(in_x)));
142143
const float x_lerp = static_cast<float>(in_x - left_x_index);
143-
auto top_left_index = top_y_index * width + left_x_index;
144-
auto top_right_index = top_y_index * width + right_x_index;
145-
auto bottom_left_index = bottom_y_index * width + left_x_index;
146-
auto bottom_right_index = bottom_y_index * width + right_x_index;
144+
auto top_left_index = SafeInt<int64_t>(top_y_index) * width + left_x_index;
145+
auto top_right_index = SafeInt<int64_t>(top_y_index) * width + right_x_index;
146+
auto bottom_left_index = SafeInt<int64_t>(bottom_y_index) * width + left_x_index;
147+
auto bottom_right_index = SafeInt<int64_t>(bottom_y_index) * width + right_x_index;
147148

148149
for (auto c = 0; c < channels; c++) {
149150
int64_t index_n_c = index_n + c * pooled_width * pooled_height;
150151
int64_t index = index_n_c + ph * pooled_width + pw;
151152
const T* offset_bottom_data =
152-
bottom_data + static_cast<int64_t>((roi_batch_ind * channels + c) * height * width);
153+
bottom_data +
154+
static_cast<int64_t>((SafeInt<int64_t>(roi_batch_ind) * channels + c) * height * width);
153155
const float top_left(static_cast<float>(offset_bottom_data[top_left_index]));
154156
const float top_right(static_cast<float>(offset_bottom_data[top_right_index]));
155157
const float bottom_left(static_cast<float>(offset_bottom_data[bottom_left_index]));
@@ -162,13 +164,14 @@ void CropAndResizeForward(const TensorShape& output_shape,
162164
} else { // mode == "nearest"
163165
const int closest_x_index = static_cast<int>(roundf(static_cast<float>(in_x)));
164166
const int closest_y_index = static_cast<int>(roundf(static_cast<float>(in_y)));
165-
auto closest_index = closest_y_index * width + closest_x_index;
167+
auto closest_index = SafeInt<int64_t>(closest_y_index) * width + closest_x_index;
166168

167169
for (auto c = 0; c < channels; c++) {
168170
int64_t index_n_c = index_n + c * pooled_width * pooled_height;
169171
int64_t index = index_n_c + ph * pooled_width + pw;
170172
const T* offset_bottom_data =
171-
bottom_data + static_cast<int64_t>((roi_batch_ind * channels + c) * height * width);
173+
bottom_data +
174+
static_cast<int64_t>((SafeInt<int64_t>(roi_batch_ind) * channels + c) * height * width);
172175
top_data[index] = static_cast<float>(offset_bottom_data[closest_index]);
173176
}
174177
}
@@ -217,6 +220,9 @@ Status CropAndResize<T>::Compute(OpKernelContext* context) const {
217220
auto crop_height = crop_size_data[0];
218221
auto crop_width = crop_size_data[1];
219222

223+
ORT_RETURN_IF_NOT(crop_height > 0 && crop_width > 0,
224+
"crop_size values must be positive; got [", crop_height, ", ", crop_width, "]");
225+
220226
auto status = CheckROIAlignValidInput(X_ptr, rois_ptr, batch_indices_ptr);
221227
if (status != Status::OK()) {
222228
return status;

onnxruntime/test/contrib_ops/crop_and_resize_op_test.cc

Lines changed: 119 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,8 @@
44
#include "gtest/gtest.h"
55
#include "test/providers/provider_test_utils.h"
66

7+
#include <limits>
8+
79
namespace onnxruntime {
810
namespace test {
911

@@ -123,5 +125,122 @@ TEST(CropAndResizeTest, CropAndResize_1133) {
123125
test3.Run(OpTester::ExpectResult::kExpectSuccess, "", {kTensorrtExecutionProvider});
124126
}
125127

128+
// Non-finite ROI coordinates (NaN) must route to the extrapolation branch instead of
129+
// reaching the integer index math, which would otherwise compute an invalid image index.
130+
TEST(CropAndResizeTest, CropAndResize_NaN_roi_extrapolates) {
131+
const float nan = std::numeric_limits<float>::quiet_NaN();
132+
133+
// NaN start/end height -> in_y is NaN for every row -> whole output extrapolates.
134+
OpTester test_y("CropAndResize", 1, onnxruntime::kMSDomain);
135+
test_y.AddInput<float>("X", {1, 1, 2, 2}, {1.1f, 2.2f, 3.3f, 4.4f});
136+
test_y.AddInput<float>("rois", {1, 4}, {nan, 0.0f, nan, 1.0f});
137+
test_y.AddInput<int32_t>("batch_indices", {1}, {0});
138+
test_y.AddInput<int32_t>("crop_size", {2}, {2, 2});
139+
test_y.AddAttribute("extrapolation_value", 5.5f);
140+
test_y.AddOutput<float>("output", {1, 1, 2, 2}, {5.5f, 5.5f, 5.5f, 5.5f});
141+
test_y.Run(OpTester::ExpectResult::kExpectSuccess, "", {kTensorrtExecutionProvider});
142+
143+
// Finite height but NaN width -> in_y is in range, in_x is NaN -> every pixel extrapolates.
144+
OpTester test_x("CropAndResize", 1, onnxruntime::kMSDomain);
145+
test_x.AddInput<float>("X", {1, 1, 2, 2}, {1.1f, 2.2f, 3.3f, 4.4f});
146+
test_x.AddInput<float>("rois", {1, 4}, {0.0f, nan, 1.0f, nan});
147+
test_x.AddInput<int32_t>("batch_indices", {1}, {0});
148+
test_x.AddInput<int32_t>("crop_size", {2}, {2, 2});
149+
test_x.AddAttribute("extrapolation_value", 5.5f);
150+
test_x.AddOutput<float>("output", {1, 1, 2, 2}, {5.5f, 5.5f, 5.5f, 5.5f});
151+
test_x.Run(OpTester::ExpectResult::kExpectSuccess, "", {kTensorrtExecutionProvider});
152+
153+
// Same NaN-height case in nearest mode.
154+
OpTester test_nearest("CropAndResize", 1, onnxruntime::kMSDomain);
155+
test_nearest.AddInput<float>("X", {1, 1, 2, 2}, {1.1f, 2.2f, 3.3f, 4.4f});
156+
test_nearest.AddInput<float>("rois", {1, 4}, {nan, 0.0f, nan, 1.0f});
157+
test_nearest.AddInput<int32_t>("batch_indices", {1}, {0});
158+
test_nearest.AddInput<int32_t>("crop_size", {2}, {2, 2});
159+
test_nearest.AddAttribute("mode", "nearest");
160+
test_nearest.AddAttribute("extrapolation_value", 5.5f);
161+
test_nearest.AddOutput<float>("output", {1, 1, 2, 2}, {5.5f, 5.5f, 5.5f, 5.5f});
162+
test_nearest.Run(OpTester::ExpectResult::kExpectSuccess, "", {kTensorrtExecutionProvider});
163+
}
164+
165+
// Infinite ROI coordinates must also land in the extrapolation branch.
166+
TEST(CropAndResizeTest, CropAndResize_Inf_roi_extrapolates) {
167+
const float inf = std::numeric_limits<float>::infinity();
168+
169+
OpTester test_pos("CropAndResize", 1, onnxruntime::kMSDomain);
170+
test_pos.AddInput<float>("X", {1, 1, 2, 2}, {1.1f, 2.2f, 3.3f, 4.4f});
171+
test_pos.AddInput<float>("rois", {1, 4}, {inf, 0.0f, inf, 1.0f});
172+
test_pos.AddInput<int32_t>("batch_indices", {1}, {0});
173+
test_pos.AddInput<int32_t>("crop_size", {2}, {2, 2});
174+
test_pos.AddAttribute("extrapolation_value", 5.5f);
175+
test_pos.AddOutput<float>("output", {1, 1, 2, 2}, {5.5f, 5.5f, 5.5f, 5.5f});
176+
test_pos.Run(OpTester::ExpectResult::kExpectSuccess, "", {kTensorrtExecutionProvider});
177+
178+
OpTester test_neg("CropAndResize", 1, onnxruntime::kMSDomain);
179+
test_neg.AddInput<float>("X", {1, 1, 2, 2}, {1.1f, 2.2f, 3.3f, 4.4f});
180+
test_neg.AddInput<float>("rois", {1, 4}, {-inf, 0.0f, -inf, 1.0f});
181+
test_neg.AddInput<int32_t>("batch_indices", {1}, {0});
182+
test_neg.AddInput<int32_t>("crop_size", {2}, {2, 2});
183+
test_neg.AddAttribute("extrapolation_value", 5.5f);
184+
test_neg.AddOutput<float>("output", {1, 1, 2, 2}, {5.5f, 5.5f, 5.5f, 5.5f});
185+
test_neg.Run(OpTester::ExpectResult::kExpectSuccess, "", {kTensorrtExecutionProvider});
186+
}
187+
188+
// Finite coordinates on and just outside the image boundary must behave exactly as the
189+
// original guard: coordinates in [0, dim-1] interpolate, coordinates just past dim-1 extrapolate.
190+
TEST(CropAndResizeTest, CropAndResize_finite_boundary_no_regression) {
191+
// Exact boundary [0, 1] maps to the identity crop (no extrapolation at 0 or height-1).
192+
OpTester test_exact("CropAndResize", 1, onnxruntime::kMSDomain);
193+
test_exact.AddInput<float>("X", {1, 1, 2, 2}, {1.1f, 2.2f, 3.3f, 4.4f});
194+
test_exact.AddInput<float>("rois", {1, 4}, {0.0f, 0.0f, 1.0f, 1.0f});
195+
test_exact.AddInput<int32_t>("batch_indices", {1}, {0});
196+
test_exact.AddInput<int32_t>("crop_size", {2}, {2, 2});
197+
test_exact.AddAttribute("extrapolation_value", 9.0f);
198+
test_exact.AddOutput<float>("output", {1, 1, 2, 2}, {1.1f, 2.2f, 3.3f, 4.4f});
199+
test_exact.Run(OpTester::ExpectResult::kExpectSuccess, "", {kTensorrtExecutionProvider});
200+
201+
// End just past the boundary (1.0001) extrapolates only the out-of-range row/column.
202+
OpTester test_outside("CropAndResize", 1, onnxruntime::kMSDomain);
203+
test_outside.AddInput<float>("X", {1, 1, 2, 2}, {1.1f, 2.2f, 3.3f, 4.4f});
204+
test_outside.AddInput<float>("rois", {1, 4}, {0.0f, 0.0f, 1.0001f, 1.0001f});
205+
test_outside.AddInput<int32_t>("batch_indices", {1}, {0});
206+
test_outside.AddInput<int32_t>("crop_size", {2}, {2, 2});
207+
test_outside.AddAttribute("extrapolation_value", 9.0f);
208+
test_outside.AddOutput<float>("output", {1, 1, 2, 2}, {1.1f, 9.0f, 9.0f, 9.0f});
209+
test_outside.Run(OpTester::ExpectResult::kExpectSuccess, "", {kTensorrtExecutionProvider});
210+
}
211+
212+
// crop_size values must be positive; non-positive crop dimensions are rejected via Status.
213+
TEST(CropAndResizeTest, CropAndResize_rejects_nonpositive_crop_size) {
214+
OpTester test_zero("CropAndResize", 1, onnxruntime::kMSDomain);
215+
test_zero.AddInput<float>("X", {1, 1, 2, 2}, {1.1f, 2.2f, 3.3f, 4.4f});
216+
test_zero.AddInput<float>("rois", {1, 4}, {0.0f, 0.0f, 1.0f, 1.0f});
217+
test_zero.AddInput<int32_t>("batch_indices", {1}, {0});
218+
test_zero.AddInput<int32_t>("crop_size", {2}, {0, 2});
219+
test_zero.AddOutput<float>("output", {1, 1, 1, 1}, {0.0f});
220+
test_zero.Run(OpTester::ExpectResult::kExpectFailure,
221+
"crop_size values must be positive", {kTensorrtExecutionProvider});
222+
223+
OpTester test_negative("CropAndResize", 1, onnxruntime::kMSDomain);
224+
test_negative.AddInput<float>("X", {1, 1, 2, 2}, {1.1f, 2.2f, 3.3f, 4.4f});
225+
test_negative.AddInput<float>("rois", {1, 4}, {0.0f, 0.0f, 1.0f, 1.0f});
226+
test_negative.AddInput<int32_t>("batch_indices", {1}, {0});
227+
test_negative.AddInput<int32_t>("crop_size", {2}, {-1, 2});
228+
test_negative.AddOutput<float>("output", {1, 1, 1, 1}, {0.0f});
229+
test_negative.Run(OpTester::ExpectResult::kExpectFailure,
230+
"crop_size values must be positive", {kTensorrtExecutionProvider});
231+
}
232+
233+
// A batch index outside [0, batch_size) must be rejected rather than computing an invalid image index.
234+
TEST(CropAndResizeTest, CropAndResize_rejects_out_of_range_batch_index) {
235+
OpTester test("CropAndResize", 1, onnxruntime::kMSDomain);
236+
test.AddInput<float>("X", {1, 1, 2, 2}, {1.1f, 2.2f, 3.3f, 4.4f});
237+
test.AddInput<float>("rois", {1, 4}, {0.0f, 0.0f, 1.0f, 1.0f});
238+
test.AddInput<int32_t>("batch_indices", {1}, {5});
239+
test.AddInput<int32_t>("crop_size", {2}, {2, 2});
240+
test.AddOutput<float>("output", {1, 1, 2, 2}, {0.0f, 0.0f, 0.0f, 0.0f});
241+
test.Run(OpTester::ExpectResult::kExpectFailure,
242+
"is out of range [0, 1)", {kTensorrtExecutionProvider});
243+
}
244+
126245
} // namespace test
127246
} // namespace onnxruntime

0 commit comments

Comments
 (0)