Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/17131
Note: Links to docs will display an error until the docs builds have been completed. ❌ 1 New Failure, 2 Unrelated FailuresAs of commit 024b63e with merge base 4c56d9b ( NEW FAILURE - The following job has failed:
BROKEN TRUNK - The following jobs failed but were present on the merge base:👉 Rebase onto the `viable/strict` branch to avoid these failures
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
meta-cla
Bot
added
the
CLA Signed
This PR needs a
|
There was a problem hiding this comment.
Pull request overview
This PR hardens tensor layout validation against malformed serialized tensors by ensuring dim_order and sizes have consistent lengths, preventing an out-of-bounds read in validateTensorLayout.
Changes:
- Added a dimensionality check in
validateTensorLayoutto requires_tensor->dim_order()->size()to match the tensor rank before iterating overdim_order. - Introduced a focused unit test that builds a tensor with
sizeslonger thandim_orderand asserts thatvalidateTensorLayoutreturnsError::InvalidExternalData. - Wired up the test harness with the
tensor_layoutAPI by adding the appropriate includes andusingdeclarations.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
runtime/executor/tensor_parser_exec_aten.cpp |
Tightens validateTensorLayout by checking that dim_order length equals the tensor dimension before accessing dim_order()[i], eliminating the out-of-bounds read for short dim_order vectors. |
runtime/executor/test/tensor_parser_test.cpp |
Adds a regression test for the dim-order/size mismatch case and includes TensorLayout/validateTensorLayout so that the new behavior is exercised. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ET_CHECK_OR_RETURN_ERROR( | ||
| s_tensor->dim_order()->size() == static_cast<size_t>(dim), | ||
| InvalidExternalData, | ||
| "Dim order size mismatch. Expected %d, got %zu.", | ||
| dim, | ||
| s_tensor->dim_order()->size()); |
There was a problem hiding this comment.
TensorLayout::create returns a Result<const TensorLayout>, but this function calls s_tensor->dim_order()->size() and later s_tensor->dim_order()->Get(i) without first checking that s_tensor->dim_order() is non-null. If a malformed or tampered flatbuffer omits the dim_order field, these dereferences will crash instead of returning Error::InvalidExternalData; consider adding an ET_CHECK_OR_RETURN_ERROR that s_tensor->sizes() != nullptr and s_tensor->dim_order() != nullptr (similar to the checks in tensor_parser_aten.cpp) before using them.
There was a problem hiding this comment.
This is validated before going into validateTensorLayout, see: https://github.com/pytorch/executorch/blob/main/runtime/executor/tensor_parser_aten.cpp#L69
https://github.com/pytorch/executorch/blob/main/runtime/executor/tensor_parser_portable.cpp#L73
^ before calling getTensorDataPtr, which calls validateTensorLayout
lucylq
force-pushed
the
lfq.security-16
branch
from
fce9ecf to
5439192
Compare
lucylq
force-pushed
the
lfq.security-16
branch
from
5439192 to
7862004
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
### Summary check sizes and dim are same length ### Test plan ``` cmake --build . --target tensor_parser_test ctest -R tensor_parser_test -V ```
### Summary check sizes and dim are same length ### Test plan ``` cmake --build . --target tensor_parser_test ctest -R tensor_parser_test -V ```
### Summary check sizes and dim are same length ### Test plan ``` cmake --build . --target tensor_parser_test ctest -R tensor_parser_test -V ```
Summary
check sizes and dim are same length
Test plan