Skip to content

Fix stack overflow in desugarer via depth limit#1318

Open
sharadboni wants to merge 1 commit intogoogle:masterfrom
sharadboni:fix-desugarer-stack-overflow
Open

Fix stack overflow in desugarer via depth limit#1318
sharadboni wants to merge 1 commit intogoogle:masterfrom
sharadboni:fix-desugarer-stack-overflow

Conversation

@sharadboni
Copy link
Copy Markdown

Summary

The parser's parseInfix function builds chains of Binary nodes using a while loop without incrementing the parser depth counter. This produces arbitrarily deep AST trees. The desugarer at core/desugarer.cpp:685 then recursively walks this tree via desugar(), causing a stack overflow crash on crafted inputs with deeply nested binary expressions.

Similarly affected recursive walkers include core/static_analysis.cpp and several vm.cpp functions (countLeaves, findObject, objectFieldsAux, otherJsonToHeap), but the desugarer runs first and now rejects these inputs.

Root Cause

parseInfix uses iteration (a while loop) to parse operator chains, so the parser's own depth limit never triggers. The resulting AST can be thousands of levels deep, but subsequent passes walk it with unbounded recursion.

Fix

  • Add a depth counter parameter to the desugar() function (and its helper methods desugarParams, desugarFields, makeObject, makeObjectComprehension)
  • Increment depth on every recursive call
  • Throw a StaticError("Maximum expression nesting depth exceeded during desugaring.") when depth exceeds 500
  • Entry points (desugarFile, stdlibAST) start at depth 0 via the default parameter

The limit of 500 is generous enough for any realistic Jsonnet program while preventing stack exhaustion.

Reproduction

// A long chain of binary additions: 1+1+1+1+...+1 (100,000 terms)
// This parses successfully but crashes the desugarer with a stack overflow.

Test plan

  • Verify normal Jsonnet programs still compile and run (depth 500 is far beyond typical nesting)
  • Verify the PoC input now produces a clean error message instead of crashing
  • Existing test suite passes

The parser's parseInfix function builds chains of Binary nodes using a
while loop without incrementing the parser depth counter. This produces
arbitrarily deep AST trees that the desugarer then walks recursively,
causing a stack overflow crash on deeply nested expressions.

Add a depth counter to the desugar() function that increments on each
recursive call and throws a StaticError when the depth exceeds 500.
This prevents the stack overflow while still allowing reasonable
nesting depths for normal programs.

The same deeply nested AST also affects recursive walkers in
static_analysis.cpp, vm.cpp (countLeaves, findObject,
objectFieldsAux, otherJsonToHeap), but the desugarer runs first and
now rejects these inputs before they reach those code paths.
@sharadboni
Copy link
Copy Markdown
Author

@johnbartholomew Could you review this security fix? It adds a depth limit to the desugarer to prevent stack overflow from deeply nested binary expression trees that bypass the parser's depth counter.

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