Repository navigation
Set recursion limit for QPY deserialization - #17415
Open
Matthew Treinish (mtreinish) wants to merge 8 commits into
Open
Matthew Treinish (mtreinish) wants to merge 8 commits into
Matthew Treinish (mtreinish) wants to merge 8 commits into
Conversation
Several parts of QPY deserialization are currently implemented recursively, primarily deserialization of control flow instruction bodies, Expr, and parameter expression substitutions. This could cause a segfault from a stack overflow if the recursion is too deep. To address this as a temporary solution this sets a recursion limit that returns a catchable error in the case the recursion is too deep. Longer term we can look at ways to either rewrite the deserialization to avoid recursion or come up with other mechanisms like using stacker, but these require more care or have larger issues associated with them then what we can fix in a security patch release. An additional place where there is recursion triggered through QPY deserialization, but is more general than just QPY, occurs during building out ParameterExpressions. This is fixed in this commit because QPY can trigger it and it would have the same issue being addressed by the rest of the commit. Unlike the recursion limit set in the qpy crate, the stacker library is used to address this for the parameter expression. Using stacker there applied cleanly and enables the stack to spill onto the heap when needed avoiding a hard limit. Co-authored-by: Matthew Treinish <mtreinish@kortar.org> Co-authored-by: Jake Lishman <jake.lishman@ibm.com> Co-authored-by: David F. Cohen <me@davidfcohen.com>
Matthew Treinish (mtreinish)
added
the
Changelog: Fixed
Collaborator
|
One or more of the following people are relevant to this code:
|
David F. Cohen (davidfcohen)
previously approved these changes
Nov 4, 2026
Turns out that we call` iter_symbols` indirectly to produce `Err` variants in `try_into_value` in non-exceptional code and we then immediately discard them... Leaving that aside, the previous implementation of `iter_symbols` required a (not lazy!) recursively boxed structure on the heap, which is problematic for `Drop` and for the iterator-combinators that end up recursive too. This logic is order-identical (left-first DFS), but turns the iteration logic into a flat structure with an iterative algorithm. We only push to the stack when we encounter `Binary` nodes; everything else is traversed with no per-level memory overhead.
In debug builds the stack frame size is larger and can trigger stack overflows while building the qpy. That's not part of this fix and to avoid the testing failure this sets a small depth that won't trigger a stack overflow and then using unittest mock to set the recursion limit even lower.
David F. Cohen (davidfcohen)
previously approved these changes
Nov 4, 2026
David F. Cohen (davidfcohen)
left a comment
David F. Cohen (davidfcohen)
left a comment
Contributor
There was a problem hiding this comment.
The latest commits LGTM.
Jake Lishman (jakelishman)
previously approved these changes
Nov 4, 2026
Comment on lines
+3712
to
+3713
| BinaryOp::Mul => match crate::stack::grow(|| self.mul_opt(l, recursive)) { | ||
| Some(e) => match crate::stack::grow(|| e.mul_opt(r, recursive)) { |
There was a problem hiding this comment.
I guess we could have used the same mul_opt_inner form we did in other cases, but not big deal.
Jake Lishman (jakelishman)
removed this pull request from the merge queue due to a manual request
Nov 4, 2026
… deserialization limit
Matthew Treinish (mtreinish)
dismissed stale reviews from Jake Lishman (jakelishman) and David F. Cohen (davidfcohen)
via
November 4, 2026 22:40
7b016b1
This branch has not been deployed
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.
Several parts of QPY deserialization are currently implemented recursively, primarily deserialization of control flow instruction bodies, Expr, and parameter expression substitutions. This could cause a segfault from a stack overflow if the recursion is too deep. To address this as a temporary solution this sets a recursion limit that returns a catchable error in the case the recursion is too deep. Longer term we can look at ways to either rewrite the deserialization to avoid recursion or come up with other mechanisms like using stacker, but these require more care or have larger issues associated with them then what we can fix in a security patch release.
An additional place where there is recursion triggered through QPY deserialization, but is more general than just QPY, occurs during building out ParameterExpressions. This is fixed in this commit because QPY can trigger it and it would have the same issue being addressed by the rest of the commit. Unlike the recursion limit set in the qpy crate, the stacker library is used to address this for the parameter expression. Using stacker there applied cleanly and enables the stack to spill onto the heap when needed avoiding a hard limit.
AI/LLM disclosure