encoding/binary: limit bytes read by Uvarint to <= 10

Limits the number of bytes that can be consumed by Uvarint
to MaxVarintLen64 (10) to avoid wasted computations.
With this change, if Uvarint reads more than MaxVarintLen64
bytes, it'll return the erroring byte count of n=-(MaxVarintLen64+1)
which is -11, as per the function signature.

Updated some tests to reflect the new change in expectations of n
when the number of bytes to be read exceeds the limits..

Fixes #41185

Change-Id: Ie346457b1ddb0214b60c72e81128e24d604d083d
Reviewed-on: https://go-review.googlesource.com/c/go/+/299531
Run-TryBot: Emmanuel Odeke <emmanuel@orijtech.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>
Trust: Emmanuel Odeke <emmanuel@orijtech.com>

diff --git a/src/encoding/binary/varint.go b/src/encoding/binary/varint.go
index 1fa325dec7fd944ad650788533dd3236047a05f2..8fe20b5c45d324f34ccf72752849c433a9bc2223 100644 (file)
--- a/src/encoding/binary/varint.go
+++ b/src/encoding/binary/varint.go
@@ -61,8 +61,13 @@ func Uvarint(buf []byte) (uint64, int) {
        var x uint64
        var s uint
        for i, b := range buf {
+               if i == MaxVarintLen64 {
+                       // Catch byte reads past MaxVarintLen64.
+                       // See issue https://golang.org/issues/41185
+                       return 0, -(i + 1) // overflow
+               }
                if b < 0x80 {
-                       if i >= MaxVarintLen64 || i == MaxVarintLen64-1 && b > 1 {
+                       if i == MaxVarintLen64-1 && b > 1 {
                                return 0, -(i + 1) // overflow
                        }
                        return x | uint64(b)<<s, i + 1

diff --git a/src/encoding/binary/varint_test.go b/src/encoding/binary/varint_test.go
index 6ef4c9950525a45c56eb7a11c5bb2ca443693b09..d025a67538ccdb66e42834d9540437fbb87caa14 100644 (file)
--- a/src/encoding/binary/varint_test.go
+++ b/src/encoding/binary/varint_test.go
@@ -7,6 +7,7 @@ package binary
 import (
        "bytes"
        "io"
+       "math"
        "testing"
 )
 
@@ -121,10 +122,66 @@ func TestBufferTooSmall(t *testing.T) {
        }
 }
 
+// Ensure that we catch overflows of bytes going past MaxVarintLen64.
+// See issue https://golang.org/issues/41185
+func TestBufferTooBigWithOverflow(t *testing.T) {
+       tests := []struct {
+               in        []byte
+               name      string
+               wantN     int
+               wantValue uint64
+       }{
+               {
+                       name: "invalid: 1000 bytes",
+                       in: func() []byte {
+                               b := make([]byte, 1000)
+                               for i := range b {
+                                       b[i] = 0xff
+                               }
+                               b[999] = 0
+                               return b
+                       }(),
+                       wantN:     -11,
+                       wantValue: 0,
+               },
+               {
+                       name:      "valid: math.MaxUint64-40",
+                       in:        []byte{0xd7, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x01},
+                       wantValue: math.MaxUint64 - 40,
+                       wantN:     10,
+               },
+               {
+                       name:      "invalid: with more than MaxVarintLen64 bytes",
+                       in:        []byte{0xd7, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x01},
+                       wantN:     -11,
+                       wantValue: 0,
+               },
+               {
+                       name:      "invalid: 10th byte",
+                       in:        []byte{0xd7, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x7f},
+                       wantN:     -10,
+                       wantValue: 0,
+               },
+       }
+
+       for _, tt := range tests {
+               tt := tt
+               t.Run(tt.name, func(t *testing.T) {
+                       value, n := Uvarint(tt.in)
+                       if g, w := n, tt.wantN; g != w {
+                               t.Errorf("bytes returned=%d, want=%d", g, w)
+                       }
+                       if g, w := value, tt.wantValue; g != w {
+                               t.Errorf("value=%d, want=%d", g, w)
+                       }
+               })
+       }
+}
+
 func testOverflow(t *testing.T, buf []byte, x0 uint64, n0 int, err0 error) {
        x, n := Uvarint(buf)
        if x != 0 || n != n0 {
-               t.Errorf("Uvarint(%v): got x = %d, n = %d; want 0, %d", buf, x, n, n0)
+               t.Errorf("Uvarint(% X): got x = %d, n = %d; want 0, %d", buf, x, n, n0)
        }
 
        r := bytes.NewReader(buf)
@@ -140,8 +197,8 @@ func testOverflow(t *testing.T, buf []byte, x0 uint64, n0 int, err0 error) {
 
 func TestOverflow(t *testing.T) {
        testOverflow(t, []byte{0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 0x2}, 0, -10, overflow)
-       testOverflow(t, []byte{0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 0x1, 0, 0}, 0, -13, overflow)
-       testOverflow(t, []byte{0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF}, 1<<64-1, 0, overflow) // 11 bytes, should overflow
+       testOverflow(t, []byte{0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 0x1, 0, 0}, 0, -11, overflow)
+       testOverflow(t, []byte{0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF}, 1<<64-1, -11, overflow) // 11 bytes, should overflow
 }
 
 func TestNonCanonicalZero(t *testing.T) {