diff --git a/nesting_depth_test.go b/nesting_depth_test.go new file mode 100644 index 0000000..8abcab8 --- /dev/null +++ b/nesting_depth_test.go @@ -0,0 +1,159 @@ +// Copyright (c) the go-ruby-parser/parser authors +// +// SPDX-License-Identifier: BSD-3-Clause + +package parser_test + +import ( + "strings" + "testing" + + "github.com/go-ruby-parser/parser" +) + +// nested builds a source with n levels of one syntactic production. Each entry +// is a shape whose recursion was measured to reach the Go stack: array, call, +// interpolation and block all overflowed the stack fatally before the limit +// existed, at 50000-55000 levels. +func nested(kind string, n int) string { + switch kind { + case "array": + return "x = " + strings.Repeat("[", n) + strings.Repeat("]", n) + "\n" + case "hash": + return "x = " + strings.Repeat("{a: ", n) + "1" + strings.Repeat("}", n) + "\n" + case "paren": + return "x = " + strings.Repeat("(", n) + "1" + strings.Repeat(")", n) + "\n" + case "call": + return "x = " + strings.Repeat("f(", n) + "1" + strings.Repeat(")", n) + "\n" + case "interp": + return "x = " + strings.Repeat(`"#{`, n) + "1" + strings.Repeat(`}"`, n) + "\n" + case "block": + return "x = " + strings.Repeat("f { ", n) + "1" + strings.Repeat(" }", n) + "\n" + case "ternary": + return "x = " + strings.Repeat("1 ? ", n) + "1" + strings.Repeat(" : 2", n) + "\n" + case "assign": + return "x = " + strings.Repeat("y = ", n) + "1\n" + } + t := "unknown production " + kind + panic(t) +} + +// deepKinds pairs each production with a depth comfortably past the limit FOR +// THAT PRODUCTION. The depths differ because the limit counts frames entered, +// not brackets nested, and the ratio is not the same everywhere: every +// bracket-like production enters parseExprOrAssign and parseTernary in turn and +// so runs at 2x the source nesting, while a ternary chain re-enters only +// parseTernary and runs at 1x. Measured at nesting 2000: array 4001, +// hash/paren/call/interp/block 4003, ternary 2003. +// +// A single depth of 20000 for all of them looked right and was not: it is 40000 +// frames for the first six and only 20001 for the ternary, which sits UNDER the +// 32768 ceiling, so the ternary case silently stopped testing anything. This +// test caught that, which is the argument for keeping the depths explicit +// rather than computing one from a constant. +var deepKinds = []struct { + kind string + depth int +}{ + {"array", 20_000}, + {"hash", 20_000}, + {"paren", 20_000}, + {"call", 20_000}, + {"interp", 20_000}, + {"block", 20_000}, + {"ternary", 35_000}, + // An assignment chain is the one shape that recurses through + // parseExprOrAssign ALONE -- the other eleven probed shapes all cross the + // limit inside parseTernary. Without this row the check in + // parseExprOrAssign is never exercised, the coverage gate says so, and + // deleting it as dead code would leave `y = `*N unguarded. Ratio 1.0, so + // it needs a depth past the frame limit like the ternary does. + {"assign", 40_000}, +} + +// Source deep enough to have overflowed the Go stack must now come back as an +// ordinary error. A Go stack overflow is a FATAL runtime error rather than a +// panic, so this test cannot assert it with recover(): before the limit existed +// the process died and the test binary went with it. That is why the assertion +// is "an error, and we are still running". +// The depth is 20000 levels and not 100000. 20000 is already past the limit for +// every production here -- the ceiling is 32768 FRAMES, which is 16384 levels of +// any bracket production and 32768 of a ternary chain -- so a deeper input +// proves nothing more and costs real time: CI runs this with -race AND coverage +// instrumentation under go test's 10-minute default, and a first revision at +// 100000 took 2.8s locally, 33s locally under -race alone, and timed out the +// whole package on all three OS lanes at 600s. Deep enough to fire is the +// requirement; deeper is only slower. +func TestDeepNestingIsAnErrorNotAStackOverflow(t *testing.T) { + for _, tc := range deepKinds { + t.Run(tc.kind, func(t *testing.T) { + _, err := parser.Parse(nested(tc.kind, tc.depth)) + if err == nil { + t.Fatalf("%s nested %d deep parsed without error; the depth limit did not fire", + tc.kind, tc.depth) + } + if !strings.Contains(err.Error(), "too deep") { + t.Fatalf("%s: error is %q, want one naming the nesting limit", tc.kind, err) + } + }) + } + t.Log("reached the end, so no production took the process down") +} + +// The other end, and the reason it is here: the limit counts FRAMES ENTERED, +// which for every bracket-like production is twice the source nesting, because +// parseExprOrAssign and parseTernary nest within each other and share the +// counter. A first revision of this limit set it to 16384 believing it counted +// brackets -- which capped nesting at 8192 and rejected 9500 levels of array +// literal, a shape MRI 4.0.5 ACCEPTS (`ruby -c` reports "Syntax OK"). +// +// So a limit that is too low is as much a defect as no limit at all, just a +// quieter one, and this test is what would have caught it. +func TestNestingMriAcceptsIsStillAccepted(t *testing.T) { + for _, n := range []int{1_000, 9_500, 16_000} { + if _, err := parser.Parse(nested("array", n)); err != nil { + t.Errorf("array nested %d deep: %v; MRI accepts this depth, so we must too", n, err) + } + } +} + +// Ordinary source must not pay for the limit in behaviour: a program nested a +// normal amount parses, and the counter does not accumulate across the many +// expressions of one parse -- which it would if the decrement were skipped on +// any ordinary return path. 10000 statements enter parseExprOrAssign tens of +// thousands of times in total, far past the 32768 ceiling, so a leaked +// decrement fails this; it is kept at 10000 rather than higher because this +// runs under -race and coverage in CI. +func TestDepthDoesNotAccumulateAcrossStatements(t *testing.T) { + var b strings.Builder + for i := 0; i < 10_000; i++ { + b.WriteString("x = f(g([1, {a: 2}]))\n") + } + if _, err := parser.Parse(b.String()); err != nil { + t.Fatalf("10000 shallow statements: %v; the depth counter is leaking per statement", err) + } +} + +func BenchmarkParseNestedShallow(b *testing.B) { + src := nested("call", 16) + b.ResetTimer() + for i := 0; i < b.N; i++ { + if _, err := parser.Parse(src); err != nil { + b.Fatal(err) + } + } +} + +func BenchmarkParseOrdinaryProgram(b *testing.B) { + var sb strings.Builder + for i := 0; i < 200; i++ { + sb.WriteString("def m(a, b = 1, *r, k: 2, **kw, &blk)\n x = f(g([a, {k => b}])) ? a : b\n x.map { |v| v + 1 }\nend\n") + } + src := sb.String() + b.ResetTimer() + for i := 0; i < b.N; i++ { + if _, err := parser.Parse(src); err != nil { + b.Fatal(err) + } + } +} diff --git a/parser.go b/parser.go index 0e9eaf5..065c977 100644 --- a/parser.go +++ b/parser.go @@ -225,6 +225,16 @@ type Parser struct { // `top_compstmt` (3029), so a BEGIN inside a BEGIN is at top level again, which // parseExeBlock spells by lending its body the enclosing depth. stmtDepth int + + // exprDepth is the recursive-descent depth of the EXPRESSION grammar, and it + // exists to bound the Go stack rather than to disambiguate anything -- unlike + // patternDepth, bracketDepth and stmtDepth above. Without it, nested source + // overflows the goroutine stack, which is a FATAL Go error and not a panic: + // `recover()` never sees it, so neither this package's own recover in Parse + // nor an embedder's can contain it, and the process dies. Measured on + // v0.11.1: `x = ` + `f(`*50000 dies with `fatal error: stack overflow`, + // where MRI refuses the same shape at 10000 with a catchable SystemStackError. + exprDepth int // preExe collects the statements of every `BEGIN { }` block. MRI accumulates // them in `p->eval_tree_begin` and runs them before the program body; Parse // prepends them for the same reason, in source order. @@ -321,6 +331,55 @@ func (p *Parser) expect(tt token.Type) token.Token { return p.advance() } +// maxNestingDepth is the ceiling on expression recursion. +// +// IT COUNTS FRAMES ENTERED, NOT BRACKETS NESTED, and the two are not the same +// number. parseExprOrAssign and parseTernary nest within each other, and they +// share this counter, so for every bracket-like production the counter runs at +// exactly 2x the source nesting -- measured at nesting 2000: array 4001, and +// hash/paren/call/interp/block 4003 each, i.e. 2.00. A ternary chain runs at +// 1.00 (2003), because it re-enters only parseTernary. Getting this wrong is +// not academic: an earlier revision set the limit to 16384 believing it was a +// nesting count, which capped nesting at 8192 and rejected `[`*9500 -- a shape +// MRI ACCEPTS. The test suite pins both ends for that reason. +// +// The bounds, both measured on v0.11.1, in nesting and in counter units: +// +// - MRI 4.0.5 accepts `[`*9500 (`ruby -c` says "Syntax OK") and raises +// SystemStackError at 10000; it refuses hash/call/interp/block/ternary by +// 8000 already, as a Prism SyntaxError. So 9500 nesting = 19000 counter +// units is the highest any probed shape must keep accepting. +// - this parser's own FATAL threshold is 50000 nesting for its worst +// productions (`f(`*N and `f { `*N; safe at 45000) and 55000 for `[`*N and +// `"#{`*N. So 45000 nesting = 90000 counter units is the deepest observed +// to be safe. +// +// 32768 sits 1.7x above the first and 2.7x below the second. +// +// The limit 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 +// saved value stale by the relocation delta. MRI can use bytes; we cannot. The +// honest consequence is that a production costing far more Go stack per frame +// than any probed here would reach the real ceiling first. +// +// BOTH check sites are load-bearing, and that was measured rather than assumed. +// Eleven bracket-like and ternary shapes all cross the limit inside +// parseTernary -- but an assignment chain (`y = `*N) recurses through +// parseExprOrAssign ALONE, at ratio 1.0, and crosses it there. Deleting the +// check in parseExprOrAssign as dead code would therefore have left `y = `*N +// unguarded; at 300000 it is 1.2 MB of source. The coverage gate named that +// statement as uncovered, which was a missing TEST and not dead code -- the +// distinction is worth the two minutes it takes to settle. +const maxNestingDepth = 32768 + +// failNestingTooDeep reports the limit as an ordinary parse error, because that +// is what callers can handle: Parse recovers parseError into its error return, +// so an embedder gets an error instead of losing the process. +func (p *Parser) failNestingTooDeep() { + p.fail("nesting of %d is too deep", p.exprDepth) +} + // fail never returns; the ast.Node result lets primary parsers write // `return p.fail(...)` without an unreachable trailing return. func (p *Parser) fail(format string, args ...any) ast.Node { @@ -2548,7 +2607,21 @@ func isPostfixStart(tt token.Type) bool { return false } +// parseExprOrAssign counts its own recursion before descending. The decrement is +// deliberately NOT deferred: p.fail panics, that panic unwinds all the way to +// Parse, 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. func (p *Parser) parseExprOrAssign() ast.Node { + p.exprDepth++ + if p.exprDepth > maxNestingDepth { + p.failNestingTooDeep() + } + n := p.parseExprOrAssignBody() + p.exprDepth-- + return n +} + +func (p *Parser) parseExprOrAssignBody() ast.Node { // A `not EXPR` prefix in value position (assignment RHS, head of a call's // argument list): `x = not true`, `foo(not flag)`. `not` binds looser than the // binary operators, so it wraps the whole following expression. @@ -2810,7 +2883,21 @@ func (p *Parser) withRescueModifier(node ast.Node) ast.Node { // parseTernary handles `cond ? then : else`, binding looser than ranges/binary // operators but tighter than assignment, and right-associative. It desugars to // an If expression. +// parseTernary counts its own recursion before descending. The decrement is +// deliberately NOT deferred: p.fail panics, that panic unwinds all the way to +// Parse, 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. func (p *Parser) parseTernary() ast.Node { + p.exprDepth++ + if p.exprDepth > maxNestingDepth { + p.failNestingTooDeep() + } + n := p.parseTernaryBody() + p.exprDepth-- + return n +} + +func (p *Parser) parseTernaryBody() ast.Node { cond := p.parseRange() if !p.accept(token.QUESTION) { return cond