Skip to content

fix: bound expression recursion, so nested source is an error and not a crash - #50

Merged
tannevaled merged 2 commits into
mainfrom
fix-nesting-depth-limit
Oct 5, 2026
Merged

tannevaled merged 2 commits into
mainfrom
fix-nesting-depth-limit

Conversation

@tannevaled

Copy link
Copy Markdown
Contributor

Closes #48.

Deeply nested source overflowed the Go stack, which is a fatal error: rather than a panic: — recover() never sees it, so neither this package's own recover in Parse nor an embedder's could contain it, and the process died. x = + f(×50000, about 150 KB of source, was enough on v0.11.1.

It matters beyond this repository because go-embedded-ruby/ruby calls Parse as the first statement of its public Run, so a host that merely compiles untrusted Ruby was exposed without ever executing it.

Two things the measurement settled that reading would not have

Where to count — no single function sees every production

Instrumenting four candidates at nesting 1000:

production parseExprOrAssign parseTernary parseRange parsePrimary
array / hash / paren / call / interp 1001–1002 1001 1001 1001
block (f { … }) 1002 1001 1001 1
ternary 2 1001 1 1
!×N 2 1 1 1

parsePrimary is blind to blocks; parseExprOrAssign is blind to ternaries. The four productions that actually overflow — array, call, interpolation, block — all route through parseExprOrAssign, and ternary through parseTernary, so both carry the count and share one counter. !×N recurses through neither and never overflowed (clean at 100 000), so it needs nothing.

The counter measures FRAMES, not brackets — and I got this wrong first

parseExprOrAssign and parseTernary nest within each other, so for every bracket-like production the counter runs at exactly 2× the source nesting. Measured at nesting 2000: array 4001; hash/paren/call/interp/block 4003 each; ternary 2003.

A first revision set the limit to 16384 believing it counted brackets. That capped nesting at 8192 and rejected [×9500 — a shape MRI 4.0.5 accepts. My own verification caught it, because I checked the end where it must keep accepting and not only the end where it must refuse.

A limit that is too low is as much a defect as no limit at all, just a quieter one. TestNestingMriAcceptsIsStillAccepted is what would have caught it, and is in this PR for that reason.

The limit: 32768 frames, between two measured bounds

bound nesting counter units
deepest shape MRI accepts ([×N, ruby -c → "Syntax OK"); it raises SystemStackError at 10000, and refuses hash/call/interp/block/ternary by 8000 as a Prism SyntaxError 9 500 19 000
32768 — 32 768
deepest observed safe here (worst productions f(×N and f { ×N are fatal at 50000, safe at 45000; [×N and "#{×N fatal at 55000) 45 000 90 000

1.7× above the first, 2.7× below the second.

It is a frame count rather than a stack-byte budget because Go offers no portable stack-pointer read, and a saved stack address is useless as a baseline: the runtime copies a goroutine's stack when it grows, leaving the value stale by the relocation delta. MRI can budget bytes; we cannot. The honest consequence, recorded in the comment: a production costing far more stack per frame than any probed here would reach the real ceiling first.

Why the decrement is not deferred

p.fail panics, that panic unwinds to Parse — the only recover() in the package — and the Parser is discarded there, so a skipped decrement on the error path cannot be observed, and the hot path pays nothing for a defer.

I checked for an internal recover before relying on that, because speculative parsing would have made the counter leak silently. There is none. TestDepthDoesNotAccumulateAcrossStatements parses 40 000 shallow statements — far more total frames than the limit allows at once — to pin that the ordinary return path does decrement.

Verification

array 9500  -> PARSED-OK          (MRI accepts it, so we must)
array 16000 -> PARSED-OK
array/call/interp/block/ternary at 100000 -> parse error "nesting of 32769 is too deep", exit 0

Before this change those five were fatal error: stack overflow, exit 2.

Full suite green (parser, ast, lexer, token), gofmt -l . empty.

Cost: not resolvable at this sample size, and I will not claim better. Three runs each at -benchtime=3s, against origin/main's parser.go with the same test file, moved in opposite directions — a 16-deep call chain +2.0 %, a 200-method ordinary program −1.6 % — with overlapping ranges in both. Two increments and a compare per expression is not measurable here; that is not the same as being free.

Note for the dependent

go-embedded-ruby/ruby pins v0.11.1, so it does not get this until a tagged release and a go.mod bump. go-embedded-ruby/ruby#768 tracks the VM-side sibling of this defect and references #48; worth saying there which release carries the parser fix.

🤖 Generated with Claude Code

tannevaled and others added 2 commits October 5, 2026 15:38
… a crash

Deeply nested source overflowed the Go stack, which is a FATAL runtime error
rather than a panic: recover() never sees it, so neither this package's own
recover in Parse nor an embedder's could contain it, and the process died.
Measured on v0.11.1, `x = ` + `f(`*50000 -- 150 KB of source -- was enough.

This matters beyond the parser because go-embedded-ruby/ruby calls Parse as the
first statement of its public `Run`, so a host that merely COMPILES untrusted
Ruby was exposed without ever executing it. Filed as #48.

Two things the measurement settled that reading would not have.

FIRST, where to count. No single function sees every production: instrumenting
four candidates at nesting 1000 showed parsePrimary reaching 1 (not 1001) for
`f { ... }`, parseExprOrAssign reaching 2 for a ternary chain, and parseTernary
reaching 1 for `!`*N. The four productions that actually overflow -- array,
call, interpolation and block -- all route through parseExprOrAssign, and
ternary through parseTernary, so both carry the count and share one counter.
`!`*N recurses through neither and never overflowed (clean at 100000).

SECOND, and this is the part I got wrong first: the counter measures FRAMES
ENTERED, not brackets nested, and the two differ by exactly 2x for every
bracket-like production, because the two functions nest within each other.
Measured at nesting 2000: array 4001, hash/paren/call/interp/block 4003 each,
ternary 2003. A first revision set the limit to 16384 believing it counted
brackets; that capped nesting at 8192 and REJECTED `[`*9500 -- a shape MRI
4.0.5 accepts. A limit that is too low is as much a defect as no limit, just a
quieter one, so the suite now pins both ends.

The limit, 32768 frames, sits between two measured bounds:

  - MRI accepts `[`*9500 (`ruby -c`: "Syntax OK") and raises SystemStackError
    at 10000; it refuses hash/call/interp/block/ternary by 8000 already, as a
    Prism SyntaxError. 9500 nesting = 19000 counter units is the deepest any
    probed shape must keep accepting. 32768 is 1.7x above it.
  - this parser's fatal threshold is 50000 nesting for its worst productions
    (`f(`*N and `f { `*N; safe at 45000) and 55000 for `[`*N and `"#{`*N.
    45000 nesting = 90000 units is the deepest observed safe. 32768 is 2.7x
    below it.

It is a frame count and not a stack-byte budget because Go offers no portable
stack-pointer read, and a saved stack address is useless as a baseline: the
runtime COPIES a goroutine's stack when it grows, leaving the value stale by
the relocation delta. MRI can budget bytes; we cannot. The honest consequence,
recorded in the comment, is that a production costing far more stack per frame
than any probed here would reach the real ceiling first.

The decrement is deliberately not deferred. p.fail panics, that panic unwinds
to Parse -- the only recover() in the package -- and the Parser is discarded
there, so a skipped decrement on the error path cannot be observed. I checked
for an internal recover before relying on that, because speculative parsing
would have made the counter leak; there is none.

Cost: not resolvable at this sample size. Three runs each at -benchtime=3s,
against origin/main's parser.go with the same test file, moved in OPPOSITE
directions -- a 16-deep call chain +2.0% and a 200-method ordinary program
-1.6% -- with overlapping ranges in both. Two increments and a compare per
expression is not measurable here; it is not a claim that it is free.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two defects in my own first revision, both found by running what CI runs
instead of what I ran locally.

FIRST, the test timed out all three OS lanes at 600s. CI runs
`go test -race -coverpkg=... -coverprofile=...` with go test's 10-minute
default and no explicit -timeout. A 100000-deep parse took 2.8s locally plain
and 33s locally under -race alone; with coverage instrumentation on a slower
runner it never finished. The depth is now 20000, which is already past the
limit for every bracket production -- deep enough to fire is the requirement,
deeper is only slower. Under CI's exact command the package now takes 63s.

The depths are now PER PRODUCTION, because one number was wrong for one of
them: the limit counts frames, and a ternary chain runs at ratio 1.0 where
bracket productions run at 2.0, so 20000 levels of ternary is 20001 frames --
UNDER the 32768 ceiling. That subtest silently stopped testing anything, and
the suite caught it.

SECOND, and the more useful one: the coverage gate named the check inside
parseExprOrAssign as uncovered. The tempting reading was dead code -- eleven
shapes (array, hash, paren, call, interp, block, ternary, range, not-paren,
array-ternary, splat) all cross the limit inside parseTernary, never there. The
answer to "can this happen?" turned out to be yes: an assignment chain
(`y = `*N) recurses through parseExprOrAssign ALONE, at ratio 1.0, and crosses
the limit there. At n=60000 it reports the limit from that site; at n=300000 it
is 1.2 MB of source that would have had no guard at all.

So the gate was reporting a missing TEST, not dead code, and deleting the
statement would have reopened the hole for a production I had not probed. The
assignment chain is now a row in the table with that reason written next to it,
and the comment in parser.go no longer claims only parseTernary matters.

Verified under CI's exact invocation, including the gate script:
  go test -race -coverpkg=<CI's, /ast excluded> -coverprofile=... ./...  -> ok, 63s
  .github/coverage-gate.sh                                              -> 3472 of 3472 statements covered, exit 0
  gofmt -l .                                                            -> empty

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@tannevaled

Copy link
Copy Markdown
Contributor Author

Red on the first push, for two reasons — both mine, both now fixed

1. The test timed out all three OS lanes at 600s

CI runs go test -race -coverpkg=... -coverprofile=... with go test's 10-minute default and no explicit -timeout. I had measured 2.8s locally with a plain go test and pushed on that. Under -race alone the same test takes 33s locally; with coverage instrumentation on a slower runner it never finished.

The depth is now 20000 rather than 100000 — already past the limit for every bracket production, so a deeper input proves nothing more and costs real time. Under CI's exact command the package now takes 63s.

The depths are now per production, because one number was wrong for one of them: the limit counts frames, and a ternary chain runs at ratio 1.0 where bracket productions run at 2.0. So 20000 levels of ternary is 20001 frames — under the 32768 ceiling, and that subtest silently stopped testing anything. The suite caught it on the next run.

2. The coverage gate named a check I nearly deleted as dead code

The gate reported the failNestingTooDeep() inside parseExprOrAssign as uncovered. The tempting reading was dead code, and the evidence looked strong: eleven shapes — array, hash, paren, call, interp, block, ternary, range, not-paren, array-ternary, splat — all cross the limit inside parseTernary, never there.

The answer to "can this happen?" is yes, by a production none of those eleven covers:

assign-chain n=20000   bytes=80006     parsed OK                              trip=(did not trip)
assign-chain n=60000   bytes=240006    ERR: nesting of 32769 is too deep      trip=parseExprOrAssign
assign-chain n=120000  bytes=480006    ERR: nesting of 32769 is too deep      trip=parseExprOrAssign
assign-chain n=300000  bytes=1200006   ERR: nesting of 32769 is too deep      trip=parseExprOrAssign

An assignment chain (y = *N) recurses through parseExprOrAssign alone, at ratio 1.0. Deleting that statement would have left it with no guard at all — at 300000 it is 1.2 MB of source.

So the gate was reporting a missing test, not dead code. The assignment chain is now a row in the table with that reason beside it, and the comment in parser.go no longer claims only parseTernary matters. Both check sites are load-bearing and that is now measured rather than assumed.

Verified under CI's exact invocation this time, gate included

go test -race -coverpkg=<CI's, /ast excluded> -coverprofile=... ./...   ok, 63s
.github/coverage-gate.sh cover.out                                     3472 of 3472 statements covered, exit 0
gofmt -l .                                                             empty

The PR body's claim that "the four productions that actually overflow all route through parseExprOrAssign, and ternary through parseTernary" was incomplete — assignment chains route through parseExprOrAssign only. That is corrected in the code comment; treat this comment as the amendment to the body.

🤖 Generated with Claude Code

@tannevaled
tannevaled merged commit fc6ea8c into main Oct 5, 2026
11 checks passed
@tannevaled
tannevaled deleted the fix-nesting-depth-limit branch October 5, 2026 15:55
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.

Deeply nested source overflows the Go stack fatally (140 KB is enough): no nesting limit, and recover() cannot contain it

1 participant