Bound recursion depth for chained operators, not just nesting - #148
Open
afonsojanu wants to merge 1 commit into
Open
afonsojanu wants to merge 1 commit into
afonsojanu wants to merge 1 commit into
Conversation
base() already refuses to parse expressions nested more than TE_MAX_DEPTH deep, specifically to stop te_eval(), te_free() and the constant-folding pass from overflowing the stack on the resulting tree. That guard only fires when parsing recurses through base() though, which happens for parentheses and function arguments but not for a long run of same-precedence operators. An expression like "1+1+1+...+1" builds a parse tree just as deep as an equivalent chain of nested parens, without ever tripping the existing check, and a few hundred thousand terms is enough to crash the process with a stack overflow in optimize() during te_compile() itself. This adds the same depth check to the iterative operator-chain parsers (term, sum_expr, rel_expr, eq_expr, and_expr, expr, list, and both variants of factor's exponent chain), so these fail cleanly with a parse error instead. Since arguments to a multi-arg function call are evaluated independently rather than nested inside one another, their depth budget is reset between arguments so genuinely independent operands aren't penalized for each other's nesting. Verified the crash with an ASan build (stack-overflow inside optimize() at tinyexpr.c:916) before the fix, and added a smoke.c regression case that builds a long operator chain and checks it's rejected past the depth limit, plus a case confirming two independently-nested function arguments still succeed.
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.
base()already refuses to parse expressions nested more thanTE_MAX_DEPTHdeep. Looking at the comment there, that limit exists specifically to keepte_eval(),te_free()and the constant-folding pass inte_compile()from overflowing the stack, since all three recurse over the parsed tree once per level.The guard only fires when parsing recurses through
base(), though, which covers parentheses and function arguments but not a long run of same-precedence operators."1+1+1+...+1"builds a parse tree exactly as deep as the equivalent chain of nested parens would, without ever going throughbase(), so it slips right past the check. A few hundred thousand terms is enough to crash the process with a stack overflow insideoptimize(), before the caller even gets a chance to callte_eval().Repro before the fix, built with
-fsanitize=address:This PR adds the same depth check to the other combinators that build chains iteratively (
term,sum_expr,rel_expr,eq_expr,and_expr,expr,list, and both variants of the^chain infactor), so they fail with a clean parse error past the limit instead of building an unbounded tree. Since the arguments to a multi-arg function call are evaluated independently of one another rather than nested inside each other, I reset the depth budget between arguments so two legitimately-deep-but-separate operands (e.g.atan2()of two 400-deep nested expressions) don't get penalized for each other's depth.Also added a couple of cases to
test_depth()insmoke.c: one that builds a long operator chain per existing depth bucket and checks it's rejected past 500, and one confirming that two independently nested function arguments still succeed. Ran the full smoke suite (both the default build and theTE_POW_FROM_RIGHT/TE_NAT_LOGvariant) under ASan+UBSan; all 10094 checks pass.