[AhmadHassan-BTed]

Summary

Fixes #23955.

This PR resolves null pointer dereference crashes encountered during runtime error logging and external tensor loading on malformed or minimal .pte and .ptd inputs:

1. Method::execute_instruction (runtime/executor/method.cpp):
   When logging a KernelCall failed error, the logger formatted op->overload()->c_str() unconditionally. If an operator has no overload string in the .pte flatbuffer (or if op is null), calling c_str() results in a SIGSEGV instead of emitting the error diagnostic.
   We guard op, op->name(), and check has_overload = op != nullptr && op->overload() != nullptr && op->overload()->size() > 0, formatting as op_name.overload when present and op_name when absent, matching populate_operator_name().

2. FlatTensorDataMap (extension/flat_tensor/flat_tensor_data_map.cpp):

   • In get_named_data(), linear key lookup dereferenced named_data->Get(i)->key()->size() without checking if the entry or key() is null. Corrupted entries now skip safely.
   • In get_num_keys(), return 0 if flat_tensor_ or named_data is null.
   • In get_key(), validate index against num_keys and verify entry != nullptr && entry->key() != nullptr, returning Error::InvalidExternalData on malformed entries rather than dereferencing null.

cc @larryliu0820 @JacobSzwejbka @lucylq @nil-is-all

Test plan

• Built and ran extension_flat_tensor_test (20/20 tests passed):

  ET_MODULE_ADD_MUL_DATA_PATH=build-test/extension/flat_tensor/test/ModuleAddMulProgram.ptd \
    ./build-test/extension/flat_tensor/test/extension_flat_tensor_test

• Built and ran runtime_executor_test (13/13 tests passed):

  ./build-test/runtime/executor/test/runtime_executor_test

• Verified out-of-bounds and numeric limit checks for FlatTensorDataMap::get_key.

[pytorch-bot]

🔗 Helpful Links

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

• 📄 Preview Python docs built from this PR

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

⚠️ 29 Awaiting Approval

As of commit cea98b1 with merge base 0015c76 ():

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

• Build Aarch64 Linux CUDA Wheels (gh)
• Build Aarch64 Linux Native Wheels (gh)
• Build Aarch64 Linux Wheels (gh)
• Build documentation (gh)
• Build Linux CUDA Wheels (gh)
• Build Linux Native Wheels (gh)
• Build Linux Wheels (gh)
• Build macOS Wheels (gh)
• Build Presets (gh)
• Build Windows CUDA Wheels (gh)
• Build Windows Wheels (gh)
• Cadence Build & Test (gh)
• Core AI (gh)
• Lint (gh)
• periodic (gh)
• pull (gh)
• Test ARM Backend (gh)
• Test CoreML Backend (gh)
• Test Cortex-M Backend (gh)
• Test CPU Backend (gh)
• Test ESP32 Emulators (gh)
• Test Native Engines (gh)
• Test NXP Backend (gh)
• Test QNN Backend (gh)
• Test QNN Windows MSVC build (gh)
• Test Vulkan Backend (gh)
• Test Vulkan Backend (specialized runners) (gh)
• Test WebGPU Backend (gh)
• Test XNNPACK Backend (gh)

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

[meta-cla]

Hi @AhmadHassan-BTed!

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]

The committers listed above are authorized under a signed CLA.

• ✅ login: AhmadHassan-BTed / name: Ahmad Hassan (B-Ted) (a880b61)

[AhmadHassan-BTed]

@pytorchbot label "release notes: runtime"

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota. Try requesting a review at a lower effort level.

[meta-cla]

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

[JakeStevens]

@claude review effort:max

[claude]

Claude finished @JakeStevens's task in 3m 4s —— View job

---

PR Review: #24801

This PR fixes the null overload() crash in the KernelCall failure log, but it will likely break -Werror builds that have logging turned off, it skips malformed named-data entries silently instead of rejecting them, and none of the new paths are tested. Request Changes.

Detailed review

Summary

The PR adds null guards to the Method::execute_instruction KernelCall error log and to the FlatTensorDataMap key lookup and enumeration. Of these guards, only two can actually trigger: op->overload() == nullptr and entry->key() == nullptr. One of the new guards changes the error semantics, the method.cpp change probably breaks the build, and the new test passes on the old code too.

Code Quality

• runtime/executor/method.cpp:1735-1740: build break when logging is compiled out (major).

  • Cause: op_name and op_overload are only read inside ET_LOG. With ET_LOG_ENABLED=0, ET_LOG expands to ((void)0) (runtime/platform/log.h:206). That leaves both as unused locals, and -Wall warns about them through -Wunused-variable.
  • Trigger: test/build_size_test.sh builds Release with -Wall -Werror. In Release, EXECUTORCH_ENABLE_LOGGING defaults off (tools/cmake/preset/default.cmake:18-21).
  • Why op didn't hit this: it is marked ET_UNUSED for exactly this reason, and the new locals didn't get the same treatment.
  • Fix: put the expressions inline in the ET_LOG arguments, the same way populate_operator_name does at lines 882-888. That also removes the extra locals.

  Fix this →

• extension/flat_tensor/flat_tensor_data_map.cpp:66-69: an entry with no key is skipped silently instead of rejected.

  • Effect: in a .ptd with a keyless NamedData entry, get_data / get_tensor_layout / load_data_into now return NotFound. Callers read that as "key absent", which hides the corruption.
  • Inconsistent within this PR: get_key() on the same index returns InvalidExternalData, so the same file is valid or invalid depending on which API you call.
  • Inconsistent with the sibling class: PteDataMap::get_data (runtime/executor/pte_data_map.cpp:31-37) fails on a null item or key rather than skipping it.
  • Fix: use ET_CHECK_OR_RETURN_ERROR(..., InvalidExternalData, ...) here, matching the other "Malformed PTD" checks in this function.
  • Smaller point: PteDataMap::get_key returns InvalidArgument for a null key, while this PR uses InvalidExternalData. Pick one on purpose so the NamedDataMap implementations agree.

• Most of the new guards can never fire, which goes against the "simpler, more concise" rule in CLAUDE.md:

  • method.cpp:1736-1738: the op != nullptr / op->name() != nullptr checks and the "unknown" fallback are dead.
    • resolve_operator (lines 915-926) already bounds-checks op_index at init.
    • populate_operator_name (lines 873-874) fails init with InvalidProgram when the name is null.
    • Vector<Offset<T>>::Get() never returns null.
    • Only the overload guard is needed.
  • flat_tensor_data_map.cpp:67, 280: entry == nullptr is dead for the same Get() reason.
  • flat_tensor_data_map.cpp:255-257: get_num_keys's null branch is dead.
    • The constructor is private, and load() (line 415) already rejects a null named_data().
    • If the branch could run, returning 0 would make a corrupt file look like an empty map. That would hide the error instead of reporting it.
  • flat_tensor_data_map.cpp:263-266, 274-278: two more dead checks in get_key.
    • The !num_keys_res.ok() branch is dead, because get_num_keys never fails.
    • The named_data != nullptr check is also unreachable: if named_data were null, num_keys would be 0 and the bounds check would already have returned InvalidArgument.
  • flat_tensor_data_map.cpp:75: const auto* found = entry; is a leftover alias. Use entry directly.

Testing

• extension/flat_tensor/test/flat_tensor_data_map_test.cpp:137-139: the new assertion doesn't test anything the PR changed.
  • It already passed before this PR: the old index >= 0 && index < num_keys rejected UINT32_MAX as well.
  • Missing: a test with a NamedData entry that has no key, covering both get_named_data and get_key. It can be built with FlatBufferBuilder and flat_tensor_generated.h, which the test already includes.
  • Missing: a runtime/executor test where a kernel fails on an operator whose overload is empty or absent. That is the crash reported in executor_runner segfaults in portable kernels when a .pte tensor has neither a data buffer nor a memory plan (null data pointer) #23955.

Recommendation

Request Changes: the likely build break with ET_LOG_ENABLED=0 and -Werror blocks merge. Also, please reject keyless entries instead of skipping them, drop the guards that can't fire, and add tests for the two null paths that can actually trigger.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota. Try requesting a review at a lower effort level.

[AhmadHassan-BTed]

Thanks for the review @JakeStevens. Updated the PR to address all feedback:

• Logging and unused variables (method.cpp): Inlined the operator overload null guard directly into the ET_LOG arguments. This eliminates local variables that trigger -Wunused-variable when logging is compiled out (ET_LOG_ENABLED=0).
• Keyless named data error reporting (flat_tensor_data_map.cpp): In get_named_data, replaced the silent continue with an ET_CHECK_OR_RETURN_ERROR returning InvalidExternalData when an entry or its key is null. In get_key, aligned error behavior to also return InvalidExternalData on a null key.
• Dead checks removed: Simplified get_num_keys to return flat_tensor_->named_data()->size() directly. In get_key, removed the unreachable named_data == nullptr check and dead error branch. Removed the redundant found alias in get_named_data.
• Test coverage:
  • Added FlatTensorDataMapTest.KeylessNamedDataReturnsInvalidExternalData in extension/flat_tensor/test/flat_tensor_data_map_test.cpp, testing that a keyless NamedData entry returns InvalidExternalData from get_key, get_data, and get_tensor_layout.
  • Added ProgramValidationTest.KernelCallFailureWithNullOverloadDoesNotCrash in runtime/executor/test/program_validation_test.cpp, creating an operator with null overload, registering a failing kernel, and asserting Method::execute() logs the error and returns Error::Internal without segfaulting.
• Code formatting: Verified clean with git-clang-format main across all modified files.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota. Try requesting a review at a lower effort level.