Skip to content

Commit 6847568

Browse files
bmehta001Copilot
andcommitted
Fix GatherND sentinel-value collision enabling OOB memory read
GatherND used err_index==0 as a sentinel for 'no validation error', but 0 is also a valid tensor index. When a dimension has size 0, index 0 correctly fails the bounds check (0 >= 0), but err_index is set to 0 which is indistinguishable from success. This allowed the operator to proceed with an out-of-bounds memcpy, leaking process heap memory. Replace the sentinel pattern with a std::atomic<bool> error flag that is independent of the index value. The err_index variable is retained solely for the error message. Files changed: - onnxruntime/core/providers/cpu/tensor/gather_nd.cc - onnxruntime/test/providers/cpu/tensor/gather_nd_op_test.cc Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent ff87826 commit 6847568

2 files changed

Lines changed: 46 additions & 2 deletions

File tree

onnxruntime/core/providers/cpu/tensor/gather_nd.cc

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
// Copyright (c) Microsoft Corporation. All rights reserved.
22
// Licensed under the MIT License.
3+
#include <atomic>
34
#include <core/common/safeint.h>
45
#include "gather_nd.h"
56
#include "core/platform/threadpool.h"
@@ -85,6 +86,11 @@ Status GatherNDBase::PrepareForCompute(const TensorShape& input_shape, const Ten
8586
sizes_from_slice_dims[onnxruntime::narrow<size_t>(i)] = input_shape.SizeFromDimension(SafeInt<size_t>(batch_dims_) + i + 1);
8687
}
8788

89+
// Use a separate flag to track validation errors instead of relying on the
90+
// index value as a sentinel. The value 0 is a valid tensor index, so using
91+
// err_index == 0 to mean "no error" creates a collision when a zero-sized
92+
// dimension makes index 0 invalid — the error goes undetected.
93+
std::atomic<bool> has_invalid_index{false};
8894
int64_t err_index = 0;
8995
p.element_bytes = bytes_per_value;
9096
p.element_count_per_slice = slice_size;
@@ -104,6 +110,7 @@ Status GatherNDBase::PrepareForCompute(const TensorShape& input_shape, const Ten
104110
const auto upper_limit = input_shape[SafeInt<size_t>(batch_dims_) + dim_idx];
105111
const auto lower_limit = -upper_limit;
106112
if (index < lower_limit || index >= upper_limit) {
113+
has_invalid_index.store(true, std::memory_order_relaxed);
107114
err_index = index;
108115
break;
109116
}
@@ -123,8 +130,9 @@ Status GatherNDBase::PrepareForCompute(const TensorShape& input_shape, const Ten
123130
}
124131
});
125132

126-
return err_index == 0 ? Status::OK()
127-
: ORT_MAKE_STATUS(ONNXRUNTIME, INVALID_ARGUMENT, "invalid index found, index = ", err_index);
133+
return !has_invalid_index.load(std::memory_order_relaxed)
134+
? Status::OK()
135+
: ORT_MAKE_STATUS(ONNXRUNTIME, INVALID_ARGUMENT, "invalid index found, index = ", err_index);
128136
}
129137

130138
template Status GatherNDBase::PrepareForCompute<int32_t>(const TensorShape&,

onnxruntime/test/providers/cpu/tensor/gather_nd_op_test.cc

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -372,5 +372,41 @@ TEST(GatherNDOpTest, GatherND_zero_batch_dims_error) {
372372
&cpu_only_ep); // force CPU
373373
}
374374

375+
// Regression test: index 0 into a zero-sized dimension must be rejected.
376+
// Previously err_index==0 was used as a sentinel for "no error", which collided
377+
// with the actual index value 0, allowing an OOB memory read.
378+
TEST(GatherNDOpTest, GatherND_zero_dim_index_zero_rejected) {
379+
OpTester test("GatherND", 12, kOnnxDomain);
380+
test.AddAttribute<int64_t>("batch_dims", 0);
381+
382+
// data shape [0]: zero-sized first dimension
383+
// indices [0]: index 0 is invalid because dimension size is 0
384+
test.AddInput<int32_t>("data", {0}, {});
385+
test.AddInput<int64_t>("indices", {1}, {0});
386+
test.AddOutput<int32_t>("output", {}, {0}); // dummy, won't be used
387+
388+
std::vector<std::unique_ptr<onnxruntime::IExecutionProvider>> cpu_only_ep;
389+
cpu_only_ep.push_back(DefaultCpuExecutionProvider());
390+
391+
test.Run(OpTester::ExpectResult::kExpectFailure,
392+
"invalid index found",
393+
{},
394+
nullptr,
395+
&cpu_only_ep);
396+
}
397+
398+
// Verify that index 0 into a non-zero dimension still works correctly.
399+
TEST(GatherNDOpTest, GatherND_valid_index_zero) {
400+
OpTester test("GatherND", 12, kOnnxDomain);
401+
test.AddAttribute<int64_t>("batch_dims", 0);
402+
403+
// data shape [3]: valid dimension, index 0 should return the first element
404+
test.AddInput<int32_t>("data", {3}, {10, 20, 30});
405+
test.AddInput<int64_t>("indices", {1}, {0});
406+
test.AddOutput<int32_t>("output", {}, {10});
407+
408+
test.Run();
409+
}
410+
375411
} // namespace test
376412
} // namespace onnxruntime

0 commit comments

Comments
 (0)