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

[xe-nvdk]

Summary

Closes #63. Two related changes (separate commits):

1. Security fix: bytes decode bypassed the alloc limit

The fork's readN/readNGrow split (upstream's readN was the chunked variant) left bytes()/bytesPtr() calling the unbounded package-level readN directly. A malicious bin32/str32 header declaring ~4GB forced a ~4GB upfront allocation — on stream decode and on Unmarshal — even with alloc limits enabled (the default). The string path was unaffected (d.readN already branches on the flag).

Bytes decoding now routes through a new readNInto method:

• Byte-slice (Unmarshal) path: declared length is validated against the remaining input before allocating → still a single exact-size allocation (1 alloc for 4MB, same as before), bomb closed for free.
• Stream path: chunked readNGrow by default; DisableAllocLimit(true) restores unbounded single-allocation reads for trusted input.

2. Perf (#63): exact-capacity chunked growth in readNGrow

Replaces append-based chunk growth with explicit min(n, max(2*pos, pos+bytesAllocLimit)) sizing — provably the same allocation-ahead-of-data bound as before (≤ max(received, 1MiB)), but no capacity overshoot beyond n and fewer alloc+copy rounds. readN similarly allocates exactly n, dropping a useless prefix copy.

Benchmarks (Apple M3 Max, benchstat, count=6)

benchmark	v6	this PR	delta
DecodeStringStream4MB (chunked on both)	549.7µs, 15.4MiB, 7 allocs	381.9µs, 11.0MiB, 6 allocs	-30.5% ns, -28.5% B
UnmarshalBytes4MB (byte-slice path)	1 alloc, 4MB	1 alloc, 4MB	unchanged
DecodeBytesStream4MB	139.6µs (unbounded, vulnerable)	317.2µs (chunked, safe)	cost of the fix; DisableAllocLimit restores old perf
DecodeBytesStream512KB	31.2µs	31.1µs	~

Testing

• Huge-declared-length tests on all three bytes paths (Decode(&[]byte), DecodeBytes, struct field) for stream, plus the Unmarshal path — fail fast with ≤1MiB allocated.
• 2.5MB round-trips (stream + Unmarshal + DisableAllocLimit); caller-ownership (no input aliasing) asserted.
• go test ./..., go test -race ./... pass; existing decoderTests EOF semantics preserved.
• 4-agent internal review (correctness/security/quality/perf) completed; all findings addressed (bsr single-alloc fast path, builtin min/max cleanup, CHANGELOG).

Note for follow-up

The security review also found a pre-existing allocation bomb in DecodeUntypedMap (decode_map.go:224 — make(map[interface{}]interface{}, n) without the maxMapSize clamp its siblings have). Separate PR incoming.

🤖 Generated with Claude Code

[gemini-code-assist]

Code Review

This pull request introduces changes to enforce allocation limits during msgpack bytes and string decoding, preventing malicious headers with huge declared lengths from triggering upfront out-of-memory allocations. It routes decoding through a new readNInto helper, optimizes slice growth in readNGrow to avoid capacity overshoot, and adds corresponding benchmarks and tests. The review feedback highlights critical integer overflow vulnerabilities on 32-bit architectures where large uint32 lengths can cast to negative int values, potentially bypassing length checks, causing panics during allocation, or being incorrectly interpreted as Nil values.

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.

[gemini-code-assist]

On 32-bit architectures, a large uint32 length (e.g., >= 2^31) decoded from bin32 or str32 headers can overflow when cast to int, resulting in a negative value for n. If n is negative, it will bypass the remaining check and cause a panic in make([]byte, n) or slice bounds out of range. We should add a check for n < 0 at the beginning of readNInto to prevent this.

Suggested change

	func (d *Decoder) readNInto(b []byte, n int) ([]byte, error) {
	func (d *Decoder) readNInto(b []byte, n int) ([]byte, error) {
	if n < 0 {
	return nil, fmt.Errorf("msgpack: invalid length %d", n)
	}

[gemini-code-assist]

To prevent panics or silent data corruption on 32-bit architectures when a large uint32 length overflows to a negative int, we should validate that n >= 0 at the beginning of readN.

func readN(r io.Reader, b []byte, n int) ([]byte, error) {
	if n < 0 {
		return nil, fmt.Errorf("msgpack: invalid length %d", n)
	}

[gemini-code-assist]

On 32-bit architectures, if pos is large (e.g., >= 2^30), 2*pos or pos+bytesAllocLimit can overflow to a negative value. This would cause max to return a negative value or a value smaller than expected, leading to a negative newCap and a panic in make([]byte, newCap). We should calculate newCap in an overflow-safe manner.

		newCap := pos + bytesAllocLimit
		if newCap < pos || newCap < 0 {
			newCap = n
		}
		if double := 2 * pos; double > newCap && double > 0 {
			newCap = double
		}
		if newCap > n || newCap < 0 {
			newCap = n
		}

[gemini-code-assist]

On 32-bit architectures, a large uint32 length (e.g., 0xffffffff) decoded from bin32 or str32 headers will overflow to -1 when cast to int. Since n == -1 is checked here, it will be incorrectly treated as msgpcode.Nil, returning nil, nil and silently succeeding instead of failing with an error. We should ensure that n == -1 only returns nil, nil if the code is actually msgpcode.Nil, and otherwise fail on negative lengths.

Suggested change

	if n == -1 {
	return nil, nil
	}
	return readN(d.r, b, n)
	return d.readNInto(b, n)
	if n == -1 && c == msgpcode.Nil {
	return nil, nil
	}
	if n < 0 {
	return nil, fmt.Errorf("msgpack: invalid length %d", n)
	}
	return d.readNInto(b, n)

[xe-nvdk]

The four 32-bit overflow findings are addressed, though not in this PR — they were a pre-existing issue affecting paths beyond this diff (decodeMapValue, decodeMapStringStringPtr, decodeTypedMapN too, as the note on #76 pointed out), so rather than scattering n < 0 checks across call sites I fixed it centrally in #79, now merged.

map32, array32, str32, and bin32 all route through one checked conversion at the point the length is decoded, so a negative n can no longer reach readNInto, make(), or a slice expression. readN/readNGrow reject negative lengths as defense in depth, and readNInto got an explicit guard in this PR (0ef1cf0) — it delegates to guarded functions, but its byte-slice branch checks n > remaining first, which would have surfaced a negative length as a misleading ErrUnexpectedEOF.

On the decode_string.go:95 finding specifically: the n == -1 check needs no c == msgpcode.Nil qualifier now, because bytesLen routes Str32/Bin32 through the checked conversion — -1 can only originate from an actual msgpcode.Nil, never from a truncated 0xffffffff.

Two things worth recording:

The Makefile ran GOARCH=386 go vet ./... — vet only, never the tests. That's why this class of bug survived. CI now runs the full suite under GOARCH=386 (386 binaries execute natively on amd64 runners).

That new job immediately caught a real bug in this PR. TestDecodeBytesHugeDeclaredLen and TestUnmarshalBytesHugeDeclaredLen declared 0xffffffff and asserted io.ErrUnexpectedEOF; on 32-bit that length is now rejected by the overflow check first, so the decode fails with "overflows int" instead. Correct behavior, wrong assertion. Fixed in 098a930 by using 0x7fffffff, which fits in an int on every platform and still declares ~2GB against a 1-byte payload — so the tests exercise chunked allocation, which is what they were written for.

All four checks green, including test (linux/386).