Skip to content

[runtime] Enforce topological EValue ordering for TensorList/OptionalTensorList and null-check CompileSpec fields - #24059

Open
professor-moody wants to merge 1 commit into
pytorch:mainfrom
professor-moody:fix/tensor-list-forward-ref-and-compile-spec-null
Open

professor-moody wants to merge 1 commit into
pytorch:mainfrom
professor-moody:fix/tensor-list-forward-ref-and-compile-spec-null

Conversation

@professor-moody

@professor-moody professor-moody commented Oct 21, 2026 •

Copy link
Copy Markdown

Summary

In Method::parse_values() (runtime/executor/method.cpp), values_ is allocated as uninitialized memory from method_allocator (allocateList<EValue>(n_value)) and constructed sequentially via placement new (&values_[i]) as i advances from 0 to n_value - 1.

Currently, when serialized_value at index i is a TensorList or OptionalTensorList:

  1. Method::parse_values() passes n_value (the total array length) rather than i (the number of EValue elements already constructed so far) as values_len to deserialization::parseTensorList() and deserialization::parseListOptionalType<executorch::aten::Tensor>().
  2. validate_program() (runtime/executor/program_validation.cpp) checks item_idx < values->size() instead of item_idx < i for TensorList, and does not validate OptionalTensorList entries.

If a serialized TensorList or OptionalTensorList at index i contains a forward reference item_idx >= i, parseTensorList / parseListOptionalType accesses values_[item_idx] before placement new has constructed that slot. When the MemoryAllocator arena is reused across method loads, values_[item_idx] can hold stale EValue bytes from a prior load.

Additionally, BackendDelegate::PopulateCompileSpecs() (runtime/executor/method.cpp) dereferences compile_spec_in_program->key() and compile_spec_in_program->value() without null checks when either optional FlatBuffer field is omitted.

Changes

  • runtime/executor/method.cpp:
    • Pass i instead of n_value as the values_len bound to deserialization::parseTensorList() and deserialization::parseListOptionalType<executorch::aten::Tensor>() inside Method::parse_values().
    • Check compile_spec_in_program != nullptr && compile_spec_in_program->key() != nullptr && compile_spec_in_program->value() != nullptr in BackendDelegate::PopulateCompileSpecs().
  • runtime/executor/program_validation.cpp:
    • Require 0 <= item_idx < i for TensorList items in validate_program().
    • Add validation for OptionalTensorList items (item_idx == -1 or 0 <= item_idx < i referencing KernelTypes::Tensor or KernelTypes::Null).

Test Plan

  • Verified with regression tests covering:
    • Valid topological ordering (Tensor at index 0, TensorList([0]) at index 1, OptionalTensorList([-1, 0]) at index 2, and well-formed CompileSpec).
    • Forward-referencing TensorList at index 0 targeting Tensor at index 1 on a reused MemoryAllocator arena.
    • Forward-referencing OptionalTensorList at index 0 targeting Tensor at index 1 on a reused MemoryAllocator arena.
    • CompileSpec with omitted key field (nullptr).
  • Confirmed clean pass under both Release (-O3 -DNDEBUG) and ASan+UBSan (-fsanitize=address,undefined -g -O1).

Note: This patch was prepared with AI assistance and verified by human review and local test execution.

@pytorch-bot

pytorch-bot Bot commented Oct 21, 2026 •

Copy link
Copy Markdown

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/24059

Note: Links to docs will display an error until the docs builds have been completed.

⚠️ 16 Awaiting Approval

As of commit 730dc57 with merge base 3ef59e9 (image):

AWAITING APPROVAL - The following workflows need approval before CI can run:

This comment was automatically generated by Dr. CI and updates every 15 minutes.

@meta-cla

meta-cla Bot commented Oct 21, 2026

Copy link
Copy Markdown

Hi @professor-moody!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks!

@linux-foundation-easycla

linux-foundation-easycla Bot commented Oct 21, 2026 •

Copy link
Copy Markdown

CLA Signed
The committers listed above are authorized under a signed CLA.

  • ✅ login: professor-moody / name: m00dy (04c21eb)

@meta-cla

meta-cla Bot commented Oct 21, 2026

Copy link
Copy Markdown

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

@meta-cla meta-cla Bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Oct 21, 2026
@meta-cla

meta-cla Bot commented Oct 21, 2026

Copy link
Copy Markdown

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

@executorch-triage executorch-triage Bot added the community: contribution PRs coming from community (excluding hardware partners) label Oct 21, 2026
@nil-is-all nil-is-all added module: runtime Issues related to the core runtime and code under runtime/ release notes: runtime Changes related to the core runtime which loads the program methods, initializes delegates, and runs labels Oct 21, 2026
@nil-is-all

Copy link
Copy Markdown
Contributor

Thanks for the PR, @professor-moody. As the CI tests run, could you sign the easyCLA?

…TensorList and null-check CompileSpec fields

### Summary

In `Method::parse_values()` (`runtime/executor/method.cpp`), `values_` is allocated as uninitialized memory from `method_allocator` (`allocateList<EValue>(n_value)`) and constructed sequentially via placement `new (&values_[i])` as `i` advances from `0` to `n_value - 1`.

Currently, when `serialized_value` at index `i` is a `TensorList` or `OptionalTensorList`:
1. `Method::parse_values()` passes `n_value` (the total array length) rather than `i` (the number of `EValue` elements already constructed so far) as `values_len` to `deserialization::parseTensorList()` and `deserialization::parseListOptionalType<executorch::aten::Tensor>()`.
2. `validate_program()` (`runtime/executor/program_validation.cpp`) checks `item_idx < values->size()` instead of `item_idx < i` for `TensorList`, and does not validate `OptionalTensorList` entries.

If a serialized `TensorList` or `OptionalTensorList` at index `i` contains a forward reference `item_idx >= i`, `parseTensorList` / `parseListOptionalType` accesses `values_[item_idx]` before placement `new` has constructed that slot. When the `MemoryAllocator` arena is reused across method loads, `values_[item_idx]` can hold stale `EValue` bytes from a prior load.

Additionally, `BackendDelegate::PopulateCompileSpecs()` (`runtime/executor/method.cpp`) dereferences `compile_spec_in_program->key()` and `compile_spec_in_program->value()` without null checks when either optional FlatBuffer field is omitted.

### Changes

- **`runtime/executor/method.cpp`**:
  - Pass `i` instead of `n_value` as the `values_len` bound to `deserialization::parseTensorList()` and `deserialization::parseListOptionalType<executorch::aten::Tensor>()` inside `Method::parse_values()`.
  - Check `compile_spec_in_program != nullptr && compile_spec_in_program->key() != nullptr && compile_spec_in_program->value() != nullptr` in `BackendDelegate::PopulateCompileSpecs()`.
- **`runtime/executor/program_validation.cpp`**:
  - Require `0 <= item_idx < i` for `TensorList` items in `validate_program()`.
  - Add validation for `OptionalTensorList` items (`item_idx == -1` or `0 <= item_idx < i` referencing `KernelTypes::Tensor` or `KernelTypes::Null`).

### Test Plan

- Verified with regression tests covering:
  - Valid topological ordering (`Tensor` at index `0`, `TensorList([0])` at index `1`, `OptionalTensorList([-1, 0])` at index `2`, and well-formed `CompileSpec`).
  - Forward-referencing `TensorList` at index `0` targeting `Tensor` at index `1` on a reused `MemoryAllocator` arena.
  - Forward-referencing `OptionalTensorList` at index `0` targeting `Tensor` at index `1` on a reused `MemoryAllocator` arena.
  - `CompileSpec` with omitted `key` field (`nullptr`).
- Confirmed clean pass under both Release (`-O3 -DNDEBUG`) and ASan+UBSan (`-fsanitize=address,undefined -g -O1`).
@professor-moody
professor-moody force-pushed the fix/tensor-list-forward-ref-and-compile-spec-null branch from 04c21eb to 730dc57 Compare October 21, 2026 19:54

This branch has not been deployed

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

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. community: contribution PRs coming from community (excluding hardware partners) module: runtime Issues related to the core runtime and code under runtime/ release notes: runtime Changes related to the core runtime which loads the program methods, initializes delegates, and runs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants