Skip to content

pgtype: hstore text parser pre-allocates from an unvalidated separator count #2639

Description

@tzh476

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.ScanscanStringparseHstore (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.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions