From 3058d38632aea679c96cd41156b2751c97578a2d Mon Sep 17 00:00:00 2001 From: Brad Fitzpatrick Date: Mon, 11 Dec 2017 15:41:24 +0000 Subject: [PATCH] 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 Reviewed-by: Ian Lance Taylor --- api/go1.10.txt | 1 - src/strings/builder.go | 56 ++++------- src/strings/builder_test.go | 190 +++++++++++++++++++----------------- 3 files changed, 120 insertions(+), 127 deletions(-) diff --git a/api/go1.10.txt b/api/go1.10.txt index c8e504c992..6647ec66dc 100644 --- a/api/go1.10.txt +++ b/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) diff --git a/src/strings/builder.go b/src/strings/builder.go index 09ebb3d91b..11bcee1dfc 100644 --- a/src/strings/builder.go +++ b/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,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 - } - } -} diff --git a/src/strings/builder_test.go b/src/strings/builder_test.go index df557082a7..c0c8fa4130 100644 --- a/src/strings/builder_test.go +++ b/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) + } + } +} -- 2.50.0