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

x/net/http2: panics after updating to commit 1a26cf06691746ee35aa7113c9b37289afc7ea28 #20501

Closed
uthark opened this issue May 26, 2017 · 4 comments
Closed
Assignees
Milestone

Comments

@uthark
Copy link

@uthark uthark commented May 26, 2017

Please answer these questions before submitting your issue. Thanks!

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

go1.8.1

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

linux, amd64

What did you do?

After recent x/net/http2 update I'm getting panics in http2 code.

I'm using kubernetes client-go, version 2.0, so this is the sources:

https://github.com/kubernetes/client-go/blob/release-2.0/rest/request.go

If possible, provide a recipe for reproducing the error.
A complete runnable program is good.
A link on play.golang.org is best.

What did you expect to see?

No panic.

What did you see instead?

clustermgr 2017/05/25 20:15:11 runtime error: invalid memory address or nil panic(0x187a980, 0x2736260)
        /usr/local/go/src/runtime/panic.go:489 +0x2cf
controller/vendor/golang.org/x/net/http2.(*pipe).Read(0xc4207b5428, 0xc420164000, 0x800, 0x2000, 0x0, 0x0, 0x0)
        controller/vendor/golang.org/x/net/http2/pipe.go:53 +0xa8
controller/vendor/golang.org/x/net/http2.transportResponseBody.Read(0xc4207b5400, 0xc420164000, 0x800, 0x2000, 0x0, 0x0, 0x0)
        controller/vendor/golang.org/x/net/http2/transport.go:1588 +0xae
io.(*LimitedReader).Read(0xc420632ba0, 0xc420164000, 0x2000, 0x2000, 0x0, 0x0, 0x0)
        /usr/local/go/src/io/io.go:436 +0x6c
io/ioutil.devNull.ReadFrom(0x0, 0x26ea7a0, 0xc420632ba0, 0x18adb60, 0x1, 0x7fcbcb4cc550)
        /usr/local/go/src/io/ioutil/ioutil.go:144 +0x85
io/ioutil.(*devNull).ReadFrom(0x2770fb0, 0x26ea7a0, 0xc420632ba0, 0x7fcbcb4cc550, 0x2770fb0, 0x35c5b79b101)
        <autogenerated>:6 +0x61
io.copyBuffer(0x26ed1a0, 0x2770fb0, 0x26ea7a0, 0xc420632ba0, 0x0, 0x0, 0x0, 0x1916660, 0x0, 0x7fcbcb4d2c98)
        /usr/local/go/src/io/io.go:384 +0x2cb
io.Copy(0x26ed1a0, 0x2770fb0, 0x26ea7a0, 0xc420632ba0, 0xc4207b5400, 0x1b267f8, 0xc420620000)
        /usr/local/go/src/io/io.go:360 +0x68
controller/vendor/k8s.io/client-go/rest.(*Request).request.func2.1(0xc42011a750)
        controller/vendor/k8s.io/client-go/rest/request.go:822 +0xe9
controller/vendor/k8s.io/client-go/rest.(*Request).request.func2(0xc42011a750, 0xc4206112c8, 0xa, 0xc420600000, 0xc420611348, 0xc420284f00, 0xc4208aaf00, 0x22, 0x1)
        controller/vendor/k8s.io/client-go/rest/request.go:843 +0xe0
controller/vendor/k8s.io/client-go/rest.(*Request).request(0xc420600000, 0xc420611348, 0x0, 0x0)
        controller/vendor/k8s.io/client-go/rest/request.go:844 +0x3c1
controller/vendor/k8s.io/client-go/rest.(*Request).Do(0xc420600000, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, ...)
        controller/vendor/k8s.io/client-go/rest/request.go:865 +0xc0
controller/vendor/k8s.io/client-go/kubernetes/typed/core/v1.(*nodes).List(0xc42074dd40, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, ...)
        controller/vendor/k8s.io/client-go/kubernetes/typed/core/v1/node.go:130 +0x112

Downgrading to commit golang/net@5961165 resolves the issue.

https://go-review.googlesource.com/c/43810/ - This was the change which introduced panic.

I checked source of Read method - it looks like after setting p.b to nil in pipe.go:61, consequent calls to Read should check if p.b is nil.

@fraenkel

This comment has been minimized.

Copy link
Contributor

@fraenkel fraenkel commented May 26, 2017

Write also needs a guard.

@bradfitz

This comment has been minimized.

Copy link
Contributor

@bradfitz bradfitz commented May 26, 2017

It looks like the invariant was originally supposed to be that p.b is only nil if p.breakErr was non-nil, and breakErr was already checked before use of p.b, but then the other p.b = nil was added in https://go-review.googlesource.com/c/43810/6/http2/pipe.go#61

@tombergan?

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented May 26, 2017

CL https://golang.org/cl/44330 mentions this issue.

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented May 26, 2017

CL https://golang.org/cl/44331 mentions this issue.

gopherbot pushed a commit to golang/net that referenced this issue May 26, 2017
Case happens if Read is called after it has already returned an error
previously. Verified that the new TestPipeCloseWithError test fails
before this change but passes after.

Updates golang/go#20501

Change-Id: I636fbb194f2d0019b0722556cc25a88da2d18e13
Reviewed-on: https://go-review.googlesource.com/44330
Run-TryBot: Tom Bergan <tombergan@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@gopherbot gopherbot closed this in 2cb3d1d May 26, 2017
c3mb0 pushed a commit to c3mb0/net that referenced this issue Apr 2, 2018
Case happens if Read is called after it has already returned an error
previously. Verified that the new TestPipeCloseWithError test fails
before this change but passes after.

Updates golang/go#20501

Change-Id: I636fbb194f2d0019b0722556cc25a88da2d18e13
Reviewed-on: https://go-review.googlesource.com/44330
Run-TryBot: Tom Bergan <tombergan@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@golang golang locked and limited conversation to collaborators May 26, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.