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: calling Reset with itself leads to infinite recursion #58423

Closed
abursavich opened this issue Feb 9, 2023 · 4 comments
Closed

bufio: calling Reset with itself leads to infinite recursion #58423

abursavich opened this issue Feb 9, 2023 · 4 comments
Labels
FrozenDueToAge

Comments

@abursavich
Copy link

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

$ go version
go version go1.20 darwin/arm64

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="arm64"
GOBIN=""
GOCACHE="/Users/REDACTED/Library/Caches/go-build"
GOENV="/Users/REDACTED/Library/Application Support/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="arm64"
GOHOSTOS="darwin"
GOINSECURE=""
GOMODCACHE="/Users/REDACTED/go/pkg/mod"
GONOPROXY="REDACTED"
GONOSUMDB="REDACTED"
GOOS="darwin"
GOPATH="/Users/REDACTED/go"
GOPRIVATE="REDACTED"
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/darwin_arm64"
GOVCS=""
GOVERSION="go1.20"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/dev/null"
GOWORK=""
CGO_CFLAGS="-O2 -g"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-O2 -g"
CGO_FFLAGS="-O2 -g"
CGO_LDFLAGS="-O2 -g"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -arch arm64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/p4/t2ydl1qj10s99sfykmbnp6cc0000gq/T/go-build2269043344=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

I ran into this issue in a prometheus package that I was using.

The problem is that bufio.NewReader will return its given io.Reader as-is, if it happens to already be a *bufio.Reader of sufficient size. If you later call bufio.(*Reader).Reset on the "new" *bufio.Reader with the original io.Reader, it ends up setting the *bufio.Reader as its own underlying io.Reader. Then when you try to Read from it, there's an infinite recursion and stack overflow.

It's kinda strange to Reset a *bufio.Reader with the same io.Reader from which it was created, but I don't think bufio.NewReader should use the return as-is optimization without also handling this Reset with self issue.

My repro code is obviously contrived, but the actual case where this happened seemed fairly reasonable.

https://go.dev/play/p/2UTNBLVR1J__E

What did you expect to see?

I expected to see my server running smoothly.

What did you see instead?

My server kept crashing or getting OOM killed.
@ianlancetaylor ianlancetaylor changed the title bufio: infinite recursion in Reader bufio: calling Reset with itself leads to infinite recursion Feb 9, 2023
@gopherbot
Copy link

Change https://go.dev/cl/466815 mentions this issue: bufio: permit r.Reset(r) without infinite recursion

@ericlagergren
Copy link
Contributor

@ianlancetaylor you might want to use the two-value type assertion in case r isn't comparable. (Sorry for commenting on a closed issue.)

@ianlancetaylor
Copy link
Contributor

@ericlagergren Thanks, but it's not necessary. When comparing a non-interface against an interface value, we first check that the types are the same. If they are not, the comparison is not-equal. Only if the types are the same do we compare the values. And if the types are the same, we know that the type must be *Reader, which is comparable.

@ericlagergren
Copy link
Contributor

@ianlancetaylor oops. Apologies for the noise.

johanbrandhorst pushed a commit to Pryz/go that referenced this issue Feb 12, 2023
@ianlancetaylor @johanbrandhorst
bufio: permit r.Reset(r) without infinite recursion
cdbae90 
This can happen in reasonable code because NewReader(r) can return r,
if r is already a Reader.

Similarly for Writer.

Fixes golang#58423

Change-Id: Iff9d9265410bee68fbaeb7175369847bd737eb2c
Reviewed-on: https://go-review.googlesource.com/c/go/+/466815
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Auto-Submit: Ian Lance Taylor <iant@google.com>
Reviewed-by: Bryan Mills <bcmills@google.com>
Run-TryBot: Ian Lance Taylor <iant@google.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Run-TryBot: Ian Lance Taylor <iant@golang.org>
@golang golang locked and limited conversation to collaborators Feb 12, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@abursavich @ianlancetaylor @ericlagergren @gopherbot