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.

Sign up for GitHub

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

Jump to bottom

bufio: Scanner.Scan panics on poorly behaved io.Reader during an error #38053

Closed
neilpa opened this issue Mar 24, 2020 · 8 comments
Closed

bufio: Scanner.Scan panics on poorly behaved io.Reader during an error #38053

neilpa opened this issue Mar 24, 2020 · 8 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.15

Comments

@neilpa
Copy link

neilpa commented Mar 24, 2020 •
edited

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

$ go version
go version go1.14 darwin/amd64

Does this issue reproduce with the latest release?

Yes

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

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/neilpa/Library/Caches/go-build"
GOENV="/Users/neilpa/Library/Application Support/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOINSECURE=""
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/neilpa/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/Cellar/go/1.14/libexec"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/Cellar/go/1.14/libexec/pkg/tool/darwin_amd64"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/Users/neilpa/code/go-iris/go.mod"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/f2/vxszd7l136nbnp3g_tx_g2n40000gp/T/go-build872804308=/tmp/go-build -gno-record-gcc-switches -fno-common"
GOROOT/bin/go version: go version go1.14 darwin/amd64
GOROOT/bin/go tool compile -V: compile version go1.14
uname -v: Darwin Kernel Version 18.7.0: Thu Jan 23 06:52:12 PST 2020; root:xnu-4903.278.25~1/RELEASE_X86_64
ProductName:	Mac OS X
ProductVersion:	10.14.6
BuildVersion:	18G3020
lldb --version: lldb-1100.0.30.6
Apple Swift version 5.1.2 (swiftlang-1100.0.278 clang-1100.0.33.9)

What did you do?

If an io.Reader incorrectly returns -1 for bytes read along with an error from Read, bufio.Scanner will panic indexing the slice here.

Here's a simple repro on the go playground

This was discovered with a real project that's exposing native serial port APIs as io.Readers. There's PRs open fix the implemenation, however, it may be useful for bufio.Scanner to be resilient to this failure mode. Especially since golang.org/x/sys/unix.Read can return negative values under error, as is the case here.

What did you expect to see?

The error from the Read call to bubble up and be returned from Scanner.Err().

What did you see instead?

The scanner panics

panic: runtime error: slice bounds out of range [5:4]

goroutine 1 [running]:
bufio.(*Scanner).Scan(0x41a750, 0x40c178, 0x41a748, 0x1)
	/usr/local/go/src/bufio/scan.go:145 +0xc60
main.main()
	/tmp/sandbox512182328/prog.go:26 +0x200
@ianlancetaylor
Copy link
Contributor

I don't think it's necessary for bufio.Scanner to avoid panicking when passed an io.Reader that doesn't implement the contract described at https://golang.org/pkg/io/#Reader. It's important that the standard library avoid corrupting memory or otherwise behaving unpredictably. It's not essential to avoid panicking when given invalid input.

@neilpa
Copy link
Author

neilpa commented Mar 24, 2020

I made an edit to my original report that I do wan to explicitly call out. This is ostensibly a wrapper of golang.org/x/sys/unix.Read which can return negative values on error. I'd imagine this isn't the only instance where someone is wrapping that into an io.Reader and blindly returning the results.

@neilpa
Copy link
Author

neilpa commented Mar 24, 2020

Alternatively, I was thinking about some way for go vet or similar tooling to detect such an invalid implementation. However, I'm not sure how tenable that is to do the data flow tracing to detect such a condition.

@robpike
Copy link
Contributor

robpike commented Mar 24, 2020

This is a tricky one. Normally I agree with @ianlancetaylor , and this case is no exception, but then there's this comment in the code:

		// Finally we can read some input. Make sure we don't get stuck with
		// a misbehaving Reader. Officially we don't need to do this, but let's
		// be extra careful: Scanner is for safe, simple jobs.

That tells me that it's worth adding the check on the next line for a non-negative n.

@ianlancetaylor
Copy link
Contributor

OK, sent CL 225357.

@gopherbot
Copy link

Change https://golang.org/cl/225357 mentions this issue: bufio: don't panic when Scanner sees an impossible Read count

@andybons andybons added the NeedsFix The path to resolution is known, but the work has not been done. label Mar 25, 2020
@andybons andybons added this to the Go1.15 milestone Mar 25, 2020
@gopherbot
Copy link

Change https://golang.org/cl/237739 mentions this issue: doc/go1.15: document new bufio.ErrBadReadCount

@gopherbot
Copy link

Change https://golang.org/cl/238217 mentions this issue: bufio: test for exact error value in TestNegativeEOFReader and TestLargeReader

gopherbot pushed a commit that referenced this issue Jun 17, 2020
@dmitshur
bufio: test for exact error value in TestNegativeEOFReader and TestLa…
dea6d92 
…rgeReader

CL 225357 added tests for Scanner not panicking on bad readers.
CL 225557 created a named error value that is returned instead.
CL 237739 documents that the bufio.ErrBadReadCount is returned
when bufio.Scanner is used with an invalid io.Reader.

This suggests we wouldn't want that behavior to be able to change
without a test noticing it, so modify the tests to check for the
exact error value instead of just any non-nil one.

For #38053.

Change-Id: I4b0b8eb6804ebfe2c768505ddb94f0b1017fcf8b
Reviewed-on: https://go-review.googlesource.com/c/go/+/238217
Reviewed-by: Ian Lance Taylor <iant@golang.org>
gopherbot pushed a commit that referenced this issue Jun 18, 2020
@ianlancetaylor
doc/go1.15: document new bufio.ErrBadReadCount
84baf41 
For #37419
For #38053

Change-Id: I206f360ff4957bc7edc3c35dfc814b7bd5ec440c
Reviewed-on: https://go-review.googlesource.com/c/go/+/237739
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
@golang golang locked and limited conversation to collaborators Jun 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Go1.15
Development

No branches or pull requests
5 participants
@neilpa @andybons @ianlancetaylor @robpike @gopherbot