Go: bytes: have buffer.ReadFrom panic with better message with negative Read count

[superstas]

What version of Go are you using (go version)?

go version go1.9 linux/amd64

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

GOARCH="amd64"
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GORACE=""
GOROOT="/usr/lib/go-1.9"
GOTOOLDIR="/usr/lib/go-1.9/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build517552475=/tmp/go-build -gno-record-gcc-switches"
CXX="g++"
CGO_ENABLED="1"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"

What did you do?

I'm trying to read a bad io.Reader implementation using ioutil.ReadAll
https://play.golang.org/p/EGy1gU3s2U

What did you expect to see?

A human-readable error message, like in a case of bufio.Reader with the same io.Reader implementation
https://play.golang.org/p/AKl7_zzjhx

panic: bufio: reader returned negative count from Read

What did you see instead?

panic: runtime error: slice bounds out of range [recovered]
    panic: runtime error: slice bounds out of range

goroutine 1 [running]:
io/ioutil.readAll.func1(0x1042bf74, 0x0)
    /usr/local/go/src/io/ioutil/ioutil.go:30 +0x180
panic(0xcfb20, 0x15ba18)
    /usr/local/go/src/runtime/panic.go:491 +0x2c0
bytes.(*Buffer).ReadFrom(0x1042befc, 0x150600, 0x1741a0, 0x0, 0x200, 0x0, 0x10454000, 0x0)
    /usr/local/go/src/bytes/buffer.go:210 +0x320
io/ioutil.readAll(0x150600, 0x1741a0, 0x200, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x1603)
    /usr/local/go/src/io/ioutil/ioutil.go:33 +0x120
io/ioutil.ReadAll(0x150600, 0x1741a0, 0x1505a0, 0x1040c128, 0x0, 0x0, 0xbe820, 0x1603)
    /usr/local/go/src/io/ioutil/ioutil.go:42 +0x40
main.main()
    /tmp/sandbox767223408/main.go:12 +0x40

[ianlancetaylor]

Want to send in a change?

[dsnet]

I reckon that most implementations that takes in an io.Reader panics when the underlying io.Reader.Read ends up returning a negative count. Is turning clearly bad implementations of io.Readers into a normal error a precedence that we want to set?

I didn't check, but I'm pretty sure all of the archive and compress Readers will panic as well when the underlying Read returns a negative count.

[superstas]

I agree with @dsnet we shouldn't add returning of an explicit error.

But the change I propose is to add to bytes.Buffer its own panic for clearly understanding what exactly happened.

by analogy with https://golang.org/src/bufio/bufio.go#L80

var errNegativeRead = errors.New("bytes: reader returned negative count from Read")

instead of

panic: runtime error: slice bounds out of range [recovered]

BTW, I've checked a few io.Reader wrappers:
archive/tar doesn't panic because of this checking https://golang.org/src/archive/tar/reader.go#L699 ( https://play.golang.org/p/gBd8z7r9dh )
compress/bzip2 panics because it uses bufio.Reader by default https://golang.org/src/compress/bzip2/bit_reader.go#L28 ( https://play.golang.org/p/EDaOemlKmO )
compress/flate ( https://play.golang.org/p/bmKXv2Aulc ) and compress/gzip ( https://play.golang.org/p/t8ygc9MEiM ) have similar behavior because of underlying bufio.Reader

cc @ianlancetaylor @dsnet

[dsnet]

Panicking with a more informative error in bytes.Buffer.ReadFrom only sounds fine to me, but I'm a little wary that there will be a slippery slope where this check is added everywhere.

archive/tar does actually panic when it tries to call io.ReadFull on a bad reader. Should a check be added to io.ReadFull as well? As you can see, there are many places that this can fail.

Something to consider is whether the panic message should be improved with the index used. Perhaps something like:

runtime error: slice bounds out of range (indexed -1 on a slice of length 32768)

[superstas]

@dsnet Wonderful!
We've agreed that it'd be great to have a more informative panic message.

But I share your concerns about adding this check everywhere. Let's ask the community whether it's a problem for them actually?

If so, since it's widely used error in my opinion it makes sense to put that message into a common place ( io pkg? ) as a global variable. After that we'll be able to replace all unexpexted panic cases to panic(io.ErrNegativeRead) for example.

[dsnet]

If so, since it's widely used error in my opinion

I'm not convinced about this. Negative count from Read is a clearly a programmer bug and most likely arises when developing, when you accidentally write an bad Reader. The main utility in the panic message is only for debugging by developers.

For that reason, we should not be adding any global variables to package io. Exported error variables should only be for error values that programs expect to alter their control flow on. For example, io.EOF is very important for control flow. Unfortunately, some errors like io.ErrShortWrite have been added to the io package, and I have never seen any code that has ever checked for that error.

[superstas]

@dsnet thank you for the explanation

Let's sum all this up:
1) having of an informative error message in case of negative count from Read sounds reasonable
2) we shouldn't return an error in this case, only panic
3) we mustn't put this panic message as a global variable into somewhere, out of scope of a pkg that somehow wraps bad io.Reader implementation

According to the above, let's add a check to negative count from Read in bytes.Buffer.ReadFrom with an informative panic message.

[dsnet]

That is an accurate summary of what been discussed, thanks. Feel free to send a change for the check in bytes.Buffer.ReadFrom.

[superstas]

Thank you, I'll take it

[gopherbot]

Change https://golang.org/cl/67970 mentions this issue: bytes: added more informative panic message in case of negative count from Read

[gopherbot]

Change https://golang.org/cl/68810 mentions this issue: bytes: panic in ReadFrom with more information with negative Read counts