fix: clamp DecodeUntypedMap allocation at maxMapSize - #76
Conversation
There was a problem hiding this comment.
Code Review
This pull request limits the initial allocation size of untyped maps during decoding to prevent excessive memory usage when a large map size is declared. Feedback focuses on 32-bit architecture compatibility: first, a signed integer overflow can bypass the nil map check and cause a panic in make, which requires adding a check for n < -1; second, the test case using 0xffffffff will overflow to -1 on 32-bit systems and should be updated to 0x7fffffff to avoid false positives.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| ln := n | ||
| if d.flags&disableAllocLimitFlag == 0 { | ||
| ln = min(ln, maxMapSize) | ||
| } | ||
|
|
||
| m := make(map[interface{}]interface{}, ln) |
There was a problem hiding this comment.
On 32-bit architectures, an extremely large declared map size (e.g., 0x80000000 / 2147483648) can overflow the signed 32-bit int and result in a negative value other than -1 (e.g., -2147483648). This bypasses the n == -1 check, leading to a negative ln value and causing make(map[interface{}]interface{}, ln) to panic with panic: size out of range.
To prevent this panic, we should check if n < -1 and return an error.
Note: The other map decoding paths (decodeMapValue, decodeMapStringStringPtr, and decodeTypedMapN) also suffer from this same overflow panic on 32-bit systems and should be updated similarly.
if n < -1 {
return nil, fmt.Errorf("msgpack: invalid map length: %d", n)
}
ln := n
if d.flags&disableAllocLimitFlag == 0 {
ln = min(ln, maxMapSize)
}
m := make(map[interface{}]interface{}, ln)| // hint must be clamped at maxMapSize before allocation (with the old | ||
| // code this allocated a multi-GB map upfront), then fail decoding the | ||
| // first key. | ||
| data := []byte{0xdf, 0xff, 0xff, 0xff, 0xff} |
There was a problem hiding this comment.
On 32-bit architectures, 0xffffffff (4294967295) overflows the signed 32-bit int and becomes -1. This causes DecodeMapLen() to return -1, which is interpreted as a nil map, making DecodeUntypedMap() return nil, nil without an error. As a result, this test will fail on 32-bit platforms because err is nil.
Using 0x7fffffff (2147483647) instead avoids the overflow on 32-bit systems while still being large enough to trigger the allocation clamp and fail decoding due to the missing payload.
| data := []byte{0xdf, 0xff, 0xff, 0xff, 0xff} | |
| data := []byte{0xdf, 0x7f, 0xff, 0xff, 0xff} |
map32/array32/str32/bin32 headers were converted to int with a plain int(n). On 32-bit builds a uint32 above math.MaxInt wraps negative: 0xffffffff becomes -1 and is silently decoded as a nil map/array/string, and values in [0x80000000, 0xfffffffe] become other negatives that panic in make() or on a slice expression. All four headers now go through a single checked conversion, uint32Len. readN, d.readN, and readNGrow additionally reject negative lengths as defense in depth, and the byte-slice fast path avoids an overflowing pos+n comparison. 64-bit platforms are unaffected -- every uint32 fits in an int, and the check compiles to a constant-false branch. The ceiling lives in maxLenForInt so tests can lower it to MaxInt32 and exercise the rejection path on a 64-bit host; without the fix all five end-to-end subtests fail. The Makefile only ran `GOARCH=386 go vet`, never the tests, which is why this was missed -- CI now runs the full suite under GOARCH=386 (386 binaries execute natively on amd64 runners). Reported by gemini-code-assist on #75 and #76. Co-authored-by: Ignacio Van Droogenbroeck <ignacio@vandroogenbroeck.net> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
DecodeUntypedMap allocated map[interface{}]interface{} with the
attacker-declared capacity, unlike every sibling map decode path
(decodeMapValue, decodeMapStringInterfaceN, decodeTypedMapN), which
clamp the size hint at maxMapSize unless DisableAllocLimit is set. A
map32 header declaring ~4G entries forced a multi-GB upfront map
allocation before any payload was read.
Found by internal security review of #63.
Two fixes from the review on #76. The header was 0xffffffff, which #79 now rejects at the int-overflow check before it ever reaches the clamp -- the test would have passed for the wrong reason. Changed to 0x7fffffff, which fits in an int on every platform and exercises the clamp itself. The test also only asserted that an error came back, which it did with or without the clamp: unclamped, the decode still failed on the missing payload, just after allocating the declared ~2G entries. It now measures TotalAlloc across the decode and fails above 256 MiB. Verified: without the clamp it reports 164000 MiB and fails in 33s; with it, well under the bound in 0.3s. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
dea64f1 to
11be10b
Compare
Summary
DecodeUntypedMapallocatedmap[interface{}]interface{}with the attacker-declared capacity — the only map decode path missing themaxMapSizeclamp (decodeMapValue,decodeMapStringInterfaceN, anddecodeTypedMapNall have it). Amap32header declaring ~4G entries forced a multi-GB upfront map allocation before any payload was read. Found by the internal security review pass on #75.The fix mirrors the sibling paths exactly: clamp the size hint at
maxMapSize(1M) unlessDisableAllocLimit(true)is set. The map still grows to the real size if the payload genuinely contains more entries.Testing
TestDecodeUntypedMapHugeDeclaredLen: map32 declaring ~4G entries fails fast (previously attempted a multi-GB allocation).TestDecodeUntypedMap: round-trip coverage for the path (it had none).go test ./...andgo test -race ./...pass.No benchmark changes: the clamp is one branch on a cold path (map header decode).
🤖 Generated with Claude Code