[go] encoding/gob: use saferio.SliceCap when decoding a slice

[Gopher Robot (Gerrit)]

Gopher Robot submitted this change.

View Change

1 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:

```
The name of the file: src/internal/saferio/io.go
Insertions: 1, Deletions: 1.

@@ -109,7 +109,7 @@
 //
 // A negative result means that the value is always too big.
 //
-// The element type is described by passing pointer to a value of that type.
+// The element type is described by passing a pointer to a value of that type.
 // This would ideally use generics, but this code is built with
 // the bootstrap compiler which need not support generics.
 // We use a pointer so that we can handle slices of interface type.
```

Approvals: Ian Lance Taylor: Looks good to me, but someone else must approve; Run TryBots; Automatically submit change Cherry Mui: Looks good to me, but someone else must approve Gopher Robot: TryBots succeeded Daniel Martí: Looks good to me, approved

encoding/gob: use saferio.SliceCap when decoding a slice

This avoids allocating an overly large slice for corrupt input.

Change the saferio.SliceCap function to take a pointer to the element type,
so that we can handle slices of interface types. This revealed that a
couple of existing calls were actually incorrect, passing the slice type
rather than the element type.

No test case because the problem can only happen for invalid data. Let
the fuzzer find cases like this.

Fixes #55338

Change-Id: I3c1724183cc275d4981379773b0b8faa01a9cbd2
Reviewed-on: https://go-review.googlesource.com/c/go/+/433296
Reviewed-by: Ian Lance Taylor <ia...@google.com>
Reviewed-by: Daniel Martí <mv...@mvdan.cc>
Reviewed-by: Cherry Mui <cher...@google.com>
TryBot-Result: Gopher Robot <go...@golang.org>
Run-TryBot: Ian Lance Taylor <ia...@google.com>
Auto-Submit: Ian Lance Taylor <ia...@google.com>
---
M src/debug/macho/fat.go
M src/debug/macho/file.go
M src/debug/pe/symbol.go
M src/encoding/gob/decode.go
M src/internal/saferio/io.go
M src/internal/saferio/io_test.go
6 files changed, 60 insertions(+), 10 deletions(-)

diff --git a/src/debug/macho/fat.go b/src/debug/macho/fat.go
index 7dc03fa..679cefb 100644
--- a/src/debug/macho/fat.go
+++ b/src/debug/macho/fat.go
@@ -86,7 +86,7 @@
 
 	// Following the fat_header comes narch fat_arch structs that index
 	// Mach-O images further in the file.
-	c := saferio.SliceCap(FatArch{}, uint64(narch))
+	c := saferio.SliceCap((*FatArch)(nil), uint64(narch))
 	if c < 0 {
 		return nil, &FormatError{offset, "too many images", nil}
 	}
diff --git a/src/debug/macho/file.go b/src/debug/macho/file.go
index 3c95803..0c6488d 100644
--- a/src/debug/macho/file.go
+++ b/src/debug/macho/file.go
@@ -253,7 +253,7 @@
 	if err != nil {
 		return nil, err
 	}
-	c := saferio.SliceCap([]Load{}, uint64(f.Ncmd))
+	c := saferio.SliceCap((*Load)(nil), uint64(f.Ncmd))
 	if c < 0 {
 		return nil, &FormatError{offset, "too many load commands", nil}
 	}
@@ -460,7 +460,7 @@
 
 func (f *File) parseSymtab(symdat, strtab, cmddat []byte, hdr *SymtabCmd, offset int64) (*Symtab, error) {
 	bo := f.ByteOrder
-	c := saferio.SliceCap([]Symbol{}, uint64(hdr.Nsyms))
+	c := saferio.SliceCap((*Symbol)(nil), uint64(hdr.Nsyms))
 	if c < 0 {
 		return nil, &FormatError{offset, "too many symbols", nil}
 	}
diff --git a/src/debug/pe/symbol.go b/src/debug/pe/symbol.go
index 0a5343f..b1654f8 100644
--- a/src/debug/pe/symbol.go
+++ b/src/debug/pe/symbol.go
@@ -59,7 +59,7 @@
 	if err != nil {
 		return nil, fmt.Errorf("fail to seek to symbol table: %v", err)
 	}
-	c := saferio.SliceCap(COFFSymbol{}, uint64(fh.NumberOfSymbols))
+	c := saferio.SliceCap((*COFFSymbol)(nil), uint64(fh.NumberOfSymbols))
 	if c < 0 {
 		return nil, errors.New("too many symbols; file may be corrupt")
 	}
diff --git a/src/encoding/gob/decode.go b/src/encoding/gob/decode.go
index 470e357..480832c 100644
--- a/src/encoding/gob/decode.go
+++ b/src/encoding/gob/decode.go
@@ -9,6 +9,7 @@
 import (
 	"encoding"
 	"errors"
+	"internal/saferio"
 	"io"
 	"math"
 	"math/bits"
@@ -514,10 +515,22 @@
 	}
 	instr := &decInstr{elemOp, 0, nil, ovfl}
 	isPtr := value.Type().Elem().Kind() == reflect.Pointer
+	ln := value.Len()
 	for i := 0; i < length; i++ {
 		if state.b.Len() == 0 {
 			errorf("decoding array or slice: length exceeds input size (%d elements)", length)
 		}
+		if i >= ln {
+			// This is a slice that we only partially allocated.
+			// Grow it using append, up to length.
+			value = reflect.Append(value, reflect.Zero(value.Type().Elem()))
+			cp := value.Cap()
+			if cp > length {
+				cp = length
+			}
+			value.SetLen(cp)
+			ln = cp
+		}
 		v := value.Index(i)
 		if isPtr {
 			v = decAlloc(v)
@@ -618,7 +631,11 @@
 		errorf("%s slice too big: %d elements of %d bytes", typ.Elem(), u, size)
 	}
 	if value.Cap() < n {
-		value.Set(reflect.MakeSlice(typ, n, n))
+		safe := saferio.SliceCap(reflect.Zero(reflect.PtrTo(typ.Elem())).Interface(), uint64(n))
+		if safe < 0 {
+			errorf("%s slice too big: %d elements of %d bytes", typ.Elem(), u, size)
+		}
+		value.Set(reflect.MakeSlice(typ, safe, safe))
 	} else {
 		value.SetLen(n)
 	}
diff --git a/src/internal/saferio/io.go b/src/internal/saferio/io.go
index 8fb27b0..b10d117 100644
--- a/src/internal/saferio/io.go
+++ b/src/internal/saferio/io.go
@@ -109,14 +109,19 @@
 //
 // A negative result means that the value is always too big.
 //
-// The element type is described by passing a value of that type.
+// The element type is described by passing a pointer to a value of that type.
 // This would ideally use generics, but this code is built with
 // the bootstrap compiler which need not support generics.
+// We use a pointer so that we can handle slices of interface type.
 func SliceCap(v any, c uint64) int {
 	if int64(c) < 0 || c != uint64(int(c)) {
 		return -1
 	}
-	size := reflect.TypeOf(v).Size()
+	typ := reflect.TypeOf(v)
+	if typ.Kind() != reflect.Ptr {
+		panic("SliceCap called with non-pointer type")
+	}
+	size := typ.Elem().Size()
 	if uintptr(c)*size > chunk {
 		c = uint64(chunk / size)
 		if c == 0 {
diff --git a/src/internal/saferio/io_test.go b/src/internal/saferio/io_test.go
index 1a7d3e1..290181f 100644
--- a/src/internal/saferio/io_test.go
+++ b/src/internal/saferio/io_test.go
@@ -105,14 +105,14 @@
 
 func TestSliceCap(t *testing.T) {
 	t.Run("small", func(t *testing.T) {
-		c := SliceCap(0, 10)
+		c := SliceCap((*int)(nil), 10)
 		if c != 10 {
 			t.Errorf("got capacity %d, want %d", c, 10)
 		}
 	})
 
 	t.Run("large", func(t *testing.T) {
-		c := SliceCap(byte(0), 1<<30)
+		c := SliceCap((*byte)(nil), 1<<30)
 		if c < 0 {
 			t.Error("SliceCap failed unexpectedly")
 		} else if c == 1<<30 {
@@ -121,7 +121,7 @@
 	})
 
 	t.Run("maxint", func(t *testing.T) {
-		c := SliceCap(byte(0), 1<<63)
+		c := SliceCap((*byte)(nil), 1<<63)
 		if c >= 0 {
 			t.Errorf("SliceCap returned %d, expected failure", c)
 		}

To view, visit change 433296. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-Project: go

Gerrit-Branch: master

Gerrit-Change-Id: I3c1724183cc275d4981379773b0b8faa01a9cbd2

Gerrit-Change-Number: 433296

Gerrit-PatchSet: 3

Gerrit-Owner: Ian Lance Taylor <ia...@golang.org>

Gerrit-Reviewer: Cherry Mui <cher...@google.com>

Gerrit-Reviewer: Daniel Martí <mv...@mvdan.cc>

Gerrit-Reviewer: Gopher Robot <go...@golang.org>

Gerrit-Reviewer: Ian Lance Taylor <ia...@golang.org>

Gerrit-Reviewer: Ian Lance Taylor <ia...@google.com>

Gerrit-MessageType: merged