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

⏳ 3 Pending, 2 Unrelated Failures

As of commit 4e15ca5 with merge base 5e8a0df ():

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:46Z

+            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.

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:18Z

              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();

lucylq · 2026-04-14T20:43:50Z

@claude re-review?

claude · 2026-04-14T20:44:39Z

Claude finished @lucylq's task in 59s —— View job

Re-review of PR #18820

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

This is a solid revision that addresses all the issues from the first review. The key improvements since the last review:

Result<const uint8_t*> return type -- Properly differentiates between "no constant data" (nullptr success) and "invalid/corrupt flatbuffer" (Error). This was the main gap identified previously.
Null checks on constant_buffer[buffer_idx] and its ->storage() (line 199-203) -- Addresses the crash path on malformed flatbuffers where vector entries could be null.
Null check on cd->Get(buffer_idx) (line 216-220) -- Addresses the null constant_data_offset crash path.
Error propagation instead of silent nullptr (lines 234, 246) -- Both the weights-cache and named-data-map error paths now return data_ptr.error() / buffer.error() instead of nullptr, preserving the specific error code for callers.
All call sites updated -- defineTensor (line 329-339), and the two scale_buffer_idx call sites (lines 484-494, 530-541) all properly unwrap the Result and propagate errors.

One minor observation, not blocking:

Line 228: constant_data_offset->named_key() could theoretically be null in a malformed flatbuffer even when IsFieldPresent returns true (the field is present but the offset points to null). Dereferencing with ->str() would crash. This is pre-existing and not introduced by this PR, so it's fine to address separately if at all -- the risk is low since IsFieldPresent guards it.

Overall the PR is clean and correct. The Result-based approach is the right design, and the validation is thorough across both the constant_buffer and constant_data paths. LGTM.

View job

Copilot

Pull request overview

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

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

Copilot · 2026-04-14T20:46:52Z

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


Same issue here: the error message uses %zu for cd->size(), but FlatBuffers vector sizes are typically uoffset_t (uint32_t). Please cast to size_t when using %zu (or use a %u-style specifier with an appropriate cast) to avoid varargs type mismatch/UB.

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

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

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

lucylq force-pushed the security41 branch from 3f9dd63 to e85653b Compare April 14, 2026 20:40

Copilot AI review requested due to automatic review settings April 14, 2026 20:43

lucylq force-pushed the security41 branch from e85653b to 91e372b Compare April 14, 2026 20:43

Copilot started reviewing on behalf of lucylq April 14, 2026 20:44 View session

Copilot AI reviewed Apr 14, 2026

View reviewed changes

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

4e15ca5

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

lucylq force-pushed the security41 branch from 91e372b to 4e15ca5 Compare April 14, 2026 20:52

lucylq merged commit d5b0816 into main Apr 14, 2026
165 of 167 checks passed

lucylq deleted the security41 branch April 14, 2026 23:18

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

⏳ 3 Pending, 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!

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI Apr 10, 2026

Choose a reason for hiding this comment

Uh oh!

lucylq commented Apr 14, 2026

Uh oh!

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

Uh oh!

Re-review of PR #18820

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Apr 14, 2026

Choose a reason for hiding this comment

Uh oh!

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

claude bot commented Apr 14, 2026 •

edited

Loading