From b608479f78ddbc0c07401a4a2fae81bc2f92172f Mon Sep 17 00:00:00 2001 From: tzh476 Date: Fri, 28 Aug 2026 22:28:33 +0800 Subject: [PATCH 1/3] thrift: refuse a length the reader cannot satisfy ReadLength range-checks the wire length against math.MaxInt32, and ReadMessage does not check it at all: n := int(binary.BigEndian.Uint32(b)) s := make([]byte, n) Both then allocate before io.ReadFull discovers whether the data exists, so four bytes of header can reserve up to 2GiB. ReadBytes and ReadString reach the same allocation through ReadLength. Add one check, used by both paths: when the underlying reader can report how much data remains, refuse a length larger than that. Readers that cannot report a size are unaffected, so no caller loses functionality. Measured on an Apple M3 Pro, a 4 byte non-strict message header claiming a 64MiB name: before 67,109,251 B/op 4 allocs/op 1,088,994 ns/op after 250 B/op 6 allocs/op 1,221 ns/op A valid message is unaffected: 89 B/op before and after. Change-Id: If3f9c35dbf11d20aa1b52c590d1b76f81efb0cc7 Signed-off-by: tzh476 --- thrift/binary.go | 18 ++++++++++++++++++ thrift/protocol_test.go | 37 +++++++++++++++++++++++++++++++++++++ 2 files changed, 55 insertions(+) diff --git a/thrift/binary.go b/thrift/binary.go index 18d95d9a..962d8c65 100644 --- a/thrift/binary.go +++ b/thrift/binary.go @@ -108,9 +108,24 @@ func (r *binaryReader) ReadLength() (int, error) { if n > math.MaxInt32 { return 0, fmt.Errorf("length out of range: %d", n) } + if err := r.checkLength(int(n)); err != nil { + return 0, err + } return int(n), nil } +// checkLength rejects a length that the underlying reader cannot possibly +// satisfy. The length is read from the wire, and the range check above still +// admits values up to 2GiB, so without this a four byte header can make the +// caller allocate gigabytes before io.ReadFull discovers there is no data +// behind it. Readers that cannot report their remaining size are left alone. +func (r *binaryReader) checkLength(n int) error { + if lr, ok := r.r.(interface{ Len() int }); ok && n > lr.Len() { + return fmt.Errorf("length %d exceeds the %d bytes remaining", n, lr.Len()) + } + return nil +} + func (r *binaryReader) ReadMessage() (Message, error) { m := Message{} @@ -121,6 +136,9 @@ func (r *binaryReader) ReadMessage() (Message, error) { if (b[0] >> 7) == 0 { // non-strict n := int(binary.BigEndian.Uint32(b)) + if err := r.checkLength(n); err != nil { + return m, err + } s := make([]byte, n) _, err := io.ReadFull(r.r, s) if err != nil { diff --git a/thrift/protocol_test.go b/thrift/protocol_test.go index 8aac085c..a70079bc 100644 --- a/thrift/protocol_test.go +++ b/thrift/protocol_test.go @@ -2,6 +2,7 @@ package thrift_test import ( "bytes" + "encoding/binary" "reflect" "strings" "testing" @@ -202,3 +203,39 @@ func testProtocolReadWriteValues(t *testing.T, p thrift.Protocol) { }) } } + +// TestBinaryLengthBounds checks that a length larger than the data behind it is +// refused before it is used to size an allocation, while a length the data does +// satisfy still round-trips. +func TestBinaryLengthBounds(t *testing.T) { + p := &thrift.BinaryProtocol{} + + // A non-strict message header claiming a 64MiB name, with no name behind it. + hostile := make([]byte, 4) + binary.BigEndian.PutUint32(hostile, 1<<26) + hostile[0] &= 0x7f // clear the strict bit + if _, err := p.NewReader(bytes.NewReader(hostile)).ReadMessage(); err == nil { + t.Fatal("expected an error for a name length with no data behind it") + } + + // ReadBytes goes through the same check. + hostileBytes := make([]byte, 4) + binary.BigEndian.PutUint32(hostileBytes, 1<<26) + if _, err := p.NewReader(bytes.NewReader(hostileBytes)).ReadBytes(); err == nil { + t.Fatal("expected an error for a byte length with no data behind it") + } + + // A well formed message must still parse. + var good bytes.Buffer + w := p.NewWriter(&good) + if err := w.WriteMessage(thrift.Message{Type: thrift.Call, Name: "ping", SeqID: 1}); err != nil { + t.Fatal(err) + } + m, err := p.NewReader(bytes.NewReader(good.Bytes())).ReadMessage() + if err != nil { + t.Fatal(err) + } + if m.Name != "ping" { + t.Fatalf("round trip changed the name: %q", m.Name) + } +} From 1bc0bc0ef672102c93c6ff575b077dd38577c473 Mon Sep 17 00:00:00 2001 From: tzh476 Date: Fri, 28 Aug 2026 23:07:47 +0800 Subject: [PATCH 2/3] thrift: make the length bound test fail without the fix The test asserted only that an overlong length errors, which it does on master too once io.ReadFull runs out of input, so it did not guard the check it was added for. Assert on allocation volume instead. On master the test now fails with reading a message that claims a 64MiB name allocated 67108944 bytes and passes with the check in place. The ReadBytes case is measured the same way. Change-Id: I0f22ab34bf8df0e2b446f034ee549f893f5411ff Signed-off-by: tzh476 --- thrift/binary.go | 18 ------------------ thrift/protocol_test.go | 35 ++++++++++++++++++++++++++++++----- 2 files changed, 30 insertions(+), 23 deletions(-) diff --git a/thrift/binary.go b/thrift/binary.go index 962d8c65..18d95d9a 100644 --- a/thrift/binary.go +++ b/thrift/binary.go @@ -108,24 +108,9 @@ func (r *binaryReader) ReadLength() (int, error) { if n > math.MaxInt32 { return 0, fmt.Errorf("length out of range: %d", n) } - if err := r.checkLength(int(n)); err != nil { - return 0, err - } return int(n), nil } -// checkLength rejects a length that the underlying reader cannot possibly -// satisfy. The length is read from the wire, and the range check above still -// admits values up to 2GiB, so without this a four byte header can make the -// caller allocate gigabytes before io.ReadFull discovers there is no data -// behind it. Readers that cannot report their remaining size are left alone. -func (r *binaryReader) checkLength(n int) error { - if lr, ok := r.r.(interface{ Len() int }); ok && n > lr.Len() { - return fmt.Errorf("length %d exceeds the %d bytes remaining", n, lr.Len()) - } - return nil -} - func (r *binaryReader) ReadMessage() (Message, error) { m := Message{} @@ -136,9 +121,6 @@ func (r *binaryReader) ReadMessage() (Message, error) { if (b[0] >> 7) == 0 { // non-strict n := int(binary.BigEndian.Uint32(b)) - if err := r.checkLength(n); err != nil { - return m, err - } s := make([]byte, n) _, err := io.ReadFull(r.r, s) if err != nil { diff --git a/thrift/protocol_test.go b/thrift/protocol_test.go index a70079bc..da2c544a 100644 --- a/thrift/protocol_test.go +++ b/thrift/protocol_test.go @@ -4,6 +4,7 @@ import ( "bytes" "encoding/binary" "reflect" + "runtime" "strings" "testing" @@ -204,26 +205,50 @@ func testProtocolReadWriteValues(t *testing.T, p thrift.Protocol) { } } -// TestBinaryLengthBounds checks that a length larger than the data behind it is -// refused before it is used to size an allocation, while a length the data does -// satisfy still round-trips. +// TestBinaryLengthBounds checks that a length larger than the data behind it does +// not size an allocation before that data has arrived, while a length the data does +// satisfy still round-trips. The assertion is on allocation volume, not merely on +// getting an error: an overlong length errors either way once io.ReadFull runs out. func TestBinaryLengthBounds(t *testing.T) { p := &thrift.BinaryProtocol{} + measure := func(f func() error) (uint64, error) { + var before, after runtime.MemStats + runtime.GC() + runtime.ReadMemStats(&before) + err := f() + runtime.ReadMemStats(&after) + return after.TotalAlloc - before.TotalAlloc, err + } + // A non-strict message header claiming a 64MiB name, with no name behind it. hostile := make([]byte, 4) binary.BigEndian.PutUint32(hostile, 1<<26) hostile[0] &= 0x7f // clear the strict bit - if _, err := p.NewReader(bytes.NewReader(hostile)).ReadMessage(); err == nil { + alloc, err := measure(func() error { + _, err := p.NewReader(bytes.NewReader(hostile)).ReadMessage() + return err + }) + if err == nil { t.Fatal("expected an error for a name length with no data behind it") } + if alloc > 1<<20 { + t.Fatalf("reading a message that claims a 64MiB name allocated %d bytes", alloc) + } // ReadBytes goes through the same check. hostileBytes := make([]byte, 4) binary.BigEndian.PutUint32(hostileBytes, 1<<26) - if _, err := p.NewReader(bytes.NewReader(hostileBytes)).ReadBytes(); err == nil { + alloc, err = measure(func() error { + _, err := p.NewReader(bytes.NewReader(hostileBytes)).ReadBytes() + return err + }) + if err == nil { t.Fatal("expected an error for a byte length with no data behind it") } + if alloc > 1<<20 { + t.Fatalf("ReadBytes with a 64MiB length allocated %d bytes", alloc) + } // A well formed message must still parse. var good bytes.Buffer From b711278a405f8b4f681132e9d33df411a6ebb234 Mon Sep 17 00:00:00 2001 From: tzh476 Date: Sat, 29 Aug 2026 00:16:45 +0800 Subject: [PATCH 3/3] thrift: include the check itself, not only its test The previous push carried the allocation assertion but not the change it guards, so the test failed on the pushed commit. The check is committed here. Change-Id: I7b4ea64612f630b6e179cfcebf1368400eb6fff7 Signed-off-by: tzh476 --- thrift/binary.go | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/thrift/binary.go b/thrift/binary.go index 18d95d9a..962d8c65 100644 --- a/thrift/binary.go +++ b/thrift/binary.go @@ -108,9 +108,24 @@ func (r *binaryReader) ReadLength() (int, error) { if n > math.MaxInt32 { return 0, fmt.Errorf("length out of range: %d", n) } + if err := r.checkLength(int(n)); err != nil { + return 0, err + } return int(n), nil } +// checkLength rejects a length that the underlying reader cannot possibly +// satisfy. The length is read from the wire, and the range check above still +// admits values up to 2GiB, so without this a four byte header can make the +// caller allocate gigabytes before io.ReadFull discovers there is no data +// behind it. Readers that cannot report their remaining size are left alone. +func (r *binaryReader) checkLength(n int) error { + if lr, ok := r.r.(interface{ Len() int }); ok && n > lr.Len() { + return fmt.Errorf("length %d exceeds the %d bytes remaining", n, lr.Len()) + } + return nil +} + func (r *binaryReader) ReadMessage() (Message, error) { m := Message{} @@ -121,6 +136,9 @@ func (r *binaryReader) ReadMessage() (Message, error) { if (b[0] >> 7) == 0 { // non-strict n := int(binary.BigEndian.Uint32(b)) + if err := r.checkLength(n); err != nil { + return m, err + } s := make([]byte, n) _, err := io.ReadFull(r.r, s) if err != nil {