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

Jump to bottom

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

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

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

uthark opened this issue May 26, 2017 · 4 comments
Labels
FrozenDueToAge
Milestone
Go1.9

Comments

@uthark
Copy link

uthark commented May 26, 2017 •
edited

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.
@ericchiang ericchiang mentioned this issue May 26, 2017
Closed
@bradfitz bradfitz added this to the Go1.9 milestone May 26, 2017
@fraenkel
Copy link
Contributor

Write also needs a guard.

@bradfitz
Copy link
Contributor

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

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

@gopherbot
Copy link

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

gopherbot pushed a commit to golang/net that referenced this issue May 26, 2017
@tombergan @bradfitz
http2: fix nil dereference after Read completes with an error
3470a06 
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>
@rs rs mentioned this issue Aug 16, 2017
Closed
c3mb0 pushed a commit to c3mb0/net that referenced this issue Apr 2, 2018
@tombergan @bradfitz
http2: fix nil dereference after Read completes with an error
0740ecb 
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.
Assignees
No one assigned
Labels
FrozenDueToAge
Projects
None yet
Milestone
Go1.9
Development

No branches or pull requests
5 participants
@bradfitz @uthark @fraenkel @gopherbot @tombergan