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: "missing status pseudo header" when reusing HTTP/2 connection #22880

Closed
leonklingele opened this Issue Nov 26, 2017 · 20 comments

Comments

Projects
None yet
7 participants
@leonklingele
Contributor

leonklingele commented Nov 26, 2017

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

1.9.2

Does this issue reproduce with the latest release?

Yes

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

GOHOSTARCH="amd64"
GOHOSTOS="darwin"

What did you do?

Reusing an HTTP client resulted in a panic, example code: https://play.golang.org/p/DGUqwSMygZ
Unfortunately, I was unable to reproduce this issue again, still find it worth reporting it here.

Panic trace:

Got response!
failed to GET: missing status pseudo header
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x30 pc=0x11d5340]

goroutine 7 [running]:
net/http.(*http2pipe).Write(0xc420457ce8, 0xc4200620e0, 0x51, 0x64, 0x0, 0x0, 0x0)
	/usr/local/Cellar/go/1.9.2/libexec/src/net/http/h2_bundle.go:3661 +0x100
net/http.(*http2clientConnReadLoop).processData(0xc420489fb0, 0xc42007f560, 0xc42007f560, 0x0)
	/usr/local/Cellar/go/1.9.2/libexec/src/net/http/h2_bundle.go:8259 +0x2ac
net/http.(*http2clientConnReadLoop).run(0xc420489fb0, 0x12a9300, 0xc4200357b0)
	/usr/local/Cellar/go/1.9.2/libexec/src/net/http/h2_bundle.go:7896 +0x54f
net/http.(*http2ClientConn).readLoop(0xc420080820)
	/usr/local/Cellar/go/1.9.2/libexec/src/net/http/h2_bundle.go:7788 +0x9d
created by net/http.(*http2Transport).newClientConn
	/usr/local/Cellar/go/1.9.2/libexec/src/net/http/h2_bundle.go:7053 +0x6b9
exit status 2

The first request succeeded, (Got response!), the second one failed (failed to GET: missing status pseudo header) and most likely the third one caused the panic.
Is this a bug? Do Go HTTP clients need to be reinstantiated upon request failure?

@bradfitz

This comment has been minimized.

Member

bradfitz commented Nov 26, 2017

Do Go HTTP clients need to be reinstantiated upon request failure?

No, they don't.

Which URL are you actually requesting? I'm wondering which server you're hitting.

Can you attach a trace with GODEBUG=http2debug=2 set?

/cc @tombergan

@leonklingele

This comment has been minimized.

Contributor

leonklingele commented Nov 26, 2017

Can you attach a trace with GODEBUG=http2debug=2 set?

No unfortunately not. The crash only occurred once and never again since.

Which URL are you actually requesting?

I initially preferred to not disclose this information, maybe it might still help though:
The app happened to crash while trying to query various coin prices from Kraken using this library: https://github.com/nakatanakatana/cryptowatch-go
Example code here: https://play.golang.org/p/qvUGs3v24f

@leonklingele

This comment has been minimized.

Contributor

leonklingele commented Nov 26, 2017

Good news, it crashed again 🎉

2017/11/26 18:56:47 http2: Transport encoding header ":authority" = "api.cryptowat.ch"
2017/11/26 18:56:47 http2: Transport encoding header ":method" = "GET"
2017/11/26 18:56:47 http2: Transport encoding header ":path" = "/markets/kraken/etceur/price"
2017/11/26 18:56:47 http2: Transport encoding header ":scheme" = "https"
2017/11/26 18:56:47 http2: Transport encoding header "accept-encoding" = "gzip"
2017/11/26 18:56:47 http2: Transport encoding header "user-agent" = "Go-http-client/2.0"
2017/11/26 18:56:47 http2: Framer 0xc4201448c0: wrote HEADERS flags=END_STREAM|END_HEADERS stream=7 len=27
2017/11/26 18:56:48 http2: Framer 0xc4201448c0: read HEADERS flags=END_HEADERS stream=7 len=31
2017/11/26 18:56:48 http2: decoded hpack field header field "content-encoding" = "gzip"
2017/11/26 18:56:48 http2: decoded hpack field header field "content-type" = "application/json"
2017/11/26 18:56:48 http2: decoded hpack field header field "content-length" = "71"
2017/11/26 18:56:48 http2: decoded hpack field header field "date" = "Sun, 26 Nov 2017 17:56:48 GMT"
2017/11/26 18:56:48 http2: Transport received HEADERS flags=END_HEADERS stream=7 len=31
2017/11/26 18:56:48 http2: Framer 0xc4201448c0: wrote RST_STREAM stream=7 len=4 ErrCode=PROTOCOL_ERROR
2017/11/26 18:56:48 http2: Framer 0xc4201448c0: read DATA flags=END_STREAM stream=7 len=71 data="\x1f\x8b\b\x00\x00\x00\x00\x00\x00\xff\xaaVJ\xcc\xc9\xc9/O\xccKNU\xb2\xaaVJ\xce/.Q\xb22365\xb7\xb0421\xd7Q*J\xcdM\xcc\xcc\xcb\xccKW\xb22\xa8\xad\xe5\x02\x04\x00\x00\xff\xff.\xa9\xda\xeb/\x00\x00\x00"
2017/11/26 18:56:48 http2: Transport received DATA flags=END_STREAM stream=7 len=71 data="\x1f\x8b\b\x00\x00\x00\x00\x00\x00\xff\xaaVJ\xcc\xc9\xc9/O\xccKNU\xb2\xaaVJ\xce/.Q\xb22365\xb7\xb0421\xd7Q*J\xcdM\xcc\xcc\xcb\xccKW\xb22\xa8\xad\xe5\x02\x04\x00\x00\xff\xff.\xa9\xda\xeb/\x00\x00\x00"
2017/11/26 18:56:48 RoundTrip failure: missing status pseudo header
failed to query price: Get https://api.cryptowat.ch/markets/kraken/etceur/price: missing status pseudo header
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x30 pc=0x11d5340]

goroutine 9 [running]:
net/http.(*http2pipe).Write(0xc4204cd928, 0xc420278000, 0x47, 0x64, 0x0, 0x0, 0x0)
	/usr/local/Cellar/go/1.9.2/libexec/src/net/http/h2_bundle.go:3661 +0x100
net/http.(*http2clientConnReadLoop).processData(0xc420055fb0, 0xc420115590, 0x1c, 0xc420055ec8)
	/usr/local/Cellar/go/1.9.2/libexec/src/net/http/h2_bundle.go:8259 +0x2ac
net/http.(*http2clientConnReadLoop).run(0xc420055fb0, 0x12a9340, 0xc420468fb0)
	/usr/local/Cellar/go/1.9.2/libexec/src/net/http/h2_bundle.go:7896 +0x54f
net/http.(*http2ClientConn).readLoop(0xc420080340)
	/usr/local/Cellar/go/1.9.2/libexec/src/net/http/h2_bundle.go:7788 +0x9d
created by net/http.(*http2Transport).newClientConn
	/usr/local/Cellar/go/1.9.2/libexec/src/net/http/h2_bundle.go:7053 +0x6b9

EDIT:
Interesting, the app crashed after querying the very last URL (price for etceur), therefore the crash is not related to reusing a connection but might be caused by destructing it?!

@bradfitz

This comment has been minimized.

Member

bradfitz commented Nov 26, 2017

That's a bogus response from the api.cryptowat.ch HTTP/2 server.

Admittedly we should handle it better (not crash, whoops), but we also need to contact the author of that HTTP/2 server and get it fixed.

What do Chrome & Firefox do when :status is missing? Hopefully they're not permitting it and encouraging such violations.

/cc @tombergan @mcmanus @mnot

@bradfitz bradfitz added NeedsFix and removed WaitingForInfo labels Nov 26, 2017

@bradfitz bradfitz added this to the Go1.10 milestone Nov 26, 2017

@mnot

This comment has been minimized.

mnot commented Nov 26, 2017

Did a quick tcpdump; that server doesn't appear to be negotiating H2, either with ALPN or Alt-Svc.

@leonklingele

This comment has been minimized.

Contributor

leonklingele commented Nov 27, 2017

@mnot both h2i and Chrome do negotiate HTTP/2 on my side:

$ h2i api.cryptowat.ch
Connecting to api.cryptowat.ch:443 ...
Connected to 96.126.127.137:443
Negotiated protocol "h2"
Sending: []
[FrameHeader SETTINGS len=24]
  [MAX_FRAME_SIZE = 1048576]
  [MAX_CONCURRENT_STREAMS = 250]
  [MAX_HEADER_LIST_SIZE = 1048896]
  [INITIAL_WINDOW_SIZE = 1048576]
[FrameHeader WINDOW_UPDATE len=4]
  Window-Increment = 983041

[FrameHeader SETTINGS flags=ACK len=0]

cryptowatch_http2

@bradfitz

This comment has been minimized.

Member

bradfitz commented Nov 27, 2017

It's possible it's only doing NPN and not ALPN. I can check later.

@mcmanus

This comment has been minimized.

mcmanus commented Nov 27, 2017

a response missing :status is a PROTOCOL_ERROR in firefox

@gopherbot

This comment has been minimized.

gopherbot commented Nov 27, 2017

Change https://golang.org/cl/80056 mentions this issue: http2: don't crash in Transport on server's DATA following bogus HEADERS

@bradfitz

This comment has been minimized.

Member

bradfitz commented Nov 27, 2017

api.cryptowat.ch seems to do both NPN and ALPN.

Still not sure which HTTP/2 server it is.

https://cryptowat.ch/about says it was created by @artursapek. Artur, can you comment on which HTTP/2 server you're using for Cryptowatch? The HTTP/2 implementation seems buggy.

@artursapek

This comment has been minimized.

artursapek commented Nov 27, 2017

hi @bradfitz. it's a net/http go server, actually. we're binding to ":443" directly using https://godoc.org/net/http#Server.ListenAndServeTLS to get the HTTP/2 features.

the binary on production was built with:

go version go1.9 linux/amd64

GOOS="linux"
GOARCH="amd64"
@bradfitz

This comment has been minimized.

Member

bradfitz commented Nov 27, 2017

Well that's embarrassing.

@artursapek, thanks. I'll try to figure out how and where we're dropping :status.

@artursapek

This comment has been minimized.

artursapek commented Nov 27, 2017

@bradfitz no worries, lmk if I can provide more specific details. and thanks for all your work on Go! we have a ton of gophers working hard for us over here.

@bradfitz

This comment has been minimized.

Member

bradfitz commented Nov 27, 2017

@artursapek, I actually do see one way for this to happen, but it's weird: your http.Handler code would have to call ResponseWriter.WriteHeader(0). In that case, we record that the user set an explicit header and don't add the implicit 200.

Is it possible you ever call WriteHeader(0), even accidentally? Hopefully that's the case (because it's easy to fix) and it's not something else I'm not seeing.

@bradfitz

This comment has been minimized.

Member

bradfitz commented Nov 27, 2017

I could imagine code like:

var status int
if someCondition {
    status = http.BadRequest
} else if otherCondition {
    status = 500
}
rw.WriteHeader(status)

... that would forget to set status.

@artursapek

This comment has been minimized.

artursapek commented Nov 27, 2017

@bradfitz you're right, there was a case where we were failing to overwrite a default zero-value and passing that into WriteHeader. that branch of logic only happened in a timeout case (during moments of very high traffic), so that's why it was only happening occasionally from @leonklingele's perspective. I've deployed to production an update which fixes this.

@bradfitz

This comment has been minimized.

Member

bradfitz commented Nov 27, 2017

Thanks for confirming. I'll fix the http2 server and make sure we're consistent in behavior from Go's API point of view between http1 and http2.

gopherbot pushed a commit to golang/net that referenced this issue Nov 27, 2017

http2: don't crash in Transport on server's DATA following bogus HEADERS
In golang/go#22880, our http2 server was sending a HEADERS response
without a :status header. Our client code correctly returned an error
from RoundTrip, but we forgot to clean up properly, and then
subsequently crashed on a DATA frame.

This fixes the Transport crash. A fix for the server bug will come separately.

Change-Id: Iea3bcf4a8c95963c8b5e2b6dd722177607bd1bc1
Reviewed-on: https://go-review.googlesource.com/80056
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Tom Bergan <tombergan@google.com>
@gopherbot

This comment has been minimized.

gopherbot commented Nov 27, 2017

Change https://golang.org/cl/80076 mentions this issue: http2: panic on invalid WriteHeader status code

gopherbot pushed a commit to golang/net that referenced this issue Nov 27, 2017

http2: panic on invalid WriteHeader status code
Tests are in net/http. (upcoming CL)

Updates golang/go#22880

Change-Id: Ie94693ad4e14f0c07926a0b6c7827caace94a0aa
Reviewed-on: https://go-review.googlesource.com/80076
Reviewed-by: Tom Bergan <tombergan@google.com>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@bradfitz

This comment has been minimized.

Member

bradfitz commented Nov 27, 2017

Also https://go-review.googlesource.com/c/go/+/80077/3 (in which I typoed this issue number).

@gopherbot

This comment has been minimized.

gopherbot commented Nov 28, 2017

Change https://golang.org/cl/80078 mentions this issue: net/http: update bundled http2

c3mb0 pushed a commit to c3mb0/net that referenced this issue Apr 2, 2018

http2: don't crash in Transport on server's DATA following bogus HEADERS
In golang/go#22880, our http2 server was sending a HEADERS response
without a :status header. Our client code correctly returned an error
from RoundTrip, but we forgot to clean up properly, and then
subsequently crashed on a DATA frame.

This fixes the Transport crash. A fix for the server bug will come separately.

Change-Id: Iea3bcf4a8c95963c8b5e2b6dd722177607bd1bc1
Reviewed-on: https://go-review.googlesource.com/80056
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Tom Bergan <tombergan@google.com>

c3mb0 pushed a commit to c3mb0/net that referenced this issue Apr 2, 2018

http2: panic on invalid WriteHeader status code
Tests are in net/http. (upcoming CL)

Updates golang/go#22880

Change-Id: Ie94693ad4e14f0c07926a0b6c7827caace94a0aa
Reviewed-on: https://go-review.googlesource.com/80076
Reviewed-by: Tom Bergan <tombergan@google.com>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>

jasonwbarnett pushed a commit to jasonwbarnett/fileserver that referenced this issue Jul 11, 2018

net/http: update bundled http2
Update http2 to x/net git rev db473f6b23.

(And un-skip TestWriteHeader0_h2 added in CL 80077, now fixed.)

Includes:

   http2: remove afterReqBodyWriteError wrapper
   https://golang.org/cl/75252

   http2: fix transport data race on reused *http.Request objects
   https://golang.org/cl/75530

   http2: require either ECDSA or RSA ciphersuite
   https://golang.org/cl/30721

   http2: don't log about timeouts reading client preface on new connections
   https://golang.org/cl/79498

   http2: don't crash in Transport on server's DATA following bogus HEADERS
   https://golang.org/cl/80056

   http2: panic on invalid WriteHeader status code
   https://golang.org/cl/80076

   http2: fix race on ClientConn.maxFrameSize
   https://golang.org/cl/79238

   http2: don't autodetect Content-Type when the response has an empty body
   https://golang.org/cl/80135

Fixes golang/go#18776
Updates golang/go#20784
Fixes golang/go#21316
Fixes golang/go#22721
Fixes golang/go#22880

Change-Id: Ie86e24e0ee2582a5a82afe5de3c7c801528be069
Reviewed-on: https://go-review.googlesource.com/80078
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Tom Bergan <tombergan@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment