strings: fix two Builder bugs allowing mutation of strings, remove ReadFrom

The Builder's ReadFrom method allows the underlying unsafe slice to
escape, and for callers to subsequently modify memory that had been
unsafely converted into an immutable string.

In the original proposal for Builder (#18990), I'd noted there should
be no Read methods:

> There would be no Reset or Bytes or Truncate or Read methods.
> Nothing that could mutate the []byte once it was unsafely converted
> to a string.

And in my prototype (https://golang.org/cl/37767), I handled ReadFrom
properly, but when https://golang.org/cl/74931 arrived, I missed that
it had a ReadFrom method and approved it.

Because we're so close to the Go 1.10 release, just remove the
ReadFrom method rather than think about possible fixes. It has
marginal utility in a Builder anyway.

Also, fix a separate bug that also allowed mutation of a slice's
backing array after it had been converted into a slice by disallowing
copies of the Builder by value.

Updates #18990
Fixes #23083
Fixes #23084

Change-Id: Id1f860f8a4f5f88b32213cf85108ebc609acb95f
Reviewed-on: https://go-review.googlesource.com/83255
Reviewed-by: Keith Randall <[email protected]>
Reviewed-by: Ian Lance Taylor <[email protected]>

Author: bradfitz

api/go1.10.txt

@@ -594,7 +594,6 @@ pkg os, method (*SyscallError) Timeout() bool
 pkg os, var ErrNoDeadline error
 pkg strings, method (*Builder) Grow(int)
 pkg strings, method (*Builder) Len() int
-pkg strings, method (*Builder) ReadFrom(io.Reader) (int64, error)
 pkg strings, method (*Builder) Reset()
 pkg strings, method (*Builder) String() string
 pkg strings, method (*Builder) Write([]uint8) (int, error)

src/strings/builder.go

@@ -5,16 +5,24 @@
 package strings
 
 import (
-	"errors"
-	"io"
 	"unicode/utf8"
 	"unsafe"
 )
 
 // A Builder is used to efficiently build a string using Write methods.
 // It minimizes memory copying. The zero value is ready to use.
+// Do not copy a non-zero Builder.
 type Builder struct {
-	buf []byte
+	addr *Builder // of receiver, to detect copies by value
+	buf  []byte
+}
+
+func (b *Builder) copyCheck() {
+	if b.addr == nil {
+		b.addr = b
+	} else if b.addr != b {
+		panic("strings: illegal use of non-zero Builder copied by value")
+	}
 }
 
 // String returns the accumulated string.
@@ -26,7 +34,10 @@ func (b *Builder) String() string {
 func (b *Builder) Len() int { return len(b.buf) }
 
 // Reset resets the Builder to be empty.
-func (b *Builder) Reset() { b.buf = nil }
+func (b *Builder) Reset() {
+	b.addr = nil
+	b.buf = nil
+}
 
 // grow copies the buffer to a new, larger buffer so that there are at least n
 // bytes of capacity beyond len(b.buf).
@@ -40,6 +51,7 @@ func (b *Builder) grow(n int) {
 // another n bytes. After Grow(n), at least n bytes can be written to b
 // without another allocation. If n is negative, Grow panics.
 func (b *Builder) Grow(n int) {
+	b.copyCheck()
 	if n < 0 {
 		panic("strings.Builder.Grow: negative count")
 	}
@@ -51,20 +63,23 @@ func (b *Builder) Grow(n int) {
 // Write appends the contents of p to b's buffer.
 // Write always returns len(p), nil.
 func (b *Builder) Write(p []byte) (int, error) {
+	b.copyCheck()
 	b.buf = append(b.buf, p...)
 	return len(p), nil
 }
 
 // WriteByte appends the byte c to b's buffer.
 // The returned error is always nil.
 func (b *Builder) WriteByte(c byte) error {
+	b.copyCheck()
 	b.buf = append(b.buf, c)
 	return nil
 }
 
 // WriteRune appends the UTF-8 encoding of Unicode code point r to b's buffer.
 // It returns the length of r and a nil error.
 func (b *Builder) WriteRune(r rune) (int, error) {
+	b.copyCheck()
 	if r < utf8.RuneSelf {
 		b.buf = append(b.buf, byte(r))
 		return 1, nil
@@ -81,38 +96,7 @@ func (b *Builder) WriteRune(r rune) (int, error) {
 // WriteString appends the contents of s to b's buffer.
 // It returns the length of s and a nil error.
 func (b *Builder) WriteString(s string) (int, error) {
+	b.copyCheck()
 	b.buf = append(b.buf, s...)
 	return len(s), nil
 }
-
-// minRead is the minimum slice passed to a Read call by Builder.ReadFrom.
-// It is the same as bytes.MinRead.
-const minRead = 512
-
-// errNegativeRead is the panic value if the reader passed to Builder.ReadFrom
-// returns a negative count.
-var errNegativeRead = errors.New("strings.Builder: reader returned negative count from Read")
-
-// ReadFrom reads data from r until EOF and appends it to b's buffer.
-// The return value n is the number of bytes read.
-// Any error except io.EOF encountered during the read is also returned.
-func (b *Builder) ReadFrom(r io.Reader) (n int64, err error) {
-	for {
-		l := len(b.buf)
-		if cap(b.buf)-l < minRead {
-			b.grow(minRead)
-		}
-		m, e := r.Read(b.buf[l:cap(b.buf)])
-		if m < 0 {
-			panic(errNegativeRead)
-		}
-		b.buf = b.buf[:l+m]
-		n += int64(m)
-		if e == io.EOF {
-			return n, nil
-		}
-		if e != nil {
-			return n, e
-		}
-	}
-}

src/strings/builder_test.go

@@ -6,12 +6,9 @@ package strings_test
 
 import (
 	"bytes"
-	"errors"
-	"io"
 	"runtime"
 	. "strings"
 	"testing"
-	"testing/iotest"
 )
 
 func check(t *testing.T, b *Builder, want string) {
@@ -169,93 +166,6 @@ func TestBuilderWriteByte(t *testing.T) {
 	check(t, &b, "a\x00")
 }
 
-func TestBuilderReadFrom(t *testing.T) {
-	for _, tt := range []struct {
-		name string
-		fn   func(io.Reader) io.Reader
-	}{
-		{"Reader", func(r io.Reader) io.Reader { return r }},
-		{"DataErrReader", iotest.DataErrReader},
-		{"OneByteReader", iotest.OneByteReader},
-	} {
-		t.Run(tt.name, func(t *testing.T) {
-			var b Builder
-
-			r := tt.fn(NewReader("hello"))
-			n, err := b.ReadFrom(r)
-			if err != nil {
-				t.Fatalf("first call: got %s", err)
-			}
-			if n != 5 {
-				t.Errorf("first call: got n=%d; want 5", n)
-			}
-			check(t, &b, "hello")
-
-			r = tt.fn(NewReader(" world"))
-			n, err = b.ReadFrom(r)
-			if err != nil {
-				t.Fatalf("first call: got %s", err)
-			}
-			if n != 6 {
-				t.Errorf("first call: got n=%d; want 6", n)
-			}
-			check(t, &b, "hello world")
-		})
-	}
-}
-
-var errRead = errors.New("boom")
-
-// errorReader sends reads to the underlying reader
-// but returns errRead instead of io.EOF.
-type errorReader struct {
-	r io.Reader
-}
-
-func (r errorReader) Read(b []byte) (int, error) {
-	n, err := r.r.Read(b)
-	if err == io.EOF {
-		err = errRead
-	}
-	return n, err
-}
-
-func TestBuilderReadFromError(t *testing.T) {
-	var b Builder
-	r := errorReader{NewReader("hello")}
-	n, err := b.ReadFrom(r)
-	if n != 5 {
-		t.Errorf("got n=%d; want 5", n)
-	}
-	if err != errRead {
-		t.Errorf("got err=%q; want %q", err, errRead)
-	}
-	check(t, &b, "hello")
-}
-
-type negativeReader struct{}
-
-func (r negativeReader) Read([]byte) (int, error) { return -1, nil }
-
-func TestBuilderReadFromNegativeReader(t *testing.T) {
-	var b Builder
-	defer func() {
-		switch err := recover().(type) {
-		case nil:
-			t.Fatal("ReadFrom didn't panic")
-		case error:
-			wantErr := "strings.Builder: reader returned negative count from Read"
-			if err.Error() != wantErr {
-				t.Fatalf("recovered panic: got %v; want %v", err.Error(), wantErr)
-			}
-		default:
-			t.Fatalf("unexpected panic value: %#v", err)
-		}
-	}()
-
-	b.ReadFrom(negativeReader{})
-}
-
 func TestBuilderAllocs(t *testing.T) {
 	var b Builder
 	b.Grow(5)
@@ -280,3 +190,103 @@ func numAllocs(fn func()) uint64 {
 	runtime.ReadMemStats(&m2)
 	return m2.Mallocs - m1.Mallocs
 }
+
+func TestBuilderCopyPanic(t *testing.T) {
+	tests := []struct {
+		name      string
+		fn        func()
+		wantPanic bool
+	}{
+		{
+			name:      "String",
+			wantPanic: false,
+			fn: func() {
+				var a Builder
+				a.WriteByte('x')
+				b := a
+				_ = b.String() // appease vet
+			},
+		},
+		{
+			name:      "Len",
+			wantPanic: false,
+			fn: func() {
+				var a Builder
+				a.WriteByte('x')
+				b := a
+				b.Len()
+			},
+		},
+		{
+			name:      "Reset",
+			wantPanic: false,
+			fn: func() {
+				var a Builder
+				a.WriteByte('x')
+				b := a
+				b.Reset()
+				b.WriteByte('y')
+			},
+		},
+		{
+			name:      "Write",
+			wantPanic: true,
+			fn: func() {
+				var a Builder
+				a.Write([]byte("x"))
+				b := a
+				b.Write([]byte("y"))
+			},
+		},
+		{
+			name:      "WriteByte",
+			wantPanic: true,
+			fn: func() {
+				var a Builder
+				a.WriteByte('x')
+				b := a
+				b.WriteByte('y')
+			},
+		},
+		{
+			name:      "WriteString",
+			wantPanic: true,
+			fn: func() {
+				var a Builder
+				a.WriteString("x")
+				b := a
+				b.WriteString("y")
+			},
+		},
+		{
+			name:      "WriteRune",
+			wantPanic: true,
+			fn: func() {
+				var a Builder
+				a.WriteRune('x')
+				b := a
+				b.WriteRune('y')
+			},
+		},
+		{
+			name:      "Grow",
+			wantPanic: true,
+			fn: func() {
+				var a Builder
+				a.Grow(1)
+				b := a
+				b.Grow(2)
+			},
+		},
+	}
+	for _, tt := range tests {
+		didPanic := make(chan bool)
+		go func() {
+			defer func() { didPanic <- recover() != nil }()
+			tt.fn()
+		}()
+		if got := <-didPanic; got != tt.wantPanic {
+			t.Errorf("%s: panicked = %v; want %v", tt.name, got, tt.wantPanic)
+		}
+	}
+}