x/net/http2: panic on (*clientStream).bufPipe.Write #21466
Comments
|
@rs I see this happens on go1.8.3, how about on go1.9rc2 or on tip? Also if possible perhaps a repro, please? /cc @tombergan |
|
I’m using I’m working on a repro. |
|
@rs thanks for letting me know. Empirically I'd have thought that mattered because I could find go18.go, go19.go files in x/net/http2 but on further examination, doesn't seem like a lot has changed at least between go1.8 and go1.9 in there so I get what you mean, thanks. |
|
One way to reproduce (I can't assert that's what happen here), is to have a broken server sending DATA frame before HEADERS frame. Here is a way to reproduce the crash: https://play.golang.org/p/itUqFozrTC |
|
Change https://golang.org/cl/56770 mentions this issue: |
|
Reopening: this is fixed in x/net/http2 but needs to be imported into net/http. I will do that eventually. |
|
FYI, this fix did not solve our panic issue. It must be another code path leading to a nil |
|
Are you using x/net/http2 directly or net/http? The fix has not been imported into net/http yet. If you're using x/net/http2, can you post a stack trace? |
|
Yes we are using |
|
@tombergan any clue? |
|
@rs so I am using Go1.8.3 and I've gone through the net repo commit by commit, running the reproducer you posted.
https://github.com/golang/net/tree/1c05540f6879653db88113bc4a2b70aec4bd491f is the first hash on which I am able to reproduce your crash. From there on, I am able to keep the crash going until we encounter Therefore I assume your code was definitely before golang/net@57efc9c but probably after https://github.com/golang/net/tree/5961165da77ad3a2abf3a77ea904c13a76b0b073/http2.
|
|
My code is locked on golang/net@59a0b19 with a few unrelated (waiting for upstream) patches. That's why the line numbers don't match. I'm not using http2 thru the standard library but as a standalone lib so the version of Go should have minimal impact. The reproducer I provided was based on an assumption of a code path the would lead to this panic. It was an actual bug but apparently not the bug hitting my code. There must be another code path that is problematic. |
|
Can you provide links to the lines of code in the stack trace from this comment? The line numbers are kind of non-sensical even at golang/net@59a0b19. Is it possible to sync to head to verify this is not happening because you're missing some patch added since Jun 3? Note that there have been 13 patches since then. If you're running an old version with custom patches, it's going to be very difficult to verify that there's a bug in head rather than in your old branch plus patches. |
|
I'm locked on this rev because I can't go past golang/net@1c0554 without risking blocking requests due to the way the connection pool is handled today (cf discussion in #17776). Here is the stack trace with line number shifted to the current master: |
|
Is it possible to get an http2debug trace? If not, anything else you could mention about the request would be useful. Possible: if it's a HEAD request, then cs.bufPipe.b will be nil. If DATA comes in response to a HEAD, we might crash (I'm not sure -- a test case would verify). Possible: we are not forgetting the stream after an error reading the response header. For example, we might need to call forgetStream here. Unfortunately, the cancel/forget logic is currently a bit ad hoc and should be cleaned up. We could prevent any crash like this by checking if cs.bufPipe.p is nil here, but I'd rather understand why p is nil before doing anything. |
|
Unfortunately I currently have no way to link the request responsible for this crash with the panic. I can't enable http2debug as it would hurt perf and produce too much logs. We get about 25 panics per day across many servers. When a server get affected, it generally get several crashes in a row then nothing for days (or ever). I agree, I would prefer to get to the bottom of it instead of masking the problem with a check on |
Repro'd with the following test: func TestTransportResponseDataWithHeadRequest(t *testing.T) {
ct := newClientTester(t)
ct.client = func() error {
defer ct.cc.(*net.TCPConn).CloseWrite()
req := httptest.NewRequest("HEAD", "https://dummy.tld/", nil)
resp, err := ct.tr.RoundTrip(req)
if err != nil {
return fmt.Errorf("RoundTrip expected response, got error: %v", err)
}
defer resp.Body.Close()
_, err = ioutil.ReadAll(resp.Body)
return err
}
ct.server = func() error {
ct.greet()
for {
f, err := ct.fr.ReadFrame()
if err == io.EOF {
return nil
} else if err != nil {
return err
}
switch f := f.(type) {
case *WindowUpdateFrame, *SettingsFrame:
case *HeadersFrame:
// Send a HEAD response with a stray DATA frame.
var buf bytes.Buffer
enc := hpack.NewEncoder(&buf)
enc.WriteField(hpack.HeaderField{Name: ":status", Value: "200"})
ct.fr.WriteHeaders(HeadersFrameParam{
StreamID: f.StreamID,
EndHeaders: true,
EndStream: false,
BlockFragment: buf.Bytes(),
})
ct.fr.WriteData(f.StreamID, true, []byte("payload"))
default:
return fmt.Errorf("Unexpected client frame %v", f)
}
}
}
ct.run()
} |
|
@rs: I mailed a patch. Unfortunately I cannot be sure it will fix all of your panics. Can you try patching it locally and let me know? Thanks! |
|
Sure I will. Thank you for your help. I will keep you posted. |
|
Change https://golang.org/cl/64731 mentions this issue: |
|
Change https://golang.org/cl/71611 mentions this issue: |
If the server returns a DATA frame before a HEADERS frame, the client must close the connection with a PROTOCOL_ERROR as describe in the section 8.1.2.6. The current implementation does not reject such sequence and panics trying to write on the non-initialized bufPipe. Fixes golang/go#21466 Change-Id: I55755dba259d026529a031e9ff82c915f5e4afae Reviewed-on: https://go-review.googlesource.com/56770 Reviewed-by: Tom Bergan <tombergan@google.com> Run-TryBot: Tom Bergan <tombergan@google.com> TryBot-Result: Gobot Gobot <gobot@golang.org>
Please answer these questions before submitting your issue. Thanks!
What version of Go are you using (
go version)?go1.8.3
What operating system and processor architecture are you using (
go env)?freebsd, amd64
What did you do?
We a getting panics in http2 code while using it as a client (direct use of
http2.Transport).We are still trying to find a way to reproduce it.
Could be related to #20501.
What did you expect to see?
No panic.
What did you see instead?
The text was updated successfully, but these errors were encountered: