Qualcomm: bounds-check the delegate argument walk instead of running … - #22237
Open
psiddh wants to merge 1 commit into
Open
Qualcomm: bounds-check the delegate argument walk instead of running …#22237psiddh wants to merge 1 commit into
psiddh wants to merge 1 commit into
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. 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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
…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.
cc @cbilgin