From be5bf04a19078ee9d7b51152695a6dbdbbe9a860 Mon Sep 17 00:00:00 2001 From: Ignacio Van Droogenbroeck Date: Fri, 21 Aug 2026 16:42:22 -0600 Subject: [PATCH] fix: reject 32-bit length headers that overflow int 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) --- .github/workflows/build.yml | 21 +++++ CHANGELOG.md | 4 + Makefile | 6 ++ decode.go | 32 ++++++- decode_map.go | 2 +- decode_slice.go | 2 +- decode_string.go | 2 +- overflow_test.go | 162 ++++++++++++++++++++++++++++++++++++ 8 files changed, 227 insertions(+), 4 deletions(-) create mode 100644 overflow_test.go diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 215e0002..c6017e5d 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -27,3 +27,24 @@ jobs: run: make test env: GOGC: 50 + + test-32bit: + name: test (linux/386) + runs-on: ubuntu-latest + + steps: + - name: Checkout code + uses: actions/checkout@v4 + + - name: Set up Go + uses: actions/setup-go@v5 + with: + go-version: '1.26.x' + + # ubuntu-latest is amd64, which runs 386 binaries natively -- no + # emulation needed. Catches int overflow on 32-bit length headers. + - name: Test (32-bit) + run: go test ./... + env: + GOARCH: 386 + GOGC: 50 diff --git a/CHANGELOG.md b/CHANGELOG.md index 80ce3428..9e41e002 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,9 @@ ## Unreleased +### Fixed + +- **decode:** reject 32-bit length headers that overflow `int` on 32-bit platforms — `map32`, `array32`, `str32`, and `bin32` headers declaring a length above `math.MaxInt` were converted with `int(n)`, which on 32-bit builds wraps to a negative value. A length of `0xffffffff` wrapped to `-1` and was silently decoded as a nil map/array/string (wrong result, no error); values in `[0x80000000, 0xfffffffe]` wrapped to other negatives and panicked in `make()` or on a slice expression. All four headers now route through a single checked conversion, and `readN`/`readNGrow` reject negative lengths as defense in depth. 64-bit platforms are unaffected (every `uint32` fits in an `int`). CI now runs the suite under `GOARCH=386`. + ### Performance - **decode:** reuse caller-supplied destination map for `map[string]interface{}` — `Decode(&m)`/`Unmarshal(data, &m)` with a non-nil `m` now decodes into the existing map (entries merged) instead of replacing it with a fresh allocation, matching the long-standing `map[string]string` behavior. Applies to all decode paths for the type: the `Decode()` fast path, struct fields, and named map types. Callers that `clear(m)` and reuse the destination get zero map allocations per decode ([#61](https://github.com/Basekick-Labs/msgpack/issues/61)) (decoding a 4-key map into a reused destination, v6 vs this change on the same benchmark: **-22.9% ns/op**, **-80.8% B/op**, 12 → 10 allocs/op). Note: this diverges from upstream, which replaces a non-nil `map[string]interface{}` destination; pass a nil map to keep replace semantics. diff --git a/Makefile b/Makefile index 674eb3fb..9244531a 100644 --- a/Makefile +++ b/Makefile @@ -3,4 +3,10 @@ test: go test ./... -short -race -count=1 -timeout=5m go test ./... -run=NONE -bench=. -benchmem env GOOS=linux GOARCH=386 go vet ./... + # 386 binaries run natively on amd64 hosts (CI); skipped elsewhere. + @if [ "$$(go env GOHOSTOS)/$$(go env GOHOSTARCH)" = "linux/amd64" ]; then \ + echo "env GOARCH=386 go test ./..."; env GOARCH=386 go test ./...; \ + else \ + echo "skipping 32-bit test run (needs linux/amd64 host)"; \ + fi go vet diff --git a/decode.go b/decode.go index bfcd5225..39501149 100644 --- a/decode.go +++ b/decode.go @@ -5,6 +5,7 @@ import ( "errors" "fmt" "io" + "math" "reflect" "sync" "time" @@ -733,9 +734,12 @@ func (d *Decoder) readFull(b []byte) error { } func (d *Decoder) readN(n int) ([]byte, error) { + if n < 0 { + return nil, fmt.Errorf("msgpack: invalid length %d", n) + } // Fast path: byte-slice reader — zero-copy sub-slice of input buffer. if d.bsr.data != nil { - if d.bsr.pos+n > len(d.bsr.data) { + if n > len(d.bsr.data)-d.bsr.pos { return nil, io.ErrUnexpectedEOF } b := d.bsr.data[d.bsr.pos : d.bsr.pos+n] @@ -762,6 +766,9 @@ func (d *Decoder) readN(n int) ([]byte, error) { } func readN(r io.Reader, b []byte, n int) ([]byte, error) { + if n < 0 { + return nil, fmt.Errorf("msgpack: invalid length %d", n) + } if b == nil { if n == 0 { return make([]byte, 0), nil @@ -780,6 +787,9 @@ func readN(r io.Reader, b []byte, n int) ([]byte, error) { } func readNGrow(r io.Reader, b []byte, n int) ([]byte, error) { + if n < 0 { + return nil, fmt.Errorf("msgpack: invalid length %d", n) + } if b == nil { if n == 0 { return make([]byte, 0), nil @@ -826,3 +836,23 @@ func min(a, b int) int { //nolint:unparam } return b } + +// uint32Len converts a length decoded from a 32-bit msgpack header to int. +// +// On 64-bit platforms every uint32 fits in an int and this is a no-op. On +// 32-bit platforms a value above math.MaxInt32 would wrap to a negative +// int, which callers treat as "nil" (-1) or pass to make(), so it is +// rejected instead. err is threaded through so call sites stay one-liners. +func uint32Len(n uint32, err error, what string) (int, error) { + if err != nil { + return 0, err + } + if uint64(n) > uint64(maxLenForInt) { + return 0, fmt.Errorf("msgpack: %s length %d overflows int", what, n) + } + return int(n), nil +} + +// maxLenForInt is math.MaxInt, in a variable so tests can lower it to +// math.MaxInt32 and exercise the 32-bit rejection path on a 64-bit host. +var maxLenForInt uint64 = math.MaxInt diff --git a/decode_map.go b/decode_map.go index c141f525..0900c16d 100644 --- a/decode_map.go +++ b/decode_map.go @@ -108,7 +108,7 @@ func (d *Decoder) mapLen(c byte) (int, error) { } if c == msgpcode.Map32 { size, err := d.uint32() - return int(size), err + return uint32Len(size, err, "map") } return 0, unexpectedCodeError{code: c, hint: "map length"} } diff --git a/decode_slice.go b/decode_slice.go index f463ec52..b1bac1fd 100644 --- a/decode_slice.go +++ b/decode_slice.go @@ -33,7 +33,7 @@ func (d *Decoder) arrayLen(c byte) (int, error) { return int(n), err case msgpcode.Array32: n, err := d.uint32() - return int(n), err + return uint32Len(n, err, "array") } return 0, fmt.Errorf("msgpack: invalid code=%x decoding array length", c) } diff --git a/decode_string.go b/decode_string.go index f7ac1077..bfd5b0f7 100644 --- a/decode_string.go +++ b/decode_string.go @@ -25,7 +25,7 @@ func (d *Decoder) bytesLen(c byte) (int, error) { return int(n), err case msgpcode.Str32, msgpcode.Bin32: n, err := d.uint32() - return int(n), err + return uint32Len(n, err, "string/bytes") } return 0, fmt.Errorf("msgpack: invalid code=%x decoding string/bytes length", c) diff --git a/overflow_test.go b/overflow_test.go new file mode 100644 index 00000000..b6d4998c --- /dev/null +++ b/overflow_test.go @@ -0,0 +1,162 @@ +package msgpack + +import ( + "math" + "strings" + "testing" +) + +// uint32Len is the single conversion point for 32-bit msgpack length +// headers. On 64-bit platforms every uint32 fits in an int; on 32-bit +// platforms values above math.MaxInt32 must be rejected rather than +// wrapped to a negative int (which callers would read as nil, or pass to +// make()). The table drives uint32Len directly so both behaviors are +// covered regardless of the host word size. +func TestUint32LenOverflow(t *testing.T) { + tests := []struct { + name string + n uint32 + wantErr bool + }{ + {name: "zero", n: 0}, + {name: "small", n: 42}, + {name: "maxint32", n: math.MaxInt32}, + // Wraps to a negative int on 32-bit; fine on 64-bit. + {name: "maxint32+1", n: math.MaxInt32 + 1, wantErr: intIs32Bit}, + // Wraps to exactly -1 on 32-bit -- the "silently decodes as nil" case. + {name: "maxuint32", n: math.MaxUint32, wantErr: intIs32Bit}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := uint32Len(tt.n, nil, "test") + if tt.wantErr { + if err == nil { + t.Fatalf("uint32Len(%d) = %d, want overflow error", tt.n, got) + } + if !strings.Contains(err.Error(), "overflows int") { + t.Fatalf("uint32Len(%d) err = %v, want overflow error", tt.n, err) + } + return + } + if err != nil { + t.Fatalf("uint32Len(%d) unexpected error: %v", tt.n, err) + } + if got < 0 { + t.Fatalf("uint32Len(%d) = %d, must never be negative", tt.n, got) + } + if uint32(got) != tt.n { + t.Fatalf("uint32Len(%d) = %d, want %d", tt.n, got, tt.n) + } + }) + } +} + +const intIs32Bit = math.MaxInt == math.MaxInt32 + +// An existing error is passed through untouched. +func TestUint32LenPropagatesError(t *testing.T) { + sentinel := errTestSentinel + if _, err := uint32Len(math.MaxUint32, sentinel, "test"); err != sentinel { + t.Fatalf("uint32Len err = %v, want the incoming error", err) + } +} + +var errTestSentinel = errSentinel{} + +type errSentinel struct{} + +func (errSentinel) Error() string { return "sentinel" } + +// Negative lengths must never reach make() or a slice expression. +func TestReadNRejectsNegative(t *testing.T) { + d := NewDecoder(strings.NewReader("whatever")) + if _, err := d.readN(-1); err == nil { + t.Fatal("d.readN(-1) = nil error, want invalid length error") + } + if _, err := readN(strings.NewReader("x"), nil, -1); err == nil { + t.Fatal("readN(-1) = nil error, want invalid length error") + } + if _, err := readNGrow(strings.NewReader("x"), nil, -1); err == nil { + t.Fatal("readNGrow(-1) = nil error, want invalid length error") + } +} + +// Forces the 32-bit rejection path on any host by lowering the ceiling to +// math.MaxInt32, so CI proves the guard works without a 32-bit runner. +func TestUint32LenRejectsOverflowAs32Bit(t *testing.T) { + orig := maxLenForInt + maxLenForInt = math.MaxInt32 + defer func() { maxLenForInt = orig }() + + for _, n := range []uint32{math.MaxInt32 + 1, 0x80000000, math.MaxUint32} { + got, err := uint32Len(n, nil, "test") + if err == nil { + t.Fatalf("uint32Len(%#x) = %d, want overflow error when int is 32-bit", n, got) + } + } + // Values that still fit must keep working. + if got, err := uint32Len(math.MaxInt32, nil, "test"); err != nil || got != math.MaxInt32 { + t.Fatalf("uint32Len(MaxInt32) = %d, %v; want %d, nil", got, err, int64(math.MaxInt32)) + } +} + +// End-to-end: malicious 32-bit headers decoded through the public API with +// the ceiling forced to 32-bit. Each must return an error rather than +// panicking in make() or silently decoding as nil. +func TestDecodeRejectsOverflowingHeadersAs32Bit(t *testing.T) { + orig := maxLenForInt + maxLenForInt = math.MaxInt32 + defer func() { maxLenForInt = orig }() + + tests := []struct { + name string + data []byte + dst func() interface{} + }{ + { + name: "map32 maxuint32", + data: []byte{0xdf, 0xff, 0xff, 0xff, 0xff}, + dst: func() interface{} { return &map[string]interface{}{} }, + }, + { + name: "array32 maxuint32", + data: []byte{0xdd, 0xff, 0xff, 0xff, 0xff}, + dst: func() interface{} { return &[]interface{}{} }, + }, + { + name: "str32 maxuint32", + data: []byte{0xdb, 0xff, 0xff, 0xff, 0xff}, + dst: func() interface{} { var s string; return &s }, + }, + { + name: "bin32 maxuint32", + data: []byte{0xc6, 0xff, 0xff, 0xff, 0xff}, + dst: func() interface{} { var b []byte; return &b }, + }, + { + name: "str32 0x80000000", + data: []byte{0xdb, 0x80, 0x00, 0x00, 0x00}, + dst: func() interface{} { var s string; return &s }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + defer func() { + if r := recover(); r != nil { + t.Fatalf("panic decoding %s: %v", tt.name, r) + } + }() + err := Unmarshal(tt.data, tt.dst()) + if err == nil { + t.Fatalf("Unmarshal(%s) = nil error, want overflow rejection", tt.name) + } + // Must be rejected at the length check, not incidentally by a + // later EOF -- that distinction is the whole point of the guard. + if !strings.Contains(err.Error(), "overflows int") { + t.Fatalf("Unmarshal(%s) err = %v, want an \"overflows int\" rejection", tt.name, err) + } + }) + } +}