[go] bufio: reject UnreadByte or UnreadRune after a Discard or WriteTo

8 views
Skip to first unread message

Bryan C. Mills (Gerrit)

unread,
Oct 1, 2021, 1:40:55 PM10/1/21
to Bryan C. Mills, goph...@pubsubhelper.golang.org, golang-...@googlegroups.com, Go Bot, Joe Tsai, golang-co...@googlegroups.com

Bryan C. Mills 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/bufio/bufio_test.go
Insertions: 24, Deletions: 64.

@@ -304,77 +304,37 @@
}
}

-func TestUnreadByteAfterDiscard(t *testing.T) {
- check := func(err error) {
- if err != nil {
- t.Helper()
- t.Fatal(err)
- }
- }
-
- const in = "abc\n"
- t.Logf("reading %q", in)
-
- b := NewReader(strings.NewReader(in))
-
- c, err := b.ReadByte()
- check(err)
- t.Logf("read byte %q", c)
-
- n, err := b.Discard(1)
- check(err)
- t.Logf("discarded %v byte(s)", n)
-
- err = b.UnreadByte()
- t.Logf("UnreadByte: %v", err)
- var want string
- if err == nil {
- want = in[1:]
- } else {
- want = in[2:]
- }
-
- s, err := b.ReadString('\n')
- check(err)
- if s != want {
- t.Fatalf("buffer corrupted: read %q, expected %q", s, want)
+func TestNoUnreadRuneAfterDiscard(t *testing.T) {
+ br := NewReader(strings.NewReader("example"))
+ br.ReadRune()
+ br.Discard(1)
+ if err := br.UnreadRune(); err == nil {
+ t.Error("UnreadRune didn't fail after Discard")
}
}

-func TestUnreadByteAfterWriteTo(t *testing.T) {
- check := func(err error) {
- if err != nil {
- t.Helper()
- t.Fatal(err)
- }
+func TestNoUnreadByteAfterDiscard(t *testing.T) {
+ br := NewReader(strings.NewReader("example"))
+ br.ReadByte()
+ br.Discard(1)
+ if err := br.UnreadByte(); err == nil {
+ t.Error("UnreadByte didn't fail after Discard")
}
+}

- const in = "abcdef"
- buf := bytes.NewBuffer(nil)
- out := bytes.NewBuffer(nil)
- want := new(strings.Builder)
-
- b := NewReader(buf)
- buf.WriteString(in[:3])
- want.WriteString(in[:3])
-
- c, err := b.ReadByte()
- check(err)
- out.Write([]byte{c})
- _, err = b.WriteTo(out)
- check(err)
-
- if err := b.UnreadByte(); err == nil {
- want.WriteString(in[2:3])
+func TestNoUnreadRuneAfterWriteTo(t *testing.T) {
+ br := NewReader(strings.NewReader("example"))
+ br.WriteTo(io.Discard)
+ if err := br.UnreadRune(); err == nil {
+ t.Error("UnreadRune didn't fail after WriteTo")
}
+}

- buf.WriteString(in[3:])
- want.WriteString(in[3:])
- _, err = b.WriteTo(out)
- check(err)
-
- if out.String() != want.String() {
- t.Fatalf("copied %q, but input was %q", out, want)
+func TestNoUnreadByteAfterWriteTo(t *testing.T) {
+ br := NewReader(strings.NewReader("example"))
+ br.WriteTo(io.Discard)
+ if err := br.UnreadByte(); err == nil {
+ t.Error("UnreadByte didn't fail after WriteTo")
}
}

```
```
The name of the file: src/bufio/bufio.go
Insertions: 2, Deletions: 2.

@@ -270,8 +270,8 @@
// UnreadByte unreads the last byte. Only the most recently read byte can be unread.
//
// UnreadByte returns an error if the most recent method called on the
-// Reader was not a read operation. Notably, Peek is not considered a
-// read operation.
+// Reader was not a read operation. Notably, Peek, Discard, and WriteTo are not
+// considered read operations.
func (b *Reader) UnreadByte() error {
if b.lastByte < 0 || b.r == 0 && b.w > 0 {
return ErrInvalidUnreadByte
```

Approvals: Joe Tsai: Looks good to me, approved; Trusted Bryan C. Mills: Trusted
bufio: reject UnreadByte or UnreadRune after a Discard or WriteTo

Discard is not really a read operation, and in theory it could
Seek the underlying Reader without actually reading anything,
so an UnreadByte following a Discard is disallowed.

Similarly, although WriteTo usually does end up calling Read on the
underlying buffer, if the underlying Reader implements io.WriterTo it
may instead terminate in a call to WriteTo, without ever buffering or
even seeing the last byte written. (It is conceptually read-like, but
not strictly “a read operation”.)

Fixes #48446

Change-Id: Ide6f2b157332b423486810399f66140c914144e5
Reviewed-on: https://go-review.googlesource.com/c/go/+/351810
Trust: Bryan C. Mills <bcm...@google.com>
Trust: Joe Tsai <joe...@digital-static.net>
Reviewed-by: Joe Tsai <joe...@digital-static.net>
---
M src/bufio/bufio_test.go
M src/bufio/bufio.go
2 files changed, 68 insertions(+), 2 deletions(-)

diff --git a/src/bufio/bufio.go b/src/bufio/bufio.go
index 506b84f..a58df25 100644
--- a/src/bufio/bufio.go
+++ b/src/bufio/bufio.go
@@ -173,6 +173,10 @@
if n == 0 {
return
}
+
+ b.lastByte = -1
+ b.lastRuneSize = -1
+
remain := n
for {
skip := b.Buffered()
@@ -266,8 +270,8 @@
// UnreadByte unreads the last byte. Only the most recently read byte can be unread.
//
// UnreadByte returns an error if the most recent method called on the
-// Reader was not a read operation. Notably, Peek is not considered a
-// read operation.
+// Reader was not a read operation. Notably, Peek, Discard, and WriteTo are not
+// considered read operations.
func (b *Reader) UnreadByte() error {
if b.lastByte < 0 || b.r == 0 && b.w > 0 {
return ErrInvalidUnreadByte
@@ -502,6 +506,9 @@
// If the underlying reader supports the WriteTo method,
// this calls the underlying WriteTo without buffering.
func (b *Reader) WriteTo(w io.Writer) (n int64, err error) {
+ b.lastByte = -1
+ b.lastRuneSize = -1
+
n, err = b.writeBuf(w)
if err != nil {
return
diff --git a/src/bufio/bufio_test.go b/src/bufio/bufio_test.go
index 04a810c..8e8a8a1 100644
--- a/src/bufio/bufio_test.go
+++ b/src/bufio/bufio_test.go
@@ -304,6 +304,40 @@
}
}

+func TestNoUnreadRuneAfterDiscard(t *testing.T) {
+ br := NewReader(strings.NewReader("example"))
+ br.ReadRune()
+ br.Discard(1)
+ if err := br.UnreadRune(); err == nil {
+ t.Error("UnreadRune didn't fail after Discard")
+ }
+}
+
+func TestNoUnreadByteAfterDiscard(t *testing.T) {
+ br := NewReader(strings.NewReader("example"))
+ br.ReadByte()
+ br.Discard(1)
+ if err := br.UnreadByte(); err == nil {
+ t.Error("UnreadByte didn't fail after Discard")
+ }
+}
+
+func TestNoUnreadRuneAfterWriteTo(t *testing.T) {
+ br := NewReader(strings.NewReader("example"))
+ br.WriteTo(io.Discard)
+ if err := br.UnreadRune(); err == nil {
+ t.Error("UnreadRune didn't fail after WriteTo")
+ }
+}
+
+func TestNoUnreadByteAfterWriteTo(t *testing.T) {
+ br := NewReader(strings.NewReader("example"))
+ br.WriteTo(io.Discard)
+ if err := br.UnreadByte(); err == nil {
+ t.Error("UnreadByte didn't fail after WriteTo")
+ }
+}
+
func TestUnreadByte(t *testing.T) {
segments := []string{"Hello, ", "world"}
r := NewReader(&StringReader{data: segments})

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

Gerrit-Project: go
Gerrit-Branch: master
Gerrit-Change-Id: Ide6f2b157332b423486810399f66140c914144e5
Gerrit-Change-Number: 351810
Gerrit-PatchSet: 7
Gerrit-Owner: Bryan C. Mills <bcm...@google.com>
Gerrit-Reviewer: Bryan C. Mills <bcm...@google.com>
Gerrit-Reviewer: Go Bot <go...@golang.org>
Gerrit-Reviewer: Joe Tsai <joe...@digital-static.net>
Gerrit-MessageType: merged
Reply all
Reply to author
Forward
0 new messages