Skip to content

G
golang
Commit 3058d386 authored by Brad Fitzpatrick's avatar Brad Fitzpatrick
Browse files
Options

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/83255Reviewed-by: 's avatarKeith Randall <khr@golang.org>
Reviewed-by: 's avatarIan Lance Taylor <iant@golang.org>
parent 29be20a1
Hide whitespace changes
Inline Side-by-side
Showing with 120 additions and 127 deletions
api/go1.10.txt
View file @ 3058d386
......@@ -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
View file @ 3058d386
......@@ -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,6 +63,7 @@ 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
}
......@@ -58,6 +71,7 @@ func (b *Builder) Write(p []byte) (int, error) {
// 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
}
......@@ -65,6 +79,7 @@ func (b *Builder) WriteByte(c byte) error {
// 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
View file @ 3058d386
......@@ -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)
}
}
}
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment