fix(expr): make the parser iterative to avoid a stack overflow#13333
Open
abendrothj wants to merge 1 commit into
Open
fix(expr): make the parser iterative to avoid a stack overflow#13333abendrothj wants to merge 1 commit into
abendrothj wants to merge 1 commit into
Conversation
The recursive-descent parser had no depth limit, so a deeply nested
expression (e.g. `expr '(' '(' ... 1 ... ')' ')'` with thousands of
parentheses, or a long chain of `length` keywords) aborted with a stack
overflow instead of evaluating. Evaluation was already made iterative in
56c3553; this converts parsing to the same explicit-stack approach
(precedence climbing) and also drops the AST iteratively so destruction
of a deep tree cannot overflow either.
Reported via security advisory GHSA-gr9g-xjfw-pqfw.
Fixes uutils#13146
|
GNU testsuite comparison: |
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.
Fixes #13146 (reported via security advisory GHSA-gr9g-xjfw-pqfw).
Problem
The recursive-descent parser has no depth limit, so a deeply nested expression aborts with a stack overflow instead of evaluating:
Each open parenthesis costs ~7 stack frames (
parse_simple_expressionplus the 6-levelparse_precedenceladder), and nested keyword operands (length length ... 1) recurse the same way. GNU expr evaluates the same input and exits 0.Fix
Evaluation was already converted to an explicit stack in 56c3553 for exactly this reason; this PR does the same for the two remaining recursive spots:
parse_precedenceladder +parse_simple_expressionrecursion is replaced with a single explicit-stack state machine using precedence climbing. Same grammar, same precedence/associativity, same error messages — nesting depth is now bounded only by memory (matching GNU).AstNodegets an iterativeDrop, since destroying a deeply nestedBoxchain would otherwise still overflow.Testing
lengthnestings in-process (test threads have 2 MB stacks, so this is a strict canary).test_deeply_nested_expressionandtest_deeply_nested_lengthfollow the existingtest_long_inputpattern (reduced depth on Windows for its 8191-char command-line limit).1with exit 0 at depth 5000 and 50000.expr.pl-derived suite); compared output/exit codes against another expr implementation on a battery of precedence/associativity/paren cases with no differences.