net/http: fix panic error message on Request.Body read after Hijack #20933
Comments
|
Yes, this was changed in Go 1.8. After your call to Hijack, you're not supposed to read from Request.Body anymore. I'll keep this bug open to fix that error message. After a hijack, you're expected to read from the returned bufio.Reader: https://golang.org/pkg/net/http/#Hijacker
(That includes the 1 byte) BTW, the time for Go 1.8 feedback was in December. We're currently wrapping up Go 1.9, which is almost out. If you could test Go 1.9beta2, that'd b egreat. |
bradfitz
changed the title
bradfitz
added
the
NeedsFix
Is that explicitly documented anywhere? I can confirm that rearranging the example so that it Hijacks only after fully reading the request seems to fix it: https://play.golang.org/p/akmrJbBnN6 |
Apparently, reading from Request.Body after a hijack is not allowed: golang/go#20933 So we need to make sure that we stop messing with the request after the hijack call. I also ensured that we read any buffered data that might be there. Signed-off-by: Wayne Song <wsong@docker.com>
Apparently, reading from Request.Body after a hijack is not allowed: golang/go#20933 So we need to make sure that we stop messing with the request after the hijack call. I also ensured that we read any buffered data that might be there. Signed-off-by: Wayne Song <wsong@docker.com>
|
Hello @bradfitz, @JiHawn replied asking if we've documented that "After your call to Hijack, you're not supposed to read from Request.Body anymore." -- it doesn't seem like that's documented AFAIK from https://tip.golang.org/pkg/net/http |
|
CL https://golang.org/cl/50634 mentions this issue. |
|
Okay, it's now documented for Go 1.9 in c522b2b. We can panic more nicely in Go 1.10. |
We can make it panic with a more explicit and readable error message during Go 1.10, but document it for now. This has always been the case; it's not a new rule. Updates #20933 Change-Id: I53c1fefb47a8f4aae0bb32fa742afa3a2ed20e8a Reviewed-on: https://go-review.googlesource.com/50634 Reviewed-by: Ian Lance Taylor <iant@golang.org> Reviewed-by: Emmanuel Odeke <emm.odeke@gmail.com>
|
Change https://golang.org/cl/59850 mentions this issue: |
What version of Go are you using (
go version)?What operating system and processor architecture are you using (
go env)?What did you do?
If
(*http.Request).Bodyis read after the connection is hijacked, abackgroundReadoperation is triggered once EOF of the body is encountered. This background read will read from the underlying connection when it shouldn't (because the connection has been hijacked).What did you expect to see?
I would expect this program to not fail: https://play.golang.org/p/QUXm31VhCr
Note that if I change that program to always read from the hijacked connection buffer, it will panic instead: https://play.golang.org/p/E7UyjfgkdS
What did you see instead?
The
backgroundReadcallback happens unconditionally and will read a byte from the connection that is lost if you then try to read directly from theconnand will result in a panic if you try to read from the buffered reader.The programs above worked as expected in go1.7
The bug seems to be introduced here:
go/src/net/http/server.go
Lines 1683 to 1684 in faf882d
cc @bradfitz
Before calling
ServeHTTP(w, r), ifrequestBodyRemainsthen a callback is registered which will begin abackgroundReadonce the body is fully read. The background read probably should not occur if the connection has been hijacked.A proposed fix would be to change
startBackgroundReadto check ifcr.conn.hijacked()immediately after acquiring the lock and return early if the connection has been hijacked.The text was updated successfully, but these errors were encountered: