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

net/http: ResponseController panic #58237

Closed
komuw opened this issue Feb 2, 2023 · 14 comments
Closed

net/http: ResponseController panic #58237

komuw opened this issue Feb 2, 2023 · 14 comments
Labels
NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@komuw
Copy link
Contributor

komuw commented Feb 2, 2023

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

$ go version
go version go1.20 linux/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="/home//.cache/go-build"
GOENV="/home//.config/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home//go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home//go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="go1.20"
GCCGO="gccgo"
GOAMD64="v1"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home//Downloads/cool/go.mod"
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 -m64 -pthread -Wl,--no-gc-sections -fmessage-length=0 -fdebug-prefix-map=/run/user/1000/go-build839916512=/tmp/go-build -gno-record-gcc-switches"

What did you do?

Run the program; https://go.dev/play/p/Alu2e-LdzqX?v=gotip
and then use curl to access it curl -vkL https://127.0.0.1:56782/

What did you expect to see?

No panic, possibly an error

What did you see instead?

listening on  :56782
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x7471f5]

goroutine 22 [running]:
net/http.(*http2pipe).CloseWithError(...)
        /usr/local/go/src/net/http/h2_bundle.go:3718
net/http.(*http2stream).onReadTimeout(0xc00024e0b0)
        /usr/local/go/src/net/http/h2_bundle.go:5639 +0xd5
created by time.goFunc
        /usr/local/go/src/time/sleep.go:176 +0x48
exit status 2
@cuiweixie
Copy link
Contributor

this line cause panic, add If st.Body == nil maybe a choice to fix this. but I am not very sure.

@seankhliao
Copy link
Member

cc @neild

@seankhliao seankhliao added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Feb 2, 2023
AlexanderYastrebov added a commit to AlexanderYastrebov/net that referenced this issue Feb 2, 2023
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/464936 mentions this issue: http2: check stream body is present on read timeout

AlexanderYastrebov added a commit to AlexanderYastrebov/go that referenced this issue Feb 2, 2023
AlexanderYastrebov added a commit to AlexanderYastrebov/net that referenced this issue Feb 2, 2023
Check stream body is not nil in the handler to cover all callsites

For golang/go#58237
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/465035 mentions this issue: net/http: add ResponseController http2 request without body read deadline test

@lwithers
Copy link

Is there any further information that would be helpful for this issue? It can still be reproduced in Go 1.20.5, and prohibits the use of http.ResponseController.SetReadDeadline() in many real-world deployments (at least where TLS is enabled, since it's HTTP/2-specific).

AlexanderYastrebov added a commit to AlexanderYastrebov/net that referenced this issue Jul 21, 2023
Check stream body is not nil in the handler to cover all callsites

For golang/go#58237
AlexanderYastrebov added a commit to AlexanderYastrebov/go that referenced this issue Jul 21, 2023
@AlexanderYastrebov
Copy link
Contributor

https://go.dev/cl/465035 is the reproducer.
https://go.dev/cl/464936 is the fix.

gopherbot pushed a commit to golang/net that referenced this issue Sep 28, 2023
Check stream body is not nil in the handler to cover all callsites

For golang/go#58237

Change-Id: Ibeb19f2597f12da71b8dfb73718e230b4b316d06
GitHub-Last-Rev: dc87bef
GitHub-Pull-Request: #162
Reviewed-on: https://go-review.googlesource.com/c/net/+/464936
Reviewed-by: Bryan Mills <[email protected]>
Reviewed-by: Matthew Dempsky <[email protected]>
Run-TryBot: Damien Neil <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>
Reviewed-by: Damien Neil <[email protected]>
Auto-Submit: Bryan Mills <[email protected]>
Commit-Queue: Bryan Mills <[email protected]>
@bcmills bcmills added this to the Go1.22 milestone Sep 28, 2023
@bcmills bcmills added the NeedsFix The path to resolution is known, but the work has not been done. label Sep 28, 2023
@gopherbot gopherbot removed the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Sep 28, 2023
@bcmills
Copy link
Contributor

bcmills commented Sep 28, 2023

https://go.dev/cl/464936 is in, so at this point I think it's just waiting on someone to re-run the bundler to import x/net/http2 into net/http.

AlexanderYastrebov added a commit to AlexanderYastrebov/go that referenced this issue Oct 7, 2023
AlexanderYastrebov added a commit to AlexanderYastrebov/go that referenced this issue Oct 12, 2023
@AlexanderYastrebov
Copy link
Contributor

I think it's just waiting on someone to re-run the bundler to import x/net/http2

http2 bundle was updated by https://go-review.googlesource.com/c/go/+/534295
I've rebased https://go-review.googlesource.com/c/go/+/465035

@lwithers
Copy link

If I have understood correctly, I think the updated h2 bundle is in Go 1.21.3 (via e175f27), but I am afraid that I still see the problem with the original reproducer (https://go.dev/play/p/Alu2e-LdzqX?v=gotip):

$ go version
go version go1.21.3 linux/amd64
$ go build
$ ./h2test &
listening on  :56782
$ curl https://localhost.example:56782/hello-world
curl: (18) HTTP/2 stream 1 was not closed cleanly before end of the underlying stream

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x602e61]

goroutine 18 [running]:
net/http.(*http2pipe).CloseWithError(...)
	/opt/go/src/net/http/h2_bundle.go:3791
net/http.(*http2stream).onReadTimeout(0xc00010ec80)
	/opt/go/src/net/http/h2_bundle.go:5727 +0x61
created by time.goFunc
	/opt/go/src/time/sleep.go:176 +0x2d

@AlexanderYastrebov
Copy link
Contributor

@lwithers Could you please check tip, I think line numbers in your trace do not match:

func (p *http2pipe) CloseWithError(err error) { p.closeWithError(&p.err, err, nil) }

st.body.CloseWithError(fmt.Errorf("%w", os.ErrDeadlineExceeded))

@lwithers
Copy link

I cloned the Go repo and built at e4f72f7, and this did not exhibit the problem:

$ PATH=/home/lwithers/tmp/goroot/bin:/usr/local/bin:/usr/bin:/bin:/usr/local/games:/usr/games go version
go version devel go1.22-e4f72f7736 Thu Oct 12 14:39:39 2023 +0000 linux/amd64
$ PATH=/home/lwithers/tmp/goroot/bin:/usr/local/bin:/usr/bin:/bin:/usr/local/games:/usr/games go build
$ ./h2test &
listening on  :56782
$ curl https://localhost.internal.yoti.com:56782/hello-world
curl: (92) HTTP/2 stream 1 was not closed cleanly: INTERNAL_ERROR (err 2)

(the curl error is expected).

So tip is fine; it just appears to be Go 1.21.3 that still exhibits the problem.

AlexanderYastrebov added a commit to AlexanderYastrebov/go that referenced this issue Nov 22, 2023
@dcboy
Copy link

dcboy commented Nov 27, 2023

always on 1.21.4

reproducer: https://go.dev/play/p/Alu2e-LdzqX?v=gotip

[15:11:43]➜  go-http2 go version
go version go1.21.4 darwin/amd64
[15:12:28]➜  go-http2 go build
[15:12:35]➜  go-http2 ./go-http2
listening on  :56782
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x12055e1]

goroutine 21 [running]:
net/http.(*http2pipe).CloseWithError(...)
	/usr/local/go/src/net/http/h2_bundle.go:3791
net/http.(*http2stream).onReadTimeout(0xc000212140)
	/usr/local/go/src/net/http/h2_bundle.go:5727 +0x61
created by time.goFunc
	/usr/local/go/src/time/sleep.go:176 +0x2d

@AlexanderYastrebov
Copy link
Contributor

always on 1.21.4

The fix was merged to master by updating h2_bundle within https://go-review.googlesource.com/c/go/+/534295 for #63426
Then there were backports for #63426 that did not include the fix for this issue

@gopherbot gopherbot modified the milestones: Go1.22, Go1.23 Feb 6, 2024
AlexanderYastrebov added a commit to AlexanderYastrebov/go that referenced this issue Mar 6, 2024
gopherbot pushed a commit that referenced this issue Mar 6, 2024
…line test

Requires CL 464936

For #58237

Change-Id: I007b61f0f216d759f8e5327d77affbd9e8f8ff23
GitHub-Last-Rev: 30a1090
GitHub-Pull-Request: #58282
Reviewed-on: https://go-review.googlesource.com/c/go/+/465035
Reviewed-by: Damien Neil <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>
Reviewed-by: Michael Knyszek <[email protected]>
@seankhliao
Copy link
Member

I'm going to close this as fixed for 1.22 / tip.
I don't believe this was ever backported to 1.21.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

8 participants