Skip to content

Commit 77b687d

Browse files
authored
Validate Method input layouts for non-contiguous tensors (#23820)
### Summary Fixes #18562 `Method::set_input` now checks the source tensor's strides against the method input's expected layout (after the input is resized, so the check matches what kernels read in both portable and ATen mode). A source that explicitly describes a different layout, such as a permuted view, padded strides, or a mismatched dim order, is rejected with `InvalidArgument` instead of being silently reinterpreted. A source with the default contiguous layout is still accepted, with a log message, when the method expects a different dim order: existing callers (plain `from_blob`, `make_tensor_ptr`, Android `Tensor.fromBlob`, the Core ML example runner) wrap data that is already in the model's layout in a default tensor just to pass its shape, and the earlier check in #23358 was backed out (#23595) for rejecting exactly those inputs. Android `Tensor.fromBlob` also accepts an explicit dim order and plumbs it through JNI, so channels-last inputs can be described correctly from Kotlin/Java and callers have a path to stop relying on the contiguous fallback. Native-created tensors report their dim order back, and `toByteArray`/`fromByteArray` preserve it. The pybindings layout conversion was dropped since main already handles it (#23957). Review order: the runtime validation commit first, then the Android Tensor commit. This PR was authored with AI assistance (Claude Code); the merge conflict resolution used Codex. ### Test plan Added a channels-last test model (`ModuleChannelsLast`) and `method_test` cases covering a matching channels-last input (accepted and executed), a mismatched explicit layout (rejected), and a default contiguous input for a channels-last method (accepted). ```bash cmake --build build --target method_test -j "$(sysctl -n hw.ncpu)" ctest --test-dir build -R '^method_test$' --output-on-failure ``` Also ran the full CMake `ctest` suite; the only failures were unrelated environment issues (temp-file creation in `extension_data_loader_test`, and a `method_meta_test` model export that needs an up-to-date Python install). Added a `TensorTest` round-trip case for `dimOrder` serialization; Android tests are covered by CI. After merging main, rebuilt `method_test` from a clean source snapshot and reran it successfully, with all 28 enabled cases passing, covering both input bounds and layout validation. Formatting and `git diff --check` also passed.
1 parent b50c571 commit 77b687d

11 files changed

Lines changed: 489 additions & 66 deletions

File tree

‎extension/android/executorch_android/consumer-proguard-rules.pro‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,8 @@
3232
-keep class org.pytorch.executorch.EValue { *; }
3333

3434
# Keep Tensor and its inner classes. The Tensor class has methods and fields
35-
# accessed from JNI (dtypeJniCode, shape, getRawDataBuffer, nativeNewTensor).
35+
# accessed from JNI (dtypeJniCode, shape, dimOrder, getRawDataBuffer,
36+
# nativeNewTensor).
3637
-keep class org.pytorch.executorch.Tensor { *; }
3738
-keep class org.pytorch.executorch.Tensor$* { *; }
3839

‎extension/android/executorch_android/src/main/java/org/pytorch/executorch/Tensor.kt‎

Lines changed: 123 additions & 57 deletions
Large diffs are not rendered by default.

‎extension/android/executorch_android/src/test/java/org/pytorch/executorch/TensorTest.kt‎

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
*/
88
package org.pytorch.executorch
99

10+
import java.nio.ByteBuffer
1011
import java.nio.ByteOrder
1112
import org.assertj.core.api.Assertions.assertThatThrownBy
1213
import org.junit.Assert.assertArrayEquals
@@ -639,6 +640,59 @@ class TensorTest {
639640
.hasMessage("Inconsistent data capacity:4 and shape number elements:2 shape:[1, 2]")
640641
}
641642

643+
@Test
644+
fun testDimOrder() {
645+
val shape = longArrayOf(1, 3, 2, 2)
646+
val dimOrder = intArrayOf(0, 2, 3, 1)
647+
val data = FloatArray(12) { it.toFloat() }
648+
649+
assertArrayEquals(dimOrder, Tensor.fromBlob(data, shape, dimOrder).dimOrder())
650+
651+
val buffer = Tensor.allocateFloatBuffer(data.size)
652+
buffer.put(data)
653+
assertArrayEquals(dimOrder, Tensor.fromBlob(buffer, shape, dimOrder).dimOrder())
654+
assertArrayEquals(intArrayOf(0, 1, 2, 3), Tensor.fromBlob(data, shape).dimOrder())
655+
656+
assertThatThrownBy { Tensor.fromBlob(data, shape, intArrayOf(0, 1, 2)) }
657+
.isInstanceOf(IllegalArgumentException::class.java)
658+
assertThatThrownBy { Tensor.fromBlob(data, shape, intArrayOf(0, 1, 2, 2)) }
659+
.isInstanceOf(IllegalArgumentException::class.java)
660+
}
661+
662+
@Test
663+
fun testDimOrderSerde() {
664+
val shape = longArrayOf(1, 3, 2, 2)
665+
val dimOrder = intArrayOf(0, 2, 3, 1)
666+
val data = FloatArray(12) { it.toFloat() }
667+
668+
val deser = Tensor.fromByteArray(Tensor.fromBlob(data, shape, dimOrder).toByteArray())
669+
670+
assertArrayEquals(shape, deser.shape())
671+
assertArrayEquals(dimOrder, deser.dimOrder())
672+
assertArrayEquals(data, deser.dataAsFloatArray, 0f)
673+
}
674+
675+
@Test
676+
fun testSerdeWithoutDimOrder() {
677+
val shape = longArrayOf(2, 2)
678+
val data = intArrayOf(1, 2, 3, 4)
679+
val buffer = ByteBuffer.allocate(1 + 1 + 4 * shape.size + 4 * data.size)
680+
buffer.put(DType.INT32.jniCode.toByte())
681+
buffer.put(shape.size.toByte())
682+
for (s in shape) {
683+
buffer.putInt(s.toInt())
684+
}
685+
for (d in data) {
686+
buffer.putInt(d)
687+
}
688+
689+
val deser = Tensor.fromByteArray(buffer.array())
690+
691+
assertArrayEquals(shape, deser.shape())
692+
assertArrayEquals(intArrayOf(0, 1), deser.dimOrder())
693+
assertArrayEquals(data, deser.dataAsIntArray)
694+
}
695+
642696
@Test
643697
fun testLongTensorSerde() {
644698
val data = longArrayOf(1, 2, 3, 4)

‎extension/android/jni/jni_layer.cpp‎

Lines changed: 56 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -60,8 +60,6 @@ class TensorHybrid : public facebook::jni::HybridClass<TensorHybrid> {
6060

6161
static facebook::jni::local_ref<TensorHybrid::javaobject>
6262
newJTensorFromTensor(const executorch::aten::Tensor& tensor) {
63-
// Java wrapper currently only supports contiguous tensors.
64-
6563
const auto scalarType = tensor.scalar_type();
6664
if (scalar_type_to_java_dtype.count(scalarType) == 0) {
6765
std::stringstream ss;
@@ -83,6 +81,22 @@ class TensorHybrid : public facebook::jni::HybridClass<TensorHybrid> {
8381
jTensorShape->setRegion(
8482
0, tensor_shape_vec.size(), tensor_shape_vec.data());
8583

84+
std::vector<jint> dim_order_vec;
85+
dim_order_vec.reserve(tensor_shape_vec.size());
86+
const auto tensor_dim_order = tensor.dim_order();
87+
if (tensor_dim_order.size() == tensor_shape_vec.size()) {
88+
for (const auto& d : tensor_dim_order) {
89+
dim_order_vec.push_back(static_cast<jint>(d));
90+
}
91+
} else {
92+
for (size_t i = 0; i < tensor_shape_vec.size(); ++i) {
93+
dim_order_vec.push_back(static_cast<jint>(i));
94+
}
95+
}
96+
facebook::jni::local_ref<facebook::jni::JArrayInt> jDimOrder =
97+
facebook::jni::make_int_array(dim_order_vec.size());
98+
jDimOrder->setRegion(0, dim_order_vec.size(), dim_order_vec.data());
99+
86100
static auto cls = TensorHybrid::javaClassStatic();
87101
// Note: this is safe as long as the data stored in tensor is valid; the
88102
// data won't go out of scope as long as the Method for the inference is
@@ -97,10 +111,16 @@ class TensorHybrid : public facebook::jni::HybridClass<TensorHybrid> {
97111
cls->getStaticMethod<facebook::jni::local_ref<TensorHybrid::javaobject>(
98112
facebook::jni::alias_ref<facebook::jni::JByteBuffer>,
99113
facebook::jni::alias_ref<jlongArray>,
114+
facebook::jni::alias_ref<facebook::jni::JArrayInt>,
100115
jint,
101116
facebook::jni::alias_ref<jhybriddata>)>("nativeNewTensor");
102117
return jMethodNewTensor(
103-
cls, jTensorBuffer, jTensorShape, jdtype, makeCxxInstance(tensor));
118+
cls,
119+
jTensorBuffer,
120+
jTensorShape,
121+
jDimOrder,
122+
jdtype,
123+
makeCxxInstance(tensor));
104124
}
105125

106126
static TensorPtr newTensorFromJTensor(
@@ -112,6 +132,10 @@ class TensorHybrid : public facebook::jni::HybridClass<TensorHybrid> {
112132
static const auto shapeField = cls->getField<jlongArray>("shape");
113133
auto jshape = jtensor->getFieldValue(shapeField);
114134

135+
static const auto dimOrderField =
136+
cls->getField<facebook::jni::JArrayInt>("dimOrder");
137+
auto jdim_order = jtensor->getFieldValue(dimOrderField);
138+
115139
static auto dataBufferMethod = cls->getMethod<
116140
facebook::jni::local_ref<facebook::jni::JBuffer::javaobject>()>(
117141
"getRawDataBuffer");
@@ -131,6 +155,29 @@ class TensorHybrid : public facebook::jni::HybridClass<TensorHybrid> {
131155
for (int i = rank - 1; i >= 0; --i) {
132156
numel *= shapeArr[i];
133157
}
158+
159+
std::vector<executorch::aten::DimOrderType> dim_order_vec;
160+
dim_order_vec.reserve(rank);
161+
if (!jdim_order || jdim_order->size() != rank) {
162+
jni_helper::throwExecutorchException(
163+
static_cast<uint32_t>(Error::InvalidArgument),
164+
"Tensor dim_order size must match shape size");
165+
return nullptr;
166+
}
167+
const auto dimOrderArr = jdim_order->getRegion(0, rank);
168+
std::vector<char> seen_dim(rank, 0);
169+
for (int i = 0; i < rank; i++) {
170+
const jint d = dimOrderArr[i];
171+
if (d < 0 || d >= static_cast<jint>(rank) || seen_dim[d] != 0) {
172+
jni_helper::throwExecutorchException(
173+
static_cast<uint32_t>(Error::InvalidArgument),
174+
"Tensor dim_order must be a permutation of shape dimensions");
175+
return nullptr;
176+
}
177+
seen_dim[d] = 1;
178+
dim_order_vec.push_back(static_cast<executorch::aten::DimOrderType>(d));
179+
}
180+
134181
JNIEnv* jni = facebook::jni::Environment::current();
135182
if (java_dtype_to_scalar_type.count(jdtype) == 0) {
136183
std::stringstream ss;
@@ -163,8 +210,12 @@ class TensorHybrid : public facebook::jni::HybridClass<TensorHybrid> {
163210
static_cast<uint32_t>(Error::InvalidArgument), ss.str().c_str());
164211
return nullptr;
165212
}
166-
return from_blob(
167-
jni->GetDirectBufferAddress(jbuffer.get()), shape_vec, scalar_type);
213+
return for_blob(
214+
jni->GetDirectBufferAddress(jbuffer.get()),
215+
shape_vec,
216+
scalar_type)
217+
.dim_order(std::move(dim_order_vec))
218+
.make_tensor_ptr();
168219
}
169220

170221
private:

‎runtime/core/portable_type/tensor_impl.h‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -175,11 +175,17 @@ class TensorImpl {
175175

176176
/// Returns the order the dimensions are laid out in memory.
177177
const ArrayRef<DimOrderType> dim_order() const {
178+
if (dim_order_ == nullptr) {
179+
return {};
180+
}
178181
return ArrayRef<DimOrderType>{dim_order_, static_cast<size_t>(dim_)};
179182
}
180183

181184
/// Returns the strides of the tensor at each dimension.
182185
const ArrayRef<StridesType> strides() const {
186+
if (strides_ == nullptr) {
187+
return {};
188+
}
183189
return ArrayRef<StridesType>{strides_, static_cast<size_t>(dim_)};
184190
}
185191

‎runtime/executor/method.cpp‎

Lines changed: 85 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -755,6 +755,87 @@ Error Method::parse_values(const NamedDataMap* external_data_map) {
755755
}
756756

757757
namespace {
758+
/**
759+
* Validates that t_src's physical layout matches t_dst's, which must already
760+
* be resized to t_src's sizes. set_input() copies or shares t_src's buffer
761+
* under t_dst's layout, so a mismatched source would be silently
762+
* reinterpreted. A source with the default contiguous layout is only warned
763+
* about, since callers commonly wrap data that is already in the method's
764+
* layout in a default tensor just to pass its shape.
765+
*/
766+
ET_NODISCARD Error validate_input_tensor_layout(
767+
const executorch::aten::Tensor& t_dst,
768+
const executorch::aten::Tensor& t_src,
769+
size_t input_idx) {
770+
const auto dim = t_src.dim();
771+
if (dim == 0 || t_src.numel() == 0) {
772+
return Error::Ok;
773+
}
774+
775+
auto src_strides = t_src.strides();
776+
#ifndef USE_ATEN_LIB
777+
std::array<executorch::aten::StridesType, kTensorDimensionLimit>
778+
derived_strides{};
779+
if (src_strides.empty()) {
780+
ET_CHECK_OR_RETURN_ERROR(
781+
static_cast<size_t>(dim) <= kTensorDimensionLimit,
782+
InvalidArgument,
783+
"Input %" ET_PRIsize_t " has unsupported rank %zd.",
784+
input_idx,
785+
dim);
786+
std::array<executorch::aten::DimOrderType, kTensorDimensionLimit>
787+
default_dim_order{};
788+
for (ssize_t i = 0; i < dim; ++i) {
789+
default_dim_order[i] = static_cast<executorch::aten::DimOrderType>(i);
790+
}
791+
const auto src_dim_order = t_src.dim_order();
792+
ET_CHECK_OK_OR_RETURN_ERROR(
793+
dim_order_to_stride(
794+
t_src.sizes().data(),
795+
src_dim_order.empty() ? default_dim_order.data()
796+
: src_dim_order.data(),
797+
dim,
798+
derived_strides.data()),
799+
"Failed to compute strides for input %" ET_PRIsize_t,
800+
input_idx);
801+
src_strides = {derived_strides.data(), static_cast<size_t>(dim)};
802+
}
803+
#endif
804+
805+
const auto dst_strides = t_dst.strides();
806+
bool matches = true;
807+
bool src_is_contiguous = true;
808+
int64_t contiguous_stride = 1;
809+
for (ssize_t i = dim - 1; i >= 0; --i) {
810+
if (t_src.size(i) > 1) {
811+
matches = matches && src_strides[i] == dst_strides[i];
812+
src_is_contiguous =
813+
src_is_contiguous && src_strides[i] == contiguous_stride;
814+
}
815+
contiguous_stride *= t_src.size(i);
816+
}
817+
818+
if (matches) {
819+
return Error::Ok;
820+
}
821+
if (src_is_contiguous) {
822+
ET_LOG(
823+
Debug,
824+
"Input %" ET_PRIsize_t
825+
" has the default contiguous layout but the method expects a "
826+
"different dim order; treating its data as already in the expected "
827+
"layout. Pass a tensor with the method's dim order to avoid this.",
828+
input_idx);
829+
return Error::Ok;
830+
}
831+
ET_LOG(
832+
Error,
833+
"Input %" ET_PRIsize_t
834+
" has strides that do not match the method's expected layout.",
835+
input_idx);
836+
return Error::InvalidArgument;
837+
}
838+
758839
/**
759840
* Private/helper method for populating operator_name from the Operator.
760841
* operator_name is a char pointer that is already allocated. The size of
@@ -1380,6 +1461,10 @@ Method::set_input(const EValue& input_evalue, size_t input_idx) {
13801461
resize_tensor(t_dst, t_src.sizes()),
13811462
"Error resizing tensor at input %" ET_PRIsize_t,
13821463
input_idx);
1464+
ET_CHECK_OK_OR_RETURN_ERROR(
1465+
validate_input_tensor_layout(t_dst, t_src, input_idx),
1466+
"Error validating tensor layout at input %" ET_PRIsize_t,
1467+
input_idx);
13831468
if (tensor_meta->is_memory_planned()) {
13841469
ET_CHECK_OK_OR_RETURN_ERROR(
13851470
internal::copy_tensor_data(t_dst, t_src),

‎runtime/executor/test/CMakeLists.txt‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@ if(CMAKE_HOST_SYSTEM_NAME STREQUAL "Windows")
2424
powershell
2525
${EXECUTORCH_ROOT}/kernels/test/export_test_model.ps1
2626
-Modules
27-
"\"ModuleAdd,ModuleAddHalf,ModuleAddMul,ModuleDynamicCatUnallocatedIO,ModuleIndex,ModuleMultipleEntry,ModuleSimpleTrain,ModuleStateful\""
27+
"\"ModuleAdd,ModuleAddHalf,ModuleAddMul,ModuleChannelsLast,ModuleDynamicCatUnallocatedIO,ModuleIndex,ModuleMultipleEntry,ModuleSimpleTrain,ModuleStateful\""
2828
-outDir
2929
"${CMAKE_CURRENT_BINARY_DIR}"
3030
-CondaEnv
@@ -36,7 +36,7 @@ else()
3636
-m
3737
test.models.export_program
3838
--modules
39-
"ModuleAdd,ModuleAddHalf,ModuleAddMul,ModuleDynamicCatUnallocatedIO,ModuleIndex,ModuleMultipleEntry,ModuleSimpleTrain,ModuleStateful"
39+
"ModuleAdd,ModuleAddHalf,ModuleAddMul,ModuleChannelsLast,ModuleDynamicCatUnallocatedIO,ModuleIndex,ModuleMultipleEntry,ModuleSimpleTrain,ModuleStateful"
4040
--outdir
4141
"${CMAKE_CURRENT_BINARY_DIR}"
4242
)
@@ -48,6 +48,7 @@ add_custom_command(
4848
"${CMAKE_CURRENT_BINARY_DIR}/ModuleAddMul.pte"
4949
"${CMAKE_CURRENT_BINARY_DIR}/ModuleAddMulProgram.pte"
5050
"${CMAKE_CURRENT_BINARY_DIR}/ModuleAddMulProgram.ptd"
51+
"${CMAKE_CURRENT_BINARY_DIR}/ModuleChannelsLast.pte"
5152
"${CMAKE_CURRENT_BINARY_DIR}/ModuleDynamicCatUnallocatedIO.pte"
5253
"${CMAKE_CURRENT_BINARY_DIR}/ModuleIndex.pte"
5354
"${CMAKE_CURRENT_BINARY_DIR}/ModuleMultipleEntry.pte"
@@ -109,6 +110,7 @@ set(test_env
109110
"ET_MODULE_ADD_MUL_PATH=${CMAKE_CURRENT_BINARY_DIR}/ModuleAddMul.pte"
110111
"ET_MODULE_ADD_MUL_PROGRAM_PATH=${CMAKE_CURRENT_BINARY_DIR}/ModuleAddMulProgram.pte"
111112
"ET_MODULE_ADD_MUL_DATA_PATH=${CMAKE_CURRENT_BINARY_DIR}/ModuleAddMulProgram.ptd"
113+
"ET_MODULE_CHANNELS_LAST_PATH=${CMAKE_CURRENT_BINARY_DIR}/ModuleChannelsLast.pte"
112114
"ET_MODULE_DYNAMIC_CAT_UNALLOCATED_IO_PATH=${CMAKE_CURRENT_BINARY_DIR}/ModuleDynamicCatUnallocatedIO.pte"
113115
"ET_MODULE_INDEX_PATH=${CMAKE_CURRENT_BINARY_DIR}/ModuleIndex.pte"
114116
"ET_MODULE_MULTI_ENTRY_PATH=${CMAKE_CURRENT_BINARY_DIR}/ModuleMultipleEntry.pte"

0 commit comments

Comments
 (0)