Skip to content

Bound recursion depth for chained operators, not just nesting - #148

Open
afonsojanu wants to merge 1 commit into
codeplea:masterfrom
afonsojanu:fix/chain-depth-stack-overflow
Open

afonsojanu wants to merge 1 commit into
codeplea:masterfrom
afonsojanu:fix/chain-depth-stack-overflow

Conversation

@afonsojanu

Copy link
Copy Markdown

base() already refuses to parse expressions nested more than TE_MAX_DEPTH deep. Looking at the comment there, that limit exists specifically to keep te_eval(), te_free() and the constant-folding pass in te_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 through base(), so it slips right past the check. A few hundred thousand terms is enough to crash the process with a stack overflow inside optimize(), before the caller even gets a chance to call te_eval().

Repro before the fix, built with -fsanitize=address:

$ ./repro 200000
compiled: (nil) ...
==...==ERROR: AddressSanitizer: stack-overflow ... in optimize tinyexpr.c:916

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 in factor), 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() in smoke.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 the TE_POW_FROM_RIGHT/TE_NAT_LOG variant) under ASan+UBSan; all 10094 checks pass.

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant