Summary
parseHstore sizes both a slice and a map from the number of > bytes in the untrusted input, before any pair is validated:
numPairsEstimate := strings.Count(s, ">")
valueStrings := make([]string, 0, numPairsEstimate)
result := make(Hstore, numPairsEstimate)
A value consisting only of > bytes reserves memory proportional to its length and then fails on the first consumeExpectedByte('"'), so none of it is ever used. make(Hstore, N) is the larger part of the cost, since it pre-sizes buckets.
This is reachable from wire data: scanPlanTextAnyToHstoreScanner.Scan → scanString → parseHstore (hstore.go:239), so the count comes from the server response rather than from caller-controlled config.
Measurement
Same clone, same machine (Apple M3 Pro, go1.25.0), -benchtime, input of 200,000 > bytes, rejected immediately:
before 754,783 ns/op 10,197,341 B/op 522 allocs/op
after 13,388 ns/op 75,265 B/op 9 allocs/op
A 200 KB value allocates ~10.2 MB, about 51×.
Two controls, because the interesting question is whether a clamp costs anything real:
5000 valid pairs (well past a 1024 clamp)
before 235,362 ns/op 340,403 B/op 10019 allocs/op
after 229,752 ns/op 317,944 B/op 10011 allocs/op
"a"=>"b" 274 B/op either way
200k '=' bytes ~2,180 B/op either way (only '>' drives the estimate)
So a legitimate large hstore is not penalised — marginally better, since the over-estimate from > inside values goes away.
Suggested fix
Clamp the estimate. Capacity is only a hint to append and the map size argument only a hint to the runtime, so this cannot change which inputs are accepted or what a successful parse returns — only how much is speculatively reserved for input that has not been validated yet.
const maxHstorePairsEstimate = 1024
numPairsEstimate := strings.Count(s, ">")
if numPairsEstimate > maxHstorePairsEstimate {
numPairsEstimate = maxHstorePairsEstimate
}
1024 is a judgement call, not derived from anything in the hstore format; happy to change it or drop the named constant.
I have an equivalence test that asserts identical results for 1, 2, 1023, 1024, 1025 and 5000 valid pairs — past the clamp, where append and map growth must take over — plus NULL, escaped quotes, and separators inside quoted values ("x,y=>z"), and that separator-only input is still rejected. It passes with and without the change and references no identifier the fix introduces, so it checks behaviour rather than the patch.
Limitations, stated plainly
- I have not demonstrated a real-world OOM. The claim is only what the benchmark shows: allocation proportional to attacker-supplied input on a parse that fails immediately.
TestHstoreCodec does not run here — no local PostgreSQL — so the DB-backed path is unverified by me. It fails identically on an unpatched tree, so it is an environment gap rather than a regression.
go vet ./pgtype/ already reports 108 unkeyed fields warnings on master, unrelated to this.
Per CONTRIBUTING I am raising an issue rather than sending a PR; glad to open one if you want the change, in whatever shape you prefer.
Disclosure per CONTRIBUTING §AI: this was written with AI assistance. I understand the change and ran every number above myself, including the before/after on one clone and the check that the equivalence test passes on an unpatched tree. I will answer review questions myself rather than relaying them.
Summary
parseHstoresizes both a slice and a map from the number of>bytes in the untrusted input, before any pair is validated:A value consisting only of
>bytes reserves memory proportional to its length and then fails on the firstconsumeExpectedByte('"'), so none of it is ever used.make(Hstore, N)is the larger part of the cost, since it pre-sizes buckets.This is reachable from wire data:
scanPlanTextAnyToHstoreScanner.Scan→scanString→parseHstore(hstore.go:239), so the count comes from the server response rather than from caller-controlled config.Measurement
Same clone, same machine (Apple M3 Pro, go1.25.0),
-benchtime, input of 200,000>bytes, rejected immediately:A 200 KB value allocates ~10.2 MB, about 51×.
Two controls, because the interesting question is whether a clamp costs anything real:
So a legitimate large hstore is not penalised — marginally better, since the over-estimate from
>inside values goes away.Suggested fix
Clamp the estimate. Capacity is only a hint to
appendand the map size argument only a hint to the runtime, so this cannot change which inputs are accepted or what a successful parse returns — only how much is speculatively reserved for input that has not been validated yet.1024is a judgement call, not derived from anything in the hstore format; happy to change it or drop the named constant.I have an equivalence test that asserts identical results for 1, 2, 1023, 1024, 1025 and 5000 valid pairs — past the clamp, where
appendand map growth must take over — plus NULL, escaped quotes, and separators inside quoted values ("x,y=>z"), and that separator-only input is still rejected. It passes with and without the change and references no identifier the fix introduces, so it checks behaviour rather than the patch.Limitations, stated plainly
TestHstoreCodecdoes not run here — no local PostgreSQL — so the DB-backed path is unverified by me. It fails identically on an unpatched tree, so it is an environment gap rather than a regression.go vet ./pgtype/already reports 108unkeyed fieldswarnings on master, unrelated to this.Per CONTRIBUTING I am raising an issue rather than sending a PR; glad to open one if you want the change, in whatever shape you prefer.
Disclosure per CONTRIBUTING §AI: this was written with AI assistance. I understand the change and ran every number above myself, including the before/after on one clone and the check that the equivalence test passes on an unpatched tree. I will answer review questions myself rather than relaying them.