Skip to content

fix: clamp DecodeUntypedMap allocation at maxMapSize - #76

Merged
xe-nvdk merged 2 commits into
v6from
fix/untyped-map-alloc-limit
Aug 21, 2026
Merged

fix: clamp DecodeUntypedMap allocation at maxMapSize#76
xe-nvdk merged 2 commits into
v6from
fix/untyped-map-alloc-limit

Conversation

@xe-nvdk

@xe-nvdk xe-nvdk commented Jun 12, 2026

Copy link
Copy Markdown
Member

Summary

DecodeUntypedMap allocated map[interface{}]interface{} with the attacker-declared capacity — the only map decode path missing the maxMapSize clamp (decodeMapValue, decodeMapStringInterfaceN, and decodeTypedMapN all have it). A map32 header 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) unless DisableAllocLimit(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 ./... and go test -race ./... pass.

No benchmark changes: the clamp is one branch on a cold path (map header decode).

🤖 Generated with Claude Code

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread decode_map.go
Comment on lines +224 to +229
ln := n
if d.flags&disableAllocLimitFlag == 0 {
ln = min(ln, maxMapSize)
}

m := make(map[interface{}]interface{}, ln)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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)

Comment thread msgpack_test.go Outdated
// 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}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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.

Suggested change
data := []byte{0xdf, 0xff, 0xff, 0xff, 0xff}
data := []byte{0xdf, 0x7f, 0xff, 0xff, 0xff}

xe-nvdk added a commit that referenced this pull request Aug 21, 2026
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>
Ignacio Van Droogenbroeck and others added 2 commits August 21, 2026 16:45
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>
@xe-nvdk
xe-nvdk force-pushed the fix/untyped-map-alloc-limit branch from dea64f1 to 11be10b Compare August 21, 2026 22:47
@xe-nvdk
xe-nvdk merged commit 49757c7 into v6 Aug 21, 2026
4 checks passed
@xe-nvdk
xe-nvdk deleted the fix/untyped-map-alloc-limit branch August 21, 2026 22:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant