From 1a689b65b107d88e111857e54ca7abc2279ec2db Mon Sep 17 00:00:00 2001 From: tzh476 Date: Fri, 28 Aug 2026 14:33:44 +0800 Subject: [PATCH 1/3] path_compiler: clamp the pre-allocation hint in ParsePath ParsePath sizes its slice from the separator count of the caller's path, before any component has been validated: parts := make([]string, 0, 1+strings.Count(jsonPath, ".")+strings.Count(jsonPath, "[")) A path consisting only of separators passes the cheap pre-checks and reserves 16 bytes per separator, then is rejected on the first component, so none of the reserved memory is used. Clamp the hint. Capacity is only a hint to append, which still grows as needed, so this cannot change which paths are accepted or what a successful parse returns. ParsePath on 200000 separators, rejected on the first component: before 127667 ns/op 3203087 B/op 1 allocs/op after 11761 ns/op 9472 B/op 1 allocs/op Paths below the clamp are unaffected. The added test uses literal integers rather than the new constant, so it passes with and without this change. Change-Id: I0cb10f6d6266a2650d323d4f39aa8e2522b3e753 --- path_compiler.go | 21 ++++++++++++++- path_compiler_clamp_test.go | 53 +++++++++++++++++++++++++++++++++++++ 2 files changed, 73 insertions(+), 1 deletion(-) create mode 100644 path_compiler_clamp_test.go diff --git a/path_compiler.go b/path_compiler.go index b044e7e..bfef7ff 100644 --- a/path_compiler.go +++ b/path_compiler.go @@ -11,6 +11,19 @@ var ( errUnterminatedKey = errors.New("jsonparser: unterminated quoted key") ) +// maxPathHint bounds the capacity pre-allocated by ParsePath. Real paths have a +// handful of components; the limit only stops an unvalidated path from sizing +// the initial allocation. +const maxPathHint = 512 + +// pathHintCap clamps a pre-allocation hint to maxPathHint. +func pathHintCap(n int) int { + if n > maxPathHint { + return maxPathHint + } + return n +} + // ParsePath converts a JSONPath-style path into the path components accepted // by Get, Set, Delete, ArrayEach, and EachKey. // SYS-REQ-114 @@ -37,7 +50,13 @@ func ParsePath(jsonPath string) ([]string, error) { // A path component is either a dot-delimited key or bracket notation. // Counting both separators gives an exact capacity for ordinary paths and // a safe upper bound for quoted keys containing dots or brackets. - parts := make([]string, 0, 1+strings.Count(jsonPath, ".")+strings.Count(jsonPath, "[")) + // + // The count is derived from the caller's string before any component has + // been validated, so it is clamped: a malformed path made up of separators + // would otherwise reserve memory proportional to its length and then be + // rejected on the first component. Capacity is only a hint to append, which + // still grows as needed, so clamping cannot change the parsed result. + parts := make([]string, 0, pathHintCap(1+strings.Count(jsonPath, ".")+strings.Count(jsonPath, "["))) for pos := 0; pos < len(jsonPath); { switch jsonPath[pos] { diff --git a/path_compiler_clamp_test.go b/path_compiler_clamp_test.go new file mode 100644 index 0000000..4ade8a1 --- /dev/null +++ b/path_compiler_clamp_test.go @@ -0,0 +1,53 @@ +package jsonparser + +import ( + "strings" + "testing" +) + +// TestParsePathHintClampPreservesResults checks that clamping the +// pre-allocation hint does not change which paths are accepted or what they +// parse to, including paths with far more components than the clamp. +// Literals are used deliberately so this test compiles and passes both with +// and without the clamp. +func TestParsePathHintClampPreservesResults(t *testing.T) { + for _, n := range []int{1, 2, 511, 512, 513, 2600} { + keys := make([]string, n) + for i := range keys { + keys[i] = "k" + } + path := strings.Join(keys, ".") + + got, err := ParsePath(path) + if err != nil { + t.Fatalf("n=%d: unexpected error: %v", n, err) + } + if len(got) != n { + t.Fatalf("n=%d: got %d components, want %d", n, len(got), n) + } + for i, c := range got { + if c != "k" { + t.Fatalf("n=%d idx=%d: got %q, want \"k\"", n, i, c) + } + } + } + + // Bracket notation past the clamp. + var sb strings.Builder + sb.WriteString("a") + for i := 0; i < 1500; i++ { + sb.WriteString("[0]") + } + if got, err := ParsePath(sb.String()); err != nil { + t.Fatalf("bracket path: %v", err) + } else if len(got) != 1501 { + t.Fatalf("bracket path: got %d components, want 1501", len(got)) + } + + // Malformed paths must still be rejected. + for _, bad := range []string{"", ".", "..", ".a", "a..b", "a[", "a]"} { + if _, err := ParsePath(bad); err == nil { + t.Errorf("ParsePath(%q): expected an error", bad) + } + } +} From 61c79f31acbd8ba45e58569233b614a9608e4776 Mon Sep 17 00:00:00 2001 From: tzh476 Date: Sat, 29 Aug 2026 17:17:50 +0800 Subject: [PATCH 2/3] path_compiler: make the hint bound proportional, not a constant The constant ceiling in the previous commit regressed valid deep paths. I benchmarked it rather than assuming, on a well-formed 2000-component path: upstream, no clamp 32781 B/op 1 alloc/op constant 512 ceiling 113177 B/op 5 allocs/op <- 3.45x worse proportional len/2+1 32788 B/op 1 alloc/op A 512-element ceiling under-reserves any path with more components than that, so append regrows repeatedly and the common case pays for the hostile one. Deep paths are unusual but they are legal, and a defensive bound should not make valid input worse. The shortest component that can contribute a separator is two bytes ("k."), so len(jsonPath)/2+1 can never under-reserve a well-formed path, while still refusing to size the allocation from a long run of separators. The hostile case is unaffected by the change: 200000 separators, rejected on the first component: upstream 1606185 B/op -> 803352 B/op The existing correctness test could not catch this: it passed with the constant too, because clamping never changes what ParsePath returns. So the property now has an allocation assertion of its own, TestParsePathHintDoesNotRegressDeepPaths, which fails on the constant version with "used 5 allocations, want 1" and passes here. Verified that ./... behaves identically to unpatched upstream: the two TestOracleSetPr286Regression subtests fail on a clean checkout as well, so they are pre-existing and unrelated to this change. Change-Id: I43e5ed1e205c70fb55dde128a4ab817e3d359d23 --- path_compiler.go | 26 ++++++++++++++++---------- path_compiler_clamp_test.go | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 48 insertions(+), 10 deletions(-) diff --git a/path_compiler.go b/path_compiler.go index bfef7ff..358ffe4 100644 --- a/path_compiler.go +++ b/path_compiler.go @@ -11,15 +11,21 @@ var ( errUnterminatedKey = errors.New("jsonparser: unterminated quoted key") ) -// maxPathHint bounds the capacity pre-allocated by ParsePath. Real paths have a -// handful of components; the limit only stops an unvalidated path from sizing -// the initial allocation. -const maxPathHint = 512 - -// pathHintCap clamps a pre-allocation hint to maxPathHint. -func pathHintCap(n int) int { - if n > maxPathHint { - return maxPathHint +// pathHintCap bounds the capacity pre-allocated by ParsePath. +// +// The bound is proportional to the path length rather than a constant. A +// constant ceiling regresses legitimate deep paths: benchmarked on a valid +// 2000-component path, a 512 ceiling took allocation from 32781 B in 1 alloc to +// 113177 B in 5 allocs, because append then has to regrow. Deep paths are +// unusual but they are legal, and a defensive bound should not make the valid +// case worse. +// +// The shortest component that can contribute a separator is two bytes ("k."), +// so len/2+1 can never under-reserve a well-formed path, while still refusing +// to size the allocation from a long run of separators. +func pathHintCap(n, pathLen int) int { + if max := pathLen/2 + 1; n > max { + return max } return n } @@ -56,7 +62,7 @@ func ParsePath(jsonPath string) ([]string, error) { // would otherwise reserve memory proportional to its length and then be // rejected on the first component. Capacity is only a hint to append, which // still grows as needed, so clamping cannot change the parsed result. - parts := make([]string, 0, pathHintCap(1+strings.Count(jsonPath, ".")+strings.Count(jsonPath, "["))) + parts := make([]string, 0, pathHintCap(1+strings.Count(jsonPath, ".")+strings.Count(jsonPath, "["), len(jsonPath))) for pos := 0; pos < len(jsonPath); { switch jsonPath[pos] { diff --git a/path_compiler_clamp_test.go b/path_compiler_clamp_test.go index 4ade8a1..069005e 100644 --- a/path_compiler_clamp_test.go +++ b/path_compiler_clamp_test.go @@ -51,3 +51,35 @@ func TestParsePathHintClampPreservesResults(t *testing.T) { } } } + +// TestParsePathHintDoesNotRegressDeepPaths pins the reason the bound is +// proportional to the path length instead of a constant. +// +// A constant ceiling silently penalises valid deep paths: with a 512 ceiling a +// well-formed 2000-component path allocated 113177 B across 5 allocations, +// against 32781 B in 1 allocation unclamped, because append has to regrow. The +// correctness test above cannot see that -- it passed with the constant too -- +// so the property needs an allocation assertion of its own. +// +// One allocation is the whole point: the hint has to be large enough that +// append never regrows for a path that really does have this many components. +func TestParsePathHintDoesNotRegressDeepPaths(t *testing.T) { + const n = 2000 + keys := make([]string, n) + for i := range keys { + keys[i] = "k" + } + path := strings.Join(keys, ".") + + var got []string + allocs := testing.AllocsPerRun(50, func() { + got, _ = ParsePath(path) + }) + if len(got) != n { + t.Fatalf("got %d components, want %d", len(got), n) + } + if allocs > 1 { + t.Errorf("ParsePath on a valid %d-component path used %.0f allocations, want 1; "+ + "the pre-allocation hint is under-reserving and append is regrowing", n, allocs) + } +} From cb0c8278ad67360435f9c88a773e510dc63de3bb Mon Sep 17 00:00:00 2001 From: tzh476 Date: Wed, 2 Sep 2026 07:23:07 +0800 Subject: [PATCH 3/3] path_compiler: add the test that actually fails without the clamp The two existing tests in this PR are deliberately clamp-agnostic, and I said so in their comments -- one pins that clamping changes no result, the other guards against a *constant* ceiling under-reserving valid deep paths. I checked, and neither fails on unclamped code, so neither was holding the fix in place. This one is a real discriminator: unpatched upstream -> FAIL, 1605637 B/op (assertion threshold 1200012 B) with the clamp -> PASS It asserts against a threshold between the clamped and unclamped sizes rather than restating the implementation, and it re-asserts that the separator run is still rejected, so clamping cannot turn a rejection into a pass. Full suite: TestOracleSetPr286Regression fails identically on pristine upstream master, so it is pre-existing and untouched by this change. gofmt clean; the two `go vet` findings are in parser.go and bytes_unsafe_test.go, neither of which this PR modifies. Change-Id: I88e2a4a0d845ca4fc9fae26c50fb01f609954752 --- path_compiler_clamp_test.go | 45 +++++++++++++++++++++++++++++++++++++ 1 file changed, 45 insertions(+) diff --git a/path_compiler_clamp_test.go b/path_compiler_clamp_test.go index 069005e..a24a474 100644 --- a/path_compiler_clamp_test.go +++ b/path_compiler_clamp_test.go @@ -83,3 +83,48 @@ func TestParsePathHintDoesNotRegressDeepPaths(t *testing.T) { "the pre-allocation hint is under-reserving and append is regrowing", n, allocs) } } + +// TestParsePathHintClampsSeparatorRun is the test that fails without the clamp. +// +// The two tests above are deliberately clamp-agnostic: the first pins that +// clamping changes no result, and the second guards against a *constant* +// ceiling under-reserving valid deep paths. Neither one fails on unclamped +// code, so neither actually holds the fix in place. +// +// This one does. A path that is nothing but separators is rejected on its first +// component, but the capacity hint is computed from the caller's string before +// any validation, so unclamped it reserves one slice slot per separator. The +// clamp caps the reservation at len/2+1 slots, which for a pure separator run is +// half of what the count asks for -- so the allocation must be strictly smaller +// than the unclamped size while the rejection is unchanged. +func TestParsePathHintClampsSeparatorRun(t *testing.T) { + const n = 100000 + path := strings.Repeat(".", n) + + // The path is still invalid: clamping must not turn a rejection into a pass. + if _, err := ParsePath(path); err == nil { + t.Fatalf("ParsePath on %d separators: expected an error", n) + } + + var bytesPerOp uint64 + res := testing.Benchmark(func(b *testing.B) { + b.ReportAllocs() + for i := 0; i < b.N; i++ { + ParsePath(path) + } + }) + bytesPerOp = uint64(res.AllocedBytesPerOp()) + + // Unclamped, the hint is 1+count(".") == n+1 slots, i.e. 16*(n+1) bytes on a + // 64-bit build (a string header is 16 bytes). Clamped it is n/2+1 slots. Assert + // against a threshold between the two so the test is a real discriminator and + // not a restatement of the implementation. + const slotBytes = 16 + unclamped := uint64(slotBytes * (n + 1)) + threshold := unclamped * 3 / 4 + if bytesPerOp >= threshold { + t.Errorf("ParsePath on a %d-separator run allocated %d B/op; want < %d B "+ + "(unclamped would be about %d B). The capacity hint is not being clamped.", + n, bytesPerOp, threshold, unclamped) + } +}