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

[neilpa]

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]

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]

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]

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]

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]

OK, sent CL 225357.

[gopherbot]

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

[gopherbot]

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

[gopherbot]

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