fix: reject 32-bit length headers that overflow int by xe-nvdk · Pull Request #79 · Basekick-Labs/msgpack

xe-nvdk · 2026-08-21T22:42:44Z

Summary

Central fix for the 32-bit integer overflow that gemini-code-assist flagged across #75 and #76. Rather than bolting per-call-site n < 0 checks onto those two PRs, this fixes it once at the four places lengths actually enter the decoder.

The bug

map32, array32, str32, and bin32 headers all did a plain int(n) on a decoded uint32. On 32-bit builds that wraps:

header	`int(n)` on 32-bit	result
`0xffffffff`	`-1`	silently decoded as nil map/array/string — wrong result, no error
`0x80000000`	`-2147483648`	negative → panic in `make()` / slice bounds
`0x7fffffff`	`2147483647`	fine

The -1 case is the worse one: it's silent data corruption, not a crash.

The fix

All four headers route through one checked conversion, uint32Len. As defense in depth, readN, d.readN, and readNGrow reject negative lengths, and the byte-slice fast path now compares n > len(data)-pos instead of the overflow-prone pos+n > len(data).

64-bit platforms are unaffected — every uint32 fits in an int, so the check is a constant-false branch.

Why this wasn't caught

The Makefile ran GOOS=linux GOARCH=386 go vet ./... — vet only, never the tests. So the code compiled for 32-bit but the suite never executed there. CI now runs the full suite under GOARCH=386 (386 binaries execute natively on amd64 runners, no emulation), and make test does the same on amd64 Linux hosts, skipping cleanly elsewhere.

Testing

The ceiling lives in maxLenForInt so tests can lower it to math.MaxInt32 and exercise the 32-bit rejection path on a 64-bit host:

TestUint32LenOverflow — table over boundary values, host-word-size aware.
TestUint32LenRejectsOverflowAs32Bit — forces the 32-bit ceiling, asserts rejection and that MaxInt32 still works.
TestDecodeRejectsOverflowingHeadersAs32Bit — end-to-end Unmarshal of malicious map32/array32/str32/bin32 payloads, asserting the specific "overflows int" error rather than an incidental EOF.
TestReadNRejectsNegative — the three readN entry points.

Verified the tests actually catch the bug: reverting the three call sites to int(n) fails all five end-to-end subtests; restoring the fix passes them. An earlier version of the test only asserted "some error" and passed without the fix — that was tightened.

go test ./..., go test -race ./..., and full make test pass. No benchmark regression (StructUnmarshal, MapStringInterfaceMsgpack flat).

Note

I could not execute a real 32-bit binary locally — Docker on Apple Silicon has no working linux/386 or linux/arm/v7 emulation here (exec format error). The 32-bit behavior is therefore verified by forcing the ceiling on a 64-bit host, plus the new CI job which does run natively on amd64. Worth a look at the first CI run on this PR to confirm the 386 job goes green.

Once this lands, #75 and #76 should be rebased on it — their remaining content is sound.

🤖 Generated with Claude Code

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: Claude Opus 5 (1M context) <noreply@anthropic.com>

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>

readNInto delegates to readN/readNGrow, which reject negative lengths since #79, but the byte-slice branch checks n > remaining first -- a negative n would surface as a misleading ErrUnexpectedEOF instead of an invalid-length error. Guard explicitly at the entry point. The n == -1 check in bytes()/bytesPtr() needs no change: bytesLen now routes Str32/Bin32 through uint32Len, so -1 can only originate from an actual msgpcode.Nil rather than a truncated 0xffffffff. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

The bytes tests declared 0xffffffff and asserted io.ErrUnexpectedEOF. On 32-bit builds that length exceeds math.MaxInt and is now rejected by the length-overflow check before the chunked read is reached, so the decode fails with "overflows int" instead -- correct behavior, wrong assertion. The GOARCH=386 job added in #79 caught it. Changed to 0x7fffffff, which fits in an int on every platform and still declares ~2GB against a 1-byte payload, so the tests exercise what they were written for: chunked allocation rather than an upfront one. Verified by forcing the 32-bit ceiling on a 64-bit host; both tests pass under the simulated 32-bit path and on amd64. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* fix: clamp DecodeUntypedMap allocation at maxMapSize 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. * test: assert the allocation bound in the untyped-map clamp test 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> --------- Co-authored-by: Ignacio Van Droogenbroeck <ignacio@vandroogenbroeck.net> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>

readNInto delegates to readN/readNGrow, which reject negative lengths since #79, but the byte-slice branch checks n > remaining first -- a negative n would surface as a misleading ErrUnexpectedEOF instead of an invalid-length error. Guard explicitly at the entry point. The n == -1 check in bytes()/bytesPtr() needs no change: bytesLen now routes Str32/Bin32 through uint32Len, so -1 can only originate from an actual msgpcode.Nil rather than a truncated 0xffffffff. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

The bytes tests declared 0xffffffff and asserted io.ErrUnexpectedEOF. On 32-bit builds that length exceeds math.MaxInt and is now rejected by the length-overflow check before the chunked read is reached, so the decode fails with "overflows int" instead -- correct behavior, wrong assertion. The GOARCH=386 job added in #79 caught it. Changed to 0x7fffffff, which fits in an int on every platform and still declares ~2GB against a 1-byte payload, so the tests exercise what they were written for: chunked allocation rather than an upfront one. Verified by forcing the 32-bit ceiling on a 64-bit host; both tests pass under the simulated 32-bit path and on amd64. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

…row growth (#63) (#75) * fix: enforce alloc limit in bytes decode paths The fork's readN/readNGrow split (upstream's readN was the chunked, limit-respecting variant) left bytes(), bytesPtr() calling the package-level readN directly — the unbounded variant that allocates the full declared length upfront. On a stream decode, a malicious bin32/ str32 header declaring ~4GB forced a ~4GB allocation before any payload was read, even with alloc limits enabled (the default). The string path (d.readN method) was unaffected — it already branches on the flag. Route bytes decoding through a new readNInto method that mirrors d.readN's branch: chunked readNGrow by default, unbounded readN only when DisableAllocLimit(true) is set. Trusted workloads with large binary payloads can keep the old single-allocation behavior via DisableAllocLimit; the follow-up commit reduces the cost of the chunked path. * perf: exact-capacity chunked growth in readNGrow Replace the append-based chunk growth in readNGrow with explicit exact-capacity sizing: grow to min(n, max(2*pos, pos+bytesAllocLimit)) per round. This preserves the prior security bound — allocation never runs more than max(received, bytesAllocLimit) ahead of data actually supplied — while eliminating append's capacity overshoot beyond n and reducing alloc+copy rounds. readN (DisableAllocLimit path) similarly allocates exactly n instead of append-growing, skipping a useless copy of old contents that ReadFull overwrites anyway. 4MB stream decode vs v6 (count=6, Apple M3 Max): DecodeStringStream4MB 549.7µs → 381.9µs (-30.5%) 15.4MiB → 11.0MiB (-28.5%) 7 → 6 allocs The bytes path is slower than v6 (+127%) only because v6 skipped the alloc limit entirely there (see previous commit); against the corrected chunked baseline this change is -20% time, -38% B/op. DisableAllocLimit restores single-allocation reads for trusted input. Closes #63 * refactor: address internal review findings for bytes alloc-limit fix - readNInto: validate the declared length against remaining input on the byte-slice (Unmarshal) path before allocating — restores single exact-size allocation there (1 alloc for a 4MB payload, matching the pre-fix behavior) while also closing the same declared-length bomb on the byte-slice path. Error semantics match io.ReadFull (EOF vs ErrUnexpectedEOF), keeping decoderTests expectations intact. - Remove the pre-Go-1.21 local min helper; use builtin min/max. - Add Unmarshal-path tests: huge declared length fails fast; 2.5MB round-trip; result is caller-owned (no aliasing of the input). - Add CHANGELOG entries (Bug Fixes + Performance) for #63. * fix: guard readNInto against negative lengths readNInto delegates to readN/readNGrow, which reject negative lengths since #79, but the byte-slice branch checks n > remaining first -- a negative n would surface as a misleading ErrUnexpectedEOF instead of an invalid-length error. Guard explicitly at the entry point. The n == -1 check in bytes()/bytesPtr() needs no change: bytesLen now routes Str32/Bin32 through uint32Len, so -1 can only originate from an actual msgpcode.Nil rather than a truncated 0xffffffff. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test: use a 32-bit-safe length in the huge-declared-length tests The bytes tests declared 0xffffffff and asserted io.ErrUnexpectedEOF. On 32-bit builds that length exceeds math.MaxInt and is now rejected by the length-overflow check before the chunked read is reached, so the decode fails with "overflows int" instead -- correct behavior, wrong assertion. The GOARCH=386 job added in #79 caught it. Changed to 0x7fffffff, which fits in an int on every platform and still declares ~2GB against a 1-byte payload, so the tests exercise what they were written for: chunked allocation rather than an upfront one. Verified by forcing the 32-bit ceiling on a 64-bit host; both tests pass under the simulated 32-bit path and on amd64. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Ignacio Van Droogenbroeck <ignacio@vandroogenbroeck.net> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>

xe-nvdk merged commit 340d89f into v6 Aug 21, 2026
4 checks passed

xe-nvdk deleted the fix/32bit-length-overflow branch August 21, 2026 22:44

xe-nvdk mentioned this pull request Aug 21, 2026

fix: enforce alloc limit in bytes decode; perf: exact-capacity readNGrow growth (#63) #75

Merged

Navigation Menu

Uh oh!

fix: reject 32-bit length headers that overflow int - #79

fix: reject 32-bit length headers that overflow int#79
xe-nvdk merged 1 commit into
v6from
fix/32bit-length-overflow

xe-nvdk commented Aug 21, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

xe-nvdk commented Aug 21, 2026

Summary

The bug

The fix

Why this wasn't caught

Testing

Note

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant