Qualcomm: bounds-check the delegate - #22237
Conversation
…off the end
execute() binds delegate arguments positionally. It walks the input and output
tensor lists recovered from the context binary and, for every tensor the name
prefixes mark as bindable, consumes one entry from args with a running counter.
Nothing relates that counter to args.size().
So when the binary and the program disagree on the delegate signature -- a stale
binary, or an AOT bug that publishes extra graph I/O -- the walk indexes past the
end of the Span and dereferences whatever is there. In the case that prompted
this, a context binary declaring 54 graph inputs and 56 graph outputs met a
program passing 4 tensors, and the result was a null dereference at 0x8 with the
two counts sitting in registers. Reading that back to a cause took days.
Count the bindable tensors with the same prefix rules the loops use, then check
once before either loop runs. A shortfall is the memory-safety case and is
fatal; a surplus is not unsafe, so it warns rather than failing, since a
trailing unused argument is not obviously wrong.
Deliberately not included: a matching "input_" prefix filter on the input loop,
for symmetry with the output loop. Inputs of a model built by from_context_binary
carry names straight from the QNN converter with no such prefix, and the runtime
only renames outputs (QnnManager.cpp SetName("output_" + tensor_name)). Filtering
on it would skip every input of those models and leave the counter at zero when
the output loop starts, writing outputs into input buffers. The count check gives
the same protection without that risk.
Authored with assistance from Claude Code.
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/22237
Note: Links to docs will display an error until the docs builds have been completed. ❌ 3 New FailuresAs of commit 25d20f1 with merge base fe5d8d6 ( NEW FAILURES - The following jobs have failed:
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
meta-cla
Bot
added
the
CLA Signed
This PR needs a
|
psiddh
added
the
module: qnn
|
" In the case that prompted this, a context binary declaring 54 graph inputs and 56 graph outputs met a program passing 4 tensors, and the result was a null dereference at 0x8 " Can we add a testcase to ensure this scenario is not broken again |
winskuo-quic
left a comment
winskuo-quic
left a comment
There was a problem hiding this comment.
Thanks for the PR.
Please have a look at the comments.
Review follow-up. A surplus is not a memory-safety problem, which is why it warned, but if the check exists to keep QNN graph I/O and the delegate signature aligned then both directions are misalignment. Verified against 14 lowerings -- single I/O, multi-input, multi-output, partially-consumed multi-output, topk with both outputs used, a mutable buffer and a conv, each under an fp16 and a quantized spec -- with no false positives. Mutable buffers are excluded from the count by the same rule the binding loops use, so they do not create a surplus.
There was a problem hiding this comment.
🟡 Changes recommended
The newly added argument-count check currently makes surplus args fatal, which contradicts the PR’s stated behavior (surplus should warn) and is stricter than required for the memory-safety fix.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Lite
Good point. Can you point me to right C++ test target for this particular test ? Can't seem to find the right one... Worth noting the scenario isn't reachable from AOT any more, #22011 removed the bug that produced it, so a stale binary is the only remaining path. |
psiddh
changed the title
There was a problem hiding this comment.
🟢 Approval recommended
The added preflight check closes a clear memory-safety hole with minimal, localized impact (only a minor error-message wording nit remains).
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Lite
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
There are a couple of small but concrete correctness/robustness improvements (notably args_index type safety) that should be addressed alongside this memory-safety fix.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 2
- Review effort level: Lite
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
|
@winskuo-quic Can you take a look at it again ? Thanks |
There was a problem hiding this comment.
🟢 Approval recommended
The change is a narrowly scoped, internally consistent bounds check that prevents a confirmed OOB dereference without altering the binding rules used by the existing loops.
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 0 new
- Review effort level: Lite
Qualcomm: bounds-check the delegate argument walk instead of running off the end
execute() binds delegate arguments positionally. It walks the input and output
tensor lists recovered from the context binary and, for every tensor the name
prefixes mark as bindable, consumes one entry from args with a running counter.
Nothing relates that counter to args.size().
So when the binary and the program disagree on the delegate signature -- a stale
binary, or an AOT bug that publishes extra graph I/O -- the walk indexes past the
end of the Span and dereferences whatever is there. In the case that prompted
this, a context binary declaring 54 graph inputs and 56 graph outputs met a
program passing 4 tensors, and the result was a null dereference at 0x8 with the
two counts sitting in registers. Reading that back to a cause took days.
Count the bindable tensors with the same prefix rules the loops use, then check
once before either loop runs. A mismatch in either direction is fatal. A
shortfall is the memory-safety case, since the walk reads past the end of args;
a surplus does not read out of bounds, but it still means the binary and the
program disagree on the signature, which is a defect either way. This started
out warning on a surplus and was changed to fail at review request.
Verified against 14 lowerings -- single I/O, multi-input, multi-output,
partially-consumed multi-output, topk with both outputs used, a mutable buffer
and a conv, each under an fp16 and a quantized spec -- with no false positives.
Mutable buffers are excluded from the count by the same rule the binding loops
use, so they do not create a surplus.
Deliberately not included: a matching "input_" prefix filter on the input loop,
for symmetry with the output loop. Inputs of a model built by from_context_binary
carry names straight from the QNN converter with no such prefix, and the runtime
only renames outputs (QnnManager.cpp SetName("output_" + tensor_name)). Filtering
on it would skip every input of those models and leave the counter at zero when
the output loop starts, writing outputs into input buffers. The count check gives
the same protection without that risk.
cc @cbilgin