Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
superstas opened this issue Oct 1, 2017 · 11 comments
Closed

Comments

@superstas
Copy link
Contributor

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 ianlancetaylor changed the title bytes/buffer: a human-readable error message in case of negative count from Read bytes: buffer: a human-readable error message in case of negative count from Read Oct 1, 2017
@ianlancetaylor ianlancetaylor changed the title bytes: buffer: a human-readable error message in case of negative count from Read bytes: have buffer.ReadFrom return an error, not panic, in case of negative count from Read Oct 1, 2017
@ianlancetaylor ianlancetaylor added the NeedsFix The path to resolution is known, but the work has not been done. label Oct 1, 2017
@ianlancetaylor ianlancetaylor added this to the Unplanned milestone Oct 1, 2017
@ianlancetaylor
Copy link
Contributor

Want to send in a change?

@dsnet
Copy link
Member

dsnet commented Oct 2, 2017

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
Copy link
Contributor Author

superstas commented Oct 2, 2017

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
Copy link
Member

dsnet commented Oct 2, 2017

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)

@dsnet dsnet added NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. and removed NeedsFix The path to resolution is known, but the work has not been done. labels Oct 2, 2017
@superstas
Copy link
Contributor Author

@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
Copy link
Member

dsnet commented Oct 2, 2017

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.

@dsnet dsnet removed the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Oct 2, 2017
@superstas
Copy link
Contributor Author

@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
Copy link
Member

dsnet commented Oct 3, 2017

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

@dsnet dsnet changed the title bytes: have buffer.ReadFrom return an error, not panic, in case of negative count from Read bytes: have buffer.ReadFrom panic with better message with negative Read count Oct 3, 2017
@superstas
Copy link
Contributor Author

Thank you, I'll take it

@gopherbot
Copy link

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

@gopherbot
Copy link

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants