Add bounds checking on buffer_idx for constant_buffer and constant_data in XNNPACK by lucylq · Pull Request #18820 · pytorch/executorch

lucylq · 2026-04-10T18:11:46Z

Check that buffer_idx is within bounds of flatbuffer_graph->constant_buffer() and flatbuffer_graph->constant_data(), whichever branch is chosen

Authored-with: Claude

pytorch-bot · 2026-04-10T18:11:52Z

🔗 Helpful Links

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

📄 Preview Python docs built from this PR

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

❗ 1 Active SEVs

There are 1 currently active SEVs. If your PR is affected, please view them below:

Rolling out OSDC (ARC) runners on pull workflow for PyTorch trunk commits

❌ 2 New Failures, 2 Unrelated Failures

As of commit 3f9dd63 with merge base 21d9c64 ():

NEW FAILURES - The following jobs have failed:

pull / test-samsung-models-linux / linux-job (gh)
test_w2l_fp16
pull / unittest / macos / macos-job (gh)
export/tests/test_target_recipes.py::TestTargetRecipes::test_mv3_model

BROKEN TRUNK - The following jobs failed but were present on the merge base:

👉 Rebase onto the `viable/strict` branch to avoid these failures

pull / unittest / windows / windows-job (gh) (trunk failure)
##[error]The operation was canceled.
pull / unittest-editable / windows / windows-job (gh) (trunk failure)
##[error]The operation was canceled.

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

github-actions · 2026-04-10T18:12:32Z

This PR needs a `release notes:` label

If your change should be included in the release notes (i.e. would users of this library care about this change?), please use a label starting with release notes:. This helps us keep track and include your important work in the next release notes.

To add a label, you can comment to pytorchbot, for example
@pytorchbot label "release notes: none"

For more information, see
https://github.com/pytorch/pytorch/wiki/PyTorch-AutoLabel-Bot#why-categorize-for-release-notes-and-how-does-it-work.

Copilot

Pull request overview

Adds bounds validation for buffer_idx when resolving constant tensor data in the XNNPACK flatbuffer path, preventing out-of-range access into constant_buffer / constant_data.

Changes:

Add null + bounds checks for flatbuffer_graph->constant_buffer() before indexing.
Add null + bounds checks for flatbuffer_graph->constant_data() before calling Get(buffer_idx).
Emit error logs and return nullptr on invalid indices.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-10T18:16:45Z

backends/xnnpack/runtime/XNNCompiler.cpp

+      if (cb == nullptr || buffer_idx >= cb->size()) {
+        ET_LOG(
+            Error,
+            "Invalid buffer_idx %u for constant_buffer of size %u",
+            buffer_idx,
+            cb ? cb->size() : 0);
+        return nullptr;
+      }


Returning nullptr on an out-of-bounds/nonexistent buffer_idx conflates “non-constant tensor” (buffer_idx==0) with “corrupt/invalid flatbuffer” (buffer_idx!=0 but invalid). Callers like defineTensor pass the returned pointer directly into xnn_define_tensor_value; with a non-zero constant_buffer_idx this can silently create an internal (non-constant) tensor instead of failing fast. Consider changing getConstantDataPtr to return a Result<const uint8_t*> (or add an explicit error flag) so invalid indices propagate as an Error, or at minimum enforce that buffer_idx!=0 implies non-null return and surface an error upstream.

Copilot · 2026-04-10T18:16:46Z

backends/xnnpack/runtime/XNNCompiler.cpp

+        return nullptr;
+      }
+      const auto& constant_buffer = *cb;
      return constant_buffer[buffer_idx]->storage()->data();


Even after the bounds check, constant_buffer[buffer_idx] (and/or its storage field) can still be null in a malformed flatbuffer, which would crash when dereferenced. Please add null checks for constant_buffer[buffer_idx] and constant_buffer[buffer_idx]->storage() before calling ->data(), and treat failure as an invalid program rather than proceeding.

Suggested change

return constant_buffer[buffer_idx]->storage()->data();

auto* buffer = constant_buffer[buffer_idx];

if (buffer == nullptr) {

ET_LOG(

Error,

"Missing constant_buffer entry for buffer_idx %u",

buffer_idx);

return nullptr;

}

auto* storage = buffer->storage();

if (storage == nullptr) {

ET_LOG(

Error,

"Missing constant_buffer storage for buffer_idx %u",

buffer_idx);

return nullptr;

}

return storage->data();

Copilot · 2026-04-10T18:16:46Z

backends/xnnpack/runtime/XNNCompiler.cpp

+            cd ? cd->size() : 0);
+        return nullptr;
+      }
+      ConstantDataOffsetPtr constant_data_offset = cd->Get(buffer_idx);


cd->Get(buffer_idx) can still return nullptr if the vector contains a null offset (possible with corrupted input). constant_data_offset is dereferenced immediately afterward; add a null check for constant_data_offset and handle it as an invalid constant_data entry (log + propagate error).

Suggested change

ConstantDataOffsetPtr constant_data_offset = cd->Get(buffer_idx);

ConstantDataOffsetPtr constant_data_offset = cd->Get(buffer_idx);

if (constant_data_offset == nullptr) {

ET_LOG(

Error,

"Invalid null constant_data entry at buffer_idx %u",

buffer_idx);

return nullptr;

}

lucylq · 2026-04-10T18:18:25Z

@claude review this PR

claude · 2026-04-10T18:19:05Z

Claude finished @lucylq's task in 1m 28s —— View job

Review of PR #18820

Read the changed files and PR diff
Analyze the changes for correctness, security, and style
Post review feedback

The fix correctly addresses the core issue: the previous code dereferenced flatbuffer_graph->constant_buffer() and flatbuffer_graph->constant_data() without null or bounds checks, which could cause out-of-bounds reads or null pointer dereferences on malformed flatbuffer input. The added validation is appropriate for a security fix.

Two issues remain that should be addressed:

1. Missing null check after constant_buffer[buffer_idx]->storage()->data() (line 197)

After the bounds check, constant_buffer[buffer_idx] could still be null in a malformed flatbuffer (flatbuffers vectors can contain null entries). Dereferencing it or its ->storage() would crash. This should be guarded:

const auto& constant_buffer = *cb;
auto* buffer_entry = constant_buffer[buffer_idx];
if (buffer_entry == nullptr || buffer_entry->storage() == nullptr) {
  ET_LOG(Error, "Null constant_buffer entry at buffer_idx %u", buffer_idx);
  return nullptr;
}
return buffer_entry->storage()->data();

2. Missing null check after cd->Get(buffer_idx) (line 208)

Similarly, cd->Get(buffer_idx) can return null even with a valid index if the flatbuffer is corrupted. The result is immediately dereferenced on line 209 (constant_data_offset->offset()), which would crash:

ConstantDataOffsetPtr constant_data_offset = cd->Get(buffer_idx);
if (constant_data_offset == nullptr) {
  ET_LOG(Error, "Null constant_data entry at buffer_idx %u", buffer_idx);
  return nullptr;
}

3. Caller impact of returning nullptr on error (minor)

For the defineTensor call site (line 318), buffer_ptr being nullptr is the normal case for non-constant tensors (when buffer_idx == 0). When buffer_idx != 0 but we return nullptr due to an error, this silently creates a non-constant tensor instead of propagating the error. The existing callers at lines 469 and 511 do check for nullptr with ET_CHECK_OR_RETURN_ERROR, so those are fine. The defineTensor path is lower risk since it would produce incorrect results rather than a crash, but worth noting for a future follow-up.

Items 1 and 2 are the higher priority since they leave crash paths open that this PR aims to close. Fix these issues →

GregoryComer

Can we update defineTensor to handle the case of a nullptr returned from getConstantDataPtr? It might silently break on malformed PTE.

lucylq · 2026-04-10T22:20:52Z

Can we update defineTensor to handle the case of a nullptr returned from getConstantDataPtr? It might silently break on malformed PTE.

thanks that's a great catch - I'll wrap the return of getConstantDataPtr in Result, so we can differentiate between nullptr and actual error.

Add bounds checking on buffer_idx in both constant_buffer and constant_data code paths of getConstantDataPtr to prevent out-of-bounds vector access from malicious flatbuffer inputs. Authored-with: Claude

Copilot

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 5 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-10T22:32:17Z

backends/xnnpack/runtime/XNNCompiler.cpp

 /**
 Gets the constant data pointer associated with the given tensor value.
 Obtaining the constant data pointer can either be from within the flatbuffer
 payload (deprecated) or via offsets to the constant_data_ptr. If no constant
 data associated with the tensor value, then returns nullptr.
 */
-const uint8_t* getConstantDataPtr(
+Result<const uint8_t*> getConstantDataPtr(


The doc comment is now inaccurate: this function returns Result<const uint8_t*>, not a raw pointer. Please update the comment to mention that failures are returned as an Error, and that the successful value may still be nullptr when the tensor has no constant data.

Copilot · 2026-04-10T22:32:18Z

backends/xnnpack/runtime/XNNCompiler.cpp

+          buffer_idx < cb->size(),
+          InvalidProgram,
+          "buffer_idx %u out of bounds for constant_buffer of size %u",
+          buffer_idx,
+          cb->size());


The format string uses %u for cb->size(), but flatbuffers::Vector::size() is size_t. This is undefined behavior on some platforms; use %zu (or cast to an unsigned type and keep %u) so the log formatting matches the argument type.

Copilot · 2026-04-10T22:32:18Z

backends/xnnpack/runtime/XNNCompiler.cpp

+          buffer_idx < cd->size(),
+          InvalidProgram,
+          "buffer_idx %u out of bounds for constant_data of size %u",
+          buffer_idx,
+          cd->size());


Same issue here: the log uses %u for cd->size() (a size_t). Please switch to %zu or cast appropriately to avoid varargs type mismatches/UB in the error path.

Copilot · 2026-04-10T22:32:18Z

backends/xnnpack/runtime/XNNCompiler.cpp

            weights_cache->load_unpacked_data(data_name);
        if (!data_ptr.ok()) {
          ET_LOG(Error, "Failed to load weights from cache");
-          return nullptr;
+          return Error::InvalidProgram;
        }


On weights-cache failures, this now always returns Error::InvalidProgram, even though load_unpacked_data() already returns a more specific error (e.g., InvalidExternalData). Consider returning data_ptr.error() (and optionally logging it) so callers can distinguish invalid models vs missing/invalid external data.

Copilot · 2026-04-10T22:32:18Z

backends/xnnpack/runtime/XNNCompiler.cpp

              data_name.c_str(),
              static_cast<uint32_t>(buffer.error()));
-          return nullptr;
+          return Error::InvalidProgram;


This branch discards the underlying named_data_map->get_data() error and always returns Error::InvalidProgram. Propagating buffer.error() would preserve the real failure mode (e.g., NotFound/InvalidExternalData) and make error handling/debugging more accurate.

Suggested change

return Error::InvalidProgram;

return buffer.error();

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 Apr 10, 2026

lucylq requested a review from Copilot April 10, 2026 18:12

lucylq marked this pull request as ready for review April 10, 2026 18:12

lucylq requested a review from digantdesai as a code owner April 10, 2026 18:12

Copilot started reviewing on behalf of lucylq April 10, 2026 18:12 View session

Copilot AI reviewed Apr 10, 2026

View reviewed changes

GregoryComer approved these changes Apr 10, 2026

View reviewed changes

lucylq added the security-fix label Apr 10, 2026

lucylq force-pushed the security41 branch from 0cf7016 to ca37d7d Compare April 10, 2026 22:26

Fix TOB-EXECUTORCH-41: validate buffer_idx bounds in getConstantDataPtr

3f9dd63

Add bounds checking on buffer_idx in both constant_buffer and constant_data code paths of getConstantDataPtr to prevent out-of-bounds vector access from malicious flatbuffer inputs. Authored-with: Claude

Copilot AI review requested due to automatic review settings April 10, 2026 22:28

lucylq force-pushed the security41 branch from ca37d7d to 3f9dd63 Compare April 10, 2026 22:28

Copilot started reviewing on behalf of lucylq April 10, 2026 22:29 View session

Copilot AI reviewed Apr 10, 2026

View reviewed changes

-      return constant_buffer[buffer_idx]->storage()->data();
+      auto* buffer = constant_buffer[buffer_idx];
+      if (buffer == nullptr) {
+        ET_LOG(
+            Error,
+            "Missing constant_buffer entry for buffer_idx %u",
+            buffer_idx);
+        return nullptr;
+      }
+      auto* storage = buffer->storage();
+      if (storage == nullptr) {
+        ET_LOG(
+            Error,
+            "Missing constant_buffer storage for buffer_idx %u",
+            buffer_idx);
+        return nullptr;
+      }
+      return storage->data();

Conversation

lucylq commented Apr 10, 2026

Uh oh!

pytorch-bot bot commented Apr 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🔗 Helpful Links

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

❗ 1 Active SEVs

❌ 2 New Failures, 2 Unrelated Failures

Uh oh!

github-actions bot commented Apr 10, 2026

This PR needs a release notes: label

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Apr 10, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 10, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 10, 2026

Choose a reason for hiding this comment

Uh oh!

lucylq commented Apr 10, 2026

Uh oh!

claude bot commented Apr 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review of PR #18820

Uh oh!

GregoryComer left a comment

Choose a reason for hiding this comment

Uh oh!

lucylq commented Apr 10, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Apr 10, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 10, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 10, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 10, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 10, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

pytorch-bot bot commented Apr 10, 2026 •

edited

Loading

This PR needs a `release notes:` label

claude bot commented Apr 10, 2026 •

edited

Loading