From 42ee69914c990698e873ebb04421a29ffe778c54 Mon Sep 17 00:00:00 2001 From: DonislawDev Date: Sun, 6 Sep 2026 14:26:38 +0200 Subject: [PATCH] fix: a recipe is refused for its depth whichever style it nests in The limit that stops a small recipe from exhausting the machine counted flow collections - the ones written with brackets and braces - because that is how the bomb it was written for was written. The same nesting in block style walks past it: "- - - - x" is four nested sequences in eight bytes and carries no bracket at all. Measured against the previous build with tfg validate: "- " x20 000 40 kB 0.40 s refused, by a limit inside the parser "- " x100 000 200 kB 7.41 s refused "- " x250 000 500 kB 88.2 s fatal error: out of memory, exit 2 "- " x500 000 1 MB 70.7 s exit 1, and not one word of output The third printed 41 kB of Go runtime stack and left with the exit code the frozen table gives a mistyped flag. The machine has 31.9 GB. The cost is the nesting rather than the size, and that is measured: 500 kB of flat mapping is refused cleanly in 0.62 s. An alias bomb does not amplify at all, because the schema refuses it before anything expands. Block sequences are now counted from the column each dash sits in, added to the flow depth that was already counted, and the limit stays at 32. An ordinary recipe reaches one and an archive declaring its contents reaches two. The same file is now refused in 0.76 s. The whole defence rests on the pre-scan being cheap, so that was measured rather than assumed - tools/probes/yamldepth: lexer.Tokenize on the bomb is 42 ms and 28.9 MB at 200 kB and 422 ms and 145 MB at 1 MB, against the parser's 70 seconds. Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 16 ++++ internal/cli/errors.go | 8 +- internal/guard/recipebombs_test.go | 52 +++++++++++ internal/recipe/limits.go | 133 ++++++++++++++++++++++------- internal/recipe/recipe.go | 2 +- 5 files changed, 177 insertions(+), 34 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 832c462..fd8a66f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -58,6 +58,22 @@ because it turns other people's test suites red. ### Security +- **A recipe that nests lists deeply is refused before it is read, whichever way + it is written.** The check that already refused deeply nested brackets counted + only brackets, and the same nesting written as `- - - - x` costs two bytes a + level and carries none. + + Measured against the previous build. A 500 kB recipe of nothing but dashes ran + for 88 seconds and ended in `fatal error: out of memory` with a page of Go + internals on standard error, leaving with the exit code a mistyped flag gets. + At 1 MB - the largest recipe the size limit allows - it ran for 70 seconds and + said nothing at all. The same file is now refused in under a second, with a + message naming the depth and the limit. + + The limit counts how deeply lists and mappings nest, in either style, and it + is 32. An ordinary recipe reaches one, and an archive declaring what it + contains reaches two. + - **A file is never written under a name something else already holds.** Every file this tool writes goes to a temporary name first and is renamed into place. Three of those temporary names were created in a way that follows a diff --git a/internal/cli/errors.go b/internal/cli/errors.go index d14c496..71fdd63 100644 --- a/internal/cli/errors.go +++ b/internal/cli/errors.go @@ -146,9 +146,11 @@ func classifyRequest(err error) (int, bool) { if errors.As(err, &recipeTooLarge) { return ExitRecipe, true } - // Same class, one more shape: a recipe whose brackets nest far enough to - // exhaust this machine. A fact about the document rather than a fault of - // ours, so it ends the way a recipe that will not parse does. + // Same class, one more shape: a recipe whose collections nest far enough to + // exhaust this machine - in either style, since 2026-09-06, because block + // lists reach the same depth in two bytes a level and carry no bracket. A + // fact about the document rather than a fault of ours, so it ends the way a + // recipe that will not parse does. var recipeTooDeep *recipe.TooDeepError if errors.As(err, &recipeTooDeep) { return ExitRecipe, true diff --git a/internal/guard/recipebombs_test.go b/internal/guard/recipebombs_test.go index 16981b9..9d0fea2 100644 --- a/internal/guard/recipebombs_test.go +++ b/internal/guard/recipebombs_test.go @@ -86,4 +86,56 @@ func TestAHostileRecipeCannotHangTheReader(t *testing.T) { t.Fatalf("a value holding brackets was refused as if they nested: %v", err) } }) + + t.Run("the same nesting written in block style is refused too", func(t *testing.T) { + // "- - - - x" is four nested sequences in eight bytes and holds not one + // bracket, so the limit that counted brackets walked straight past it. + // Measured on 2026-09-06 against the shipped binary, before this was + // counted: 500 kB of it took 88 seconds and ended in "fatal error: out + // of memory" with 41 kB of Go stack on standard error, and 1 MB - the + // most the size limit allows - left with exit 1 and no output at all. + // After: 756 ms and this refusal. + src := []byte(strings.Repeat("- ", 20000) + "x") + _, err := recipe.Parse(src, "blockbomb.yaml") + var deep *recipe.TooDeepError + if !errors.As(err, &deep) { + t.Fatalf("a block style bomb was answered with %v, expected a refusal about its depth", err) + } + }) + + t.Run("a long list at one level is not nesting", func(t *testing.T) { + // Every dash in the same column is the next entry of one sequence + // rather than a sequence inside the last one. Anything counting dashes + // instead of nesting refuses this at the thirty third target, which is + // an ordinary recipe. + var b strings.Builder + b.WriteString("version: 1\nseed: 7\noutput:\n dir: out\ntargets:\n") + for i := 0; i < 200; i++ { + b.WriteString(" - id: t") + b.WriteString(strconv.Itoa(i)) + b.WriteString("\n format: txt\n size: 100\n") + } + if _, err := recipe.Parse([]byte(b.String()), "long.yaml"); err != nil { + t.Fatalf("a recipe with 200 targets in one list was refused: %v", err) + } + }) + + t.Run("a list inside a list is two, not two hundred", func(t *testing.T) { + // The deepest shape this tool actually produces: targets, and an + // archive declaring what it contains. Two levels, whatever the length + // of either list. + var b strings.Builder + b.WriteString("version: 1\nseed: 7\noutput:\n dir: out\ntargets:\n") + for i := 0; i < 50; i++ { + b.WriteString(" - id: a") + b.WriteString(strconv.Itoa(i)) + b.WriteString("\n format: zip\n contains:\n") + for j := 0; j < 20; j++ { + b.WriteString(" - format: txt\n count: 1\n size: 8kb\n") + } + } + if _, err := recipe.Parse([]byte(b.String()), "nested.yaml"); err != nil { + t.Fatalf("an archive declaring its contents was refused: %v", err) + } + }) } diff --git a/internal/recipe/limits.go b/internal/recipe/limits.go index 65347f2..c417faf 100644 --- a/internal/recipe/limits.go +++ b/internal/recipe/limits.go @@ -25,30 +25,53 @@ import ( // apparent cliff sat exactly there - 1900 pairs produce a 4010 B message and // pass, 1950 produce 4110 B and blocked. The budget was taken back out, since // nothing could have made it fire. - +// +// THE FIRST VERSION OF THIS LIMIT COUNTED THE WRONG THING, and that cost more +// than the bomb it was written for. It counted flow collections - the ones +// written with brackets and braces - because that is how the bomb of +// 2026-09-02 was written. The same nesting written in block style walks past +// it: "- - - - x" is four nested sequences in eight bytes, and the lexer calls +// none of them a bracket. Measured on 2026-09-06 against the shipped binary, +// with tfg validate: +// +// "- " x20 000 40 kB 0.40 s refused, by a limit inside the parser +// "- " x100 000 200 kB 7.41 s refused +// "- " x250 000 500 kB 88.2 s fatal error: out of memory, exit 2 +// "- " x500 000 1 MB 70.7 s exit 1, and not one word of output +// +// The third of those printed 41 kB of Go runtime stack to standard error and +// left with the exit code the frozen table gives a MISTYPED FLAG. The machine +// it did that on has 31.9 GB. +// +// THE COST IS THE NESTING, NOT THE SIZE, and that is measured rather than +// assumed: 500 kB of flat mapping - a hundred thousand lines of "a: 1" - is +// refused cleanly in 0.62 s. So a limit on bytes could never have answered +// this, and neither could one on how many tokens a document holds. const ( - // MaxFlowDepth is how far flow collections - the ones written with - // brackets and braces - may nest. + // MaxNestingDepth is how far collections may nest, in any style. // - // This is the deterministic half, and it is deterministic because the - // lexer has already decided what is a bracket and what is a character - // inside a quoted value. Measured across shapes on 2026-09-02: + // This is the deterministic half, and it is deterministic because the lexer + // has already decided what opens a collection and what is a character + // inside a quoted value. Measured across shapes on 2026-09-02 and again on + // 2026-09-06 with tools/probes/yamldepth: // // block style, a thousand targets 0 // forty thousand brackets inside a string 0 // spread: [1B, 1kb, 1mb] 1 // a thousand targets written in flow style 1 - // the nesting bomb 20000 + // an ordinary recipe 1 + // the flow bomb 20000 + // the block bomb 500000 // - // So one is what real recipes reach and thirty two is far above anything - // a person writes. Counting rather than guessing also rules out the two + // So one is what real recipes reach and thirty two is far above anything a + // person writes. Counting rather than guessing also rules out the two // obvious mistakes: a bracket inside a quoted value is not a collection, // and neither is one inside a comment. - MaxFlowDepth = 32 + MaxNestingDepth = 32 ) -// TooDeepError is returned for a recipe whose flow collections nest past -// MaxFlowDepth. +// TooDeepError is returned for a recipe whose collections nest past +// MaxNestingDepth. type TooDeepError struct { Name string Depth int @@ -56,11 +79,12 @@ type TooDeepError struct { func (e *TooDeepError) Error() string { return fmt.Sprintf( - "%s nests brackets and braces %d deep and the limit is %d. Reading a deeply nested document costs memory that grows far faster than the document does, so a small file can exhaust this machine before anything is written. Write the targets out as an ordinary list instead", - e.Name, e.Depth, MaxFlowDepth) + "%s nests lists and mappings %d deep and the limit is %d. Reading a deeply nested document costs memory that grows far faster than the document does, so a small file can exhaust this machine before anything is written. Write the targets out as an ordinary list instead", + e.Name, e.Depth, MaxNestingDepth) } -// flowDepth is the deepest the flow collections in src nest. +// nestingDepth is the deepest the collections in src nest, counting both +// styles. // // The lexer is asked rather than the bytes, and that is the whole point of // doing it this way: it has already decided which brackets open a collection @@ -69,28 +93,77 @@ func (e *TooDeepError) Error() string { // depth nought, which a scan over the raw bytes could only manage by // reimplementing the quoting rules. // -// Cheap enough to run on every recipe: 0.002 s for a forty kilobyte document, -// 0.008 s for the bomb, and the cost grows with the input rather than with the -// shape - which is exactly what the parser underneath does not do. -func flowDepth(src []byte) int { - current, deepest := 0, 0 +// Cheap enough to run on every recipe, and this is the measurement the whole +// defence rests on, because it has to happen BEFORE the parser sees anything. +// Measured on 2026-09-06 with tools/probes/yamldepth, on the block bomb: +// +// 20 000 levels 40 kB 7 ms 5.5 MB +// 100 000 levels 200 kB 42 ms 28.9 MB +// 500 000 levels 1 MB 422 ms 145.2 MB +// +// The last line is the worst input the size limit allows, and it is what the +// parser answered with 70 seconds and no output at all. The memory is the +// tokeniser's own and it is bounded by MaxBytes, which is the same bargain this +// function has always made - it is not new work, only work that now counts one +// more thing. +func nestingDepth(src []byte) int { + flow, deepest := 0, 0 + + // The columns of the block sequences that are open. A dash further right + // than the innermost open one starts a sequence inside it, a dash at the + // same column is the next entry of that one, and a dash further left closes + // however many it has come back out of. That is the whole of block nesting + // as far as this needs to know. + var dashes []int + for _, t := range lexer.Tokenize(string(src)) { - current += depthChange(t.Type) - if current > deepest { - deepest = current + if t.Type == token.SequenceEntryType && flow == 0 && t.Position != nil { + dashes = openSequences(dashes, t.Position.Column) + } else { + flow += depthChange(t.Type) + } + if d := flow + len(dashes); d > deepest { + deepest = d } } return deepest } -// depthChange is what one token does to the nesting: a collection opening adds -// a level, one closing takes it away, and anything else leaves it where it was. +// openSequences is the block sequences still open after a dash at this column. +// +// A function of its own rather than the body of the loop above, for the reason +// written beside depthChange: together they nest three deep - the loop, the +// branch, the pop - and the shape guard counts how many functions sit that deep +// as well as how deep the deepest one is. +// +// WHAT THIS OVER-COUNTS, said out loud because a limit that hides its edges is +// worse than none. Two sequences that are siblings under different keys, the +// second indented further than the first, are read as nested: +// +// a: +// - x +// b: +// - y counted as depth two, and it is two sequences at depth one +// +// Reaching the limit that way needs thirty two keys each indented further than +// the one before, in one document, which is not a shape anybody writes and is +// not what our own canonical form produces. The alternative is to track where +// mappings open as well, which is more machinery for a case that costs a +// refusal naming the file rather than a wrong answer. +func openSequences(open []int, column int) []int { + for len(open) > 0 && open[len(open)-1] > column { + open = open[:len(open)-1] + } + if len(open) == 0 || open[len(open)-1] < column { + open = append(open, column) + } + return open +} + +// depthChange is what one token does to the flow nesting: a collection opening +// adds a level, one closing takes it away, and anything else leaves it where it +// was. // -// A function of its own rather than a switch inside the loop above, because -// together they nested three deep - the loop, the switch, the comparison - and -// the shape guard counts how many functions sit that deep as well as how deep -// the deepest one is. Splitting is what that guard asks for and it costs -// nothing here. // The default is not decoration. token.Type has thirty four members and a // switch on it without one is reported as incomplete, which is correct of the // linter and wrong about this function: everything that is not a bracket or a diff --git a/internal/recipe/recipe.go b/internal/recipe/recipe.go index f548b9c..cff7179 100644 --- a/internal/recipe/recipe.go +++ b/internal/recipe/recipe.go @@ -188,7 +188,7 @@ func Parse(src []byte, name string) (*Recipe, error) { // cost is and no amount of it can be given back afterwards. See limits.go // for the two shapes this and the budget below answer, and why one number // cannot answer both. - if depth := flowDepth(src); depth > MaxFlowDepth { + if depth := nestingDepth(src); depth > MaxNestingDepth { return nil, &TooDeepError{Name: name, Depth: depth} }