code review 86590043: bufio: fix potential endless loop in ReadByte

bufio: fix potential endless loop in ReadByte

Also: Simplify ReadSlice implementation and
ensure that it doesn't call fill() with a full
buffer (this caused a failure in net/textproto
TestLargeReadMIMEHeader because fill() wasn't able
to read more data).

Fixes issue 7745.

[gri, 2014-04-10 21:39:42]

Hello r@golang.org, bradfitz@golang.org (cc:
golang-codereviews@googlegroups.com),

I'd like you to review this change to
https://code.google.com/p/go/

[gri, 2014-04-10 21:41:54]

PS: This one passes all tests on my machine.


On Thu, Apr 10, 2014 at 2:39 PM, <gri@golang.org> wrote:

> Reviewers: r, bradfitz,
>
> Message:
> Hello r@golang.org, bradfitz@golang.org (cc:
> golang-codereviews@googlegroups.com),
>
> I'd like you to review this change to
> https://code.google.com/p/go/
>
>
> Description:
> bufio: fix potential endless loop in ReadByte
>
> Also: Simplify ReadSlice implementation and
> ensure that it doesn't call fill() with a full
> buffer (this caused a failure in net/textproto
> TestLargeReadMIMEHeader because fill() wasn't able
> to read more data).
>
> Fixes issue 7745.
>
> Please review this at https://codereview.appspot.com/86590043/
>
> Affected files (+59, -28 lines):
>   M src/pkg/bufio/bufio.go
>   M src/pkg/bufio/bufio_test.go
>
>
> Index: src/pkg/bufio/bufio.go
> ===================================================================
> --- a/src/pkg/bufio/bufio.go
> +++ b/src/pkg/bufio/bufio.go
> @@ -88,15 +88,22 @@
>                 b.r = 0
>         }
>
> -       // Read new data.
> -       n, err := b.rd.Read(b.buf[b.w:])
> -       if n < 0 {
> -               panic(errNegativeRead)
> +       // Read new data: try a limited number of times.
> +       for i := maxConsecutiveEmptyReads; i > 0; i-- {
> +               n, err := b.rd.Read(b.buf[b.w:])
> +               if n < 0 {
> +                       panic(errNegativeRead)
> +               }
> +               b.w += n
> +               if err != nil {
> +                       b.err = err
> +                       return
> +               }
> +               if n > 0 {
> +                       return
> +               }
>         }
> -       b.w += n
> -       if err != nil {
> -               b.err = err
> -       }
> +       b.err = io.ErrNoProgress
>  }
>
>  func (b *Reader) readErr() error {
> @@ -151,6 +158,9 @@
>                         // Large read, empty buffer.
>                         // Read directly into p to avoid copy.
>                         n, b.err = b.rd.Read(p)
> +                       if n < 0 {
> +                               panic(errNegativeRead)
> +                       }
>                         if n > 0 {
>                                 b.lastByte = int(p[n-1])
>                                 b.lastRuneSize = -1
> @@ -256,36 +266,28 @@
>  // ReadBytes or ReadString instead.
>  // ReadSlice returns err != nil if and only if line does not end in delim.
>  func (b *Reader) ReadSlice(delim byte) (line []byte, err error) {
> -       // Look in buffer.
> -       if i := bytes.IndexByte(b.buf[b.r:b.w], delim); i >= 0 {
> -               line1 := b.buf[b.r : b.r+i+1]
> -               b.r += i + 1
> -               return line1, nil
> -       }
> +       for {
> +               // Search buffer.
> +               if i := bytes.IndexByte(b.buf[b.r:b.w], delim); i >= 0 {
> +                       line := b.buf[b.r : b.r+i+1]
> +                       b.r += i + 1
> +                       return line, nil
> +               }
>
> -       // Read more into buffer, until buffer fills or we find delim.
> -       for {
> +               // Pending error?
>                 if b.err != nil {
>                         line := b.buf[b.r:b.w]
>                         b.r = b.w
>                         return line, b.readErr()
>                 }
>
> -               n := b.Buffered()
> -               b.fill()
> -
> -               // Search new part of buffer
> -               if i := bytes.IndexByte(b.buf[n:b.w], delim); i >= 0 {
> -                       line := b.buf[0 : n+i+1]
> -                       b.r = n + i + 1
> -                       return line, nil
> -               }
> -
> -               // Buffer is full?
> -               if b.Buffered() >= len(b.buf) {
> +               // Buffer full?
> +               if n := b.Buffered(); n >= len(b.buf) {
>                         b.r = b.w
>                         return b.buf, ErrBufferFull
>                 }
> +
> +               b.fill()
>         }
>  }
>
> Index: src/pkg/bufio/bufio_test.go
> ===================================================================
> --- a/src/pkg/bufio/bufio_test.go
> +++ b/src/pkg/bufio/bufio_test.go
> @@ -14,6 +14,7 @@
>         "strings"
>         "testing"
>         "testing/iotest"
> +       "time"
>         "unicode/utf8"
>  )
>
> @@ -174,6 +175,34 @@
>         }
>  }
>
> +type zeroReader struct{}
> +
> +func (zeroReader) Read(p []byte) (int, error) {
> +       return 0, nil
> +}
> +
> +func TestZeroReader(t *testing.T) {
> +       var z zeroReader
> +       r := NewReader(z)
> +
> +       c := make(chan error)
> +       go func() {
> +               _, err := r.ReadByte()
> +               c <- err
> +       }()
> +
> +       select {
> +       case err := <-c:
> +               if err == nil {
> +                       t.Error("error expected")
> +               } else if err != io.ErrNoProgress {
> +                       t.Error("unexpected error:", err)
> +               }
> +       case <-time.After(time.Second):
> +               t.Error("test timed out (endless loop in ReadByte?)")
> +       }
> +}
> +
>  // A StringReader delivers its data one string segment at a time via Read.
>  type StringReader struct {
>         data []string
>
>
>

[bradfitz, 2014-04-10 21:49:00]

Protip: run "go test -short std" before all "hg submits".  :-)





On Thu, Apr 10, 2014 at 2:41 PM, Robert Griesemer <gri@golang.org> wrote:

> PS: This one passes all tests on my machine.
>
>
> On Thu, Apr 10, 2014 at 2:39 PM, <gri@golang.org> wrote:
>
>> Reviewers: r, bradfitz,
>>
>> Message:
>> Hello r@golang.org, bradfitz@golang.org (cc:
>> golang-codereviews@googlegroups.com),
>>
>> I'd like you to review this change to
>> https://code.google.com/p/go/
>>
>>
>> Description:
>> bufio: fix potential endless loop in ReadByte
>>
>> Also: Simplify ReadSlice implementation and
>> ensure that it doesn't call fill() with a full
>> buffer (this caused a failure in net/textproto
>> TestLargeReadMIMEHeader because fill() wasn't able
>> to read more data).
>>
>> Fixes issue 7745.
>>
>> Please review this at https://codereview.appspot.com/86590043/
>>
>> Affected files (+59, -28 lines):
>>   M src/pkg/bufio/bufio.go
>>   M src/pkg/bufio/bufio_test.go
>>
>>
>> Index: src/pkg/bufio/bufio.go
>> ===================================================================
>> --- a/src/pkg/bufio/bufio.go
>> +++ b/src/pkg/bufio/bufio.go
>> @@ -88,15 +88,22 @@
>>                 b.r = 0
>>         }
>>
>> -       // Read new data.
>> -       n, err := b.rd.Read(b.buf[b.w:])
>> -       if n < 0 {
>> -               panic(errNegativeRead)
>> +       // Read new data: try a limited number of times.
>> +       for i := maxConsecutiveEmptyReads; i > 0; i-- {
>> +               n, err := b.rd.Read(b.buf[b.w:])
>> +               if n < 0 {
>> +                       panic(errNegativeRead)
>> +               }
>> +               b.w += n
>> +               if err != nil {
>> +                       b.err = err
>> +                       return
>> +               }
>> +               if n > 0 {
>> +                       return
>> +               }
>>         }
>> -       b.w += n
>> -       if err != nil {
>> -               b.err = err
>> -       }
>> +       b.err = io.ErrNoProgress
>>  }
>>
>>  func (b *Reader) readErr() error {
>> @@ -151,6 +158,9 @@
>>                         // Large read, empty buffer.
>>                         // Read directly into p to avoid copy.
>>                         n, b.err = b.rd.Read(p)
>> +                       if n < 0 {
>> +                               panic(errNegativeRead)
>> +                       }
>>                         if n > 0 {
>>                                 b.lastByte = int(p[n-1])
>>                                 b.lastRuneSize = -1
>> @@ -256,36 +266,28 @@
>>  // ReadBytes or ReadString instead.
>>  // ReadSlice returns err != nil if and only if line does not end in
>> delim.
>>  func (b *Reader) ReadSlice(delim byte) (line []byte, err error) {
>> -       // Look in buffer.
>> -       if i := bytes.IndexByte(b.buf[b.r:b.w], delim); i >= 0 {
>> -               line1 := b.buf[b.r : b.r+i+1]
>> -               b.r += i + 1
>> -               return line1, nil
>> -       }
>> +       for {
>> +               // Search buffer.
>> +               if i := bytes.IndexByte(b.buf[b.r:b.w], delim); i >= 0 {
>> +                       line := b.buf[b.r : b.r+i+1]
>> +                       b.r += i + 1
>> +                       return line, nil
>> +               }
>>
>> -       // Read more into buffer, until buffer fills or we find delim.
>> -       for {
>> +               // Pending error?
>>                 if b.err != nil {
>>                         line := b.buf[b.r:b.w]
>>                         b.r = b.w
>>                         return line, b.readErr()
>>                 }
>>
>> -               n := b.Buffered()
>> -               b.fill()
>> -
>> -               // Search new part of buffer
>> -               if i := bytes.IndexByte(b.buf[n:b.w], delim); i >= 0 {
>> -                       line := b.buf[0 : n+i+1]
>> -                       b.r = n + i + 1
>> -                       return line, nil
>> -               }
>> -
>> -               // Buffer is full?
>> -               if b.Buffered() >= len(b.buf) {
>> +               // Buffer full?
>> +               if n := b.Buffered(); n >= len(b.buf) {
>>                         b.r = b.w
>>                         return b.buf, ErrBufferFull
>>                 }
>> +
>> +               b.fill()
>>         }
>>  }
>>
>> Index: src/pkg/bufio/bufio_test.go
>> ===================================================================
>> --- a/src/pkg/bufio/bufio_test.go
>> +++ b/src/pkg/bufio/bufio_test.go
>> @@ -14,6 +14,7 @@
>>         "strings"
>>         "testing"
>>         "testing/iotest"
>> +       "time"
>>         "unicode/utf8"
>>  )
>>
>> @@ -174,6 +175,34 @@
>>         }
>>  }
>>
>> +type zeroReader struct{}
>> +
>> +func (zeroReader) Read(p []byte) (int, error) {
>> +       return 0, nil
>> +}
>> +
>> +func TestZeroReader(t *testing.T) {
>> +       var z zeroReader
>> +       r := NewReader(z)
>> +
>> +       c := make(chan error)
>> +       go func() {
>> +               _, err := r.ReadByte()
>> +               c <- err
>> +       }()
>> +
>> +       select {
>> +       case err := <-c:
>> +               if err == nil {
>> +                       t.Error("error expected")
>> +               } else if err != io.ErrNoProgress {
>> +                       t.Error("unexpected error:", err)
>> +               }
>> +       case <-time.After(time.Second):
>> +               t.Error("test timed out (endless loop in ReadByte?)")
>> +       }
>> +}
>> +
>>  // A StringReader delivers its data one string segment at a time via
>> Read.
>>  type StringReader struct {
>>         data []string
>>
>>
>>
>

[bradfitz, 2014-04-10 21:50:07]

LGTM

https://codereview.appspot.com/86590043/diff/40001/src/pkg/bufio/bufio.go
File src/pkg/bufio/bufio.go (right):

https://codereview.appspot.com/86590043/diff/40001/src/pkg/bufio/bufio.go#new...
src/pkg/bufio/bufio.go:93: n, err := b.rd.Read(b.buf[b.w:])
before this,

  if len(b.buf[b.w:]) == 0 {
     return or panic?
  }

Either go back to the old behavior in that case, or panic and find any other
dumb users of fill?

[gri, 2014-04-10 23:14:26]

https://codereview.appspot.com/86590043/diff/40001/src/pkg/bufio/bufio.go
File src/pkg/bufio/bufio.go (right):

https://codereview.appspot.com/86590043/diff/40001/src/pkg/bufio/bufio.go#new...
src/pkg/bufio/bufio.go:93: n, err := b.rd.Read(b.buf[b.w:])
On 2014/04/10 21:50:07, bradfitz wrote:
> before this,
> 
>   if len(b.buf[b.w:]) == 0 {
>      return or panic?
>   }
> 
> Either go back to the old behavior in that case, or panic and find any other
> dumb users of fill?

verified all call sites, added panic

[bradfitz, 2014-04-10 23:18:51]

ship it.

[gri, 2014-04-11 04:46:05]

*** Submitted as https://code.google.com/p/go/source/detail?r=8e553855d1a3 ***

bufio: fix potential endless loop in ReadByte

Also: Simplify ReadSlice implementation and
ensure that it doesn't call fill() with a full
buffer (this caused a failure in net/textproto
TestLargeReadMIMEHeader because fill() wasn't able
to read more data).

Fixes issue 7745.

LGTM=bradfitz
R=r, bradfitz
CC=golang-codereviews
https://codereview.appspot.com/86590043

[gobot, 2014-04-11 04:57:05]

This CL appears to have broken the freebsd-amd64 builder.
See http://build.golang.org/log/3e2ee12ab1ca1eca8cdb59c7c443ac6a3733c576