path_compiler: clamp the pre-allocation hint in ParsePath

[tzh476]

Problem

ParsePath sizes its slice from the separator count of the caller's string, before any component has been validated:

parts := make([]string, 0, 1+strings.Count(jsonPath, ".")+strings.Count(jsonPath, "["))

for pos := 0; pos < len(jsonPath); {
	switch jsonPath[pos] {
	case '.':
		return nil, errMalformedPath

A path consisting only of separators passes the two cheap pre-checks and reserves 16 bytes per separator — string is a two-word header — and is then rejected on the first component, so none of the reserved memory is used.

Change

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.

Measurements

ParsePath on 200000 separators, rejected on the first component (same clone, same machine, -benchtime 300x, Apple M3 Pro, go1.22.5):

before   127667 ns/op   3203087 B/op   1 allocs/op
after     11761 ns/op      9472 B/op   1 allocs/op

(Two runs on different clones gave 144736/8219 ns and 127667/11761 ns; the byte
figures were stable at 3203085-3203087 and 9472.)

Paths below the clamp are unaffected.

Tests

TestParsePathHintClampPreservesResults asserts identical results for 1, 2, 511, 512, 513 and 2600 dot-separated components — well past the clamp, where append must grow — plus a 1500-element bracket-notation path, and that malformed paths ("", ".", "..", ".a", "a..b", "a[", "a]") are still rejected. It uses literal integers rather than the new constant, so it compiles and passes on an unpatched tree as well; I checked that, so it is testing behaviour rather than the patch.

gofmt is clean. The package has the same 728 passing tests before and after the change. Two things I should mention rather than have you discover them: go vet reports one unreachable code warning at parser.go:2023, and TestOracleSetPr286Regression fails — both reproduce identically on an unpatched tree here and are unrelated to path parsing.

Notes

512 is a judgement call, not derived from anything in the path grammar — happy to change it, inline the comparison, or drop the helper if you would prefer a smaller diff. I have not tried to show a practical denial of service in a real caller; the claim is limited to what the benchmark shows, which is allocation proportional to caller-supplied input on a parse that is rejected immediately.

Disclosure: this was found by a small static checker I wrote for this pattern, and the patch was written with AI assistance. I ran and verified every number above myself, including the before/after on the same clone and the check that the new test passes without the change.

[tzh476]

Correcting my own patch before you spend time on it: the constant ceiling I
originally proposed regresses valid deep paths, and I only found it because I went
back and benchmarked the legitimate case rather than just the hostile one.

Well-formed 2000-component path, same clone and machine, -benchtime 200x:

	B/op	allocs/op
upstream, no clamp	32,781	1
my original constant 512 ceiling	113,177	5
proportional len/2+1 (now pushed)	32,788	1

A 512 ceiling under-reserves any path with more components than that, so append
regrows and the common case pays for the hostile one. Deep paths are unusual but
legal, and a defensive bound shouldn't make valid input 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.

The hostile case is essentially unchanged by the correction — 200,000 separators
rejected on the first component still goes 1,606,185 B/op → 803,352 B/op.

Worth flagging why this slipped past my own test: the existing correctness test
passed with the constant too, because clamping never changes what ParsePath
returns. Allocation behaviour needed its own assertion, so
TestParsePathHintDoesNotRegressDeepPaths now checks it and fails on the constant
version with used 5 allocations, want 1.

One note for reproducing: TestOracleSetPr286Regression/pr286-top-level-oob and
/pr286-top-level-far already fail on a clean unpatched checkout of master here,
so please don't read those as this PR's fault — I checked the control before
believing my own diff. Happy to look at them separately if useful.

[tzh476]

Measuring my own patch's actual value before you spend review time on it: it is a 2x improvement with
no absolute ceiling, not a fix for the underlying amplification. I would rather say that myself than
have you find it.

I benchmarked both arms on the same machine, -benchtime 200x, darwin/arm64 go1.22.5:

	upstream	this PR
hostile (100,000 .)	1,605,633 B/op	802,836 B/op
legitimate 2000-component path	32,784 B/op, 1 alloc	32,768 B/op, 1 alloc
shallow a.b.c	48 B/op	48 B/op

So the hostile case halves and the valid cases are untouched. But the useful number is the
amplification ratio, and it is constant:

input=   1,000 bytes -> reserved  8,016 bytes   8.0x
input=  10,000 bytes -> reserved 80,016 bytes   8.0x
input= 100,000 bytes -> reserved 800,016 bytes  8.0x
input=1,000,000 bytes -> reserved 8,000,016 bytes 8.0x

len/2+1 takes the attacker from 16x to 8x. It does not bound the allocation — a caller passing an
attacker-controlled path can still command memory proportional to input length. If your threat model
here is "untrusted path string", this patch narrows the factor but does not close it.

I also checked whether the bound could simply be lowered, and it cannot. For the densest legal path
(k.k.k…, single-character keys) the reservation is exactly what the parse consumes:

len= 9,999  actual components=5,000  reserved=5,000  waste=1.00x
len=99,999  actual components=50,000 reserved=50,000 waste=1.00x

Any tighter bound under-reserves a well-formed deep path and makes append regrow — which is precisely
what my first version of this patch got wrong (a constant 512 ceiling took the valid 2000-component
case from 32,781 B in 1 alloc to 113,177 B in 5).

Given that, the honest framing is: this is a cheap, regression-free halving, and the real fix for
untrusted input would be a caller-side length limit on jsonPath rather than a hint clamp. If you
would rather not carry a partial mitigation, closing this is a reasonable call and I will not argue
it. The test in the PR does assert the allocation rather than just correctness, so whatever bound you
prefer is enforced.

[tzh476]

Correction to my own comment above, found by checking a claim I had already made to you.

I wrote: "The test in the PR does assert the allocation rather than just correctness, so whatever bound you prefer is enforced." That was misleading. I ran both tests against unpatched upstream master:

TestParsePathHintClampPreservesResults    PASS on upstream
TestParsePathHintDoesNotRegressDeepPaths  PASS on upstream

Neither fails without the clamp, so neither was holding the fix in place. In fairness to them, both say so in their own comments — the first is explicitly clamp-agnostic, and the second guards against a constant ceiling under-reserving valid deep paths, which upstream never had. But "asserts the allocation" implied to you that a future change loosening the bound would be caught, and it would not have been.

cb0c827 adds the test that is a real discriminator:

	result
unpatched upstream master	FAIL — allocated 1605637 B/op; want < 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 all-separator path is still rejected — so clamping can never turn a rejection into a pass.

While verifying, I also re-ran the benchmark numbers from my previous comment on a clean clone of the PR head, both arms, -benchtime 200x:

	upstream	this PR
hostile (100,000 .)	1,606,206 B/op	803,365 B/op
legitimate 2000-component path	32,788 B/op, 1 alloc	32,788 B/op, 1 alloc
shallow a.b.c	48 B/op	48 B/op

Those reproduce what I posted (802,836 / 32,768 / 48) to within run-to-run noise, so the 2× halving and the untouched valid cases both stand. My assessment of the patch's value is unchanged and still deliberately modest: it halves the factor, it does not bound the allocation, and if you would rather not carry a partial mitigation, closing this remains a reasonable call.

Two things I checked so you do not have to: TestOracleSetPr286Regression fails identically on pristine upstream master (pre-existing, not from this PR), and the two go vet findings are in parser.go and bytes_unsafe_test.go, neither of which this PR touches. gofmt is clean.

Also worth flagging: CI has not run on this PR — both workflows show action_required, so no job has executed. Everything above is from my machine on darwin/arm64 go1.22.5.