x/net/http2: Server can write invalid HTTP/2 if handler writes before reading 100-continue request body #14030
Labels
FrozenDueToAge
NeedsInvestigation
Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Comments
quentinmit
added
the
NeedsInvestigation
|
@bradfitz Punt to Go 1.8? |
|
I will either finish everything or punt everything in the next two days before I disappear for a month. This one I'm still curious about and is easier to test now that https://go-review.googlesource.com/#/c/23235/ for #13851 is ready. |
|
Confirmed that this is a bug. I wrote this test: // golang.org/issue/14030
func TestExpect100ContinueAfterHandlerWrites(t *testing.T) {
const msg = "Hello"
const msg2 = "World"
doRead := make(chan bool, 1)
defer close(doRead) // fallback cleanup
st := newServerTester(t, func(w http.ResponseWriter, r *http.Request) {
io.WriteString(w, msg)
w.(http.Flusher).Flush()
// Do a read, which might force a 100-continue status to be sent.
<-doRead
r.Body.Read(make([]byte, 10))
io.WriteString(w, msg2)
}, optOnlyServer)
defer st.Close()
tr := &Transport{TLSClientConfig: tlsConfigInsecure}
defer tr.CloseIdleConnections()
req, _ := http.NewRequest("POST", st.ts.URL, io.LimitReader(neverEnding('A'), 2<<20))
req.Header.Set("Expect", "100-continue")
res, err := tr.RoundTrip(req)
if err != nil {
t.Fatal(err)
}
defer res.Body.Close()
buf := make([]byte, len(msg))
if _, err := io.ReadFull(res.Body, buf); err != nil {
t.Fatal(err)
}
if string(buf) != msg {
t.Fatalf("msg = %q; want %q", buf, msg)
}
doRead <- true
if _, err := io.ReadFull(res.Body, buf); err != nil {
t.Fatal(err)
}
if string(buf) != msg2 {
t.Fatalf("second msg = %q; want %q", buf, msg2)
}
}And it results in: (where line 1963 is the second read....) So we are sending a bogus 100 continue mid-data. |
|
/cc @mholt |
bradfitz
changed the title
gopherbot
pushed a commit
to golang/net
that referenced
this issue
May 21, 2016
Since Go's HTTP/2 Handlers can read & write concurrently, it was previously possible for a Handler to get an HTTP Request with "Expect: 100-continue" and to first Write some data, then read some data, and the read of the data was triggering the server's automatic "100-continue" response path. But if the server had already replied with a non-100 HEADERS frame, sending a HEADERS frame with :status 100 later is in violation of the spec. Fix it by tracking whether we've already replied with something, and ignoring the 100-continue when it's inappropriate. Updates golang/go#14030 Change-Id: I6745ba0f257a31eaf63816e4263cadbef5a896a2 Reviewed-on: https://go-review.googlesource.com/23311 Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
|
CL https://golang.org/cl/23312 mentions this issue. |
|
👍 Enjoy your vacation! |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
I had this in an old local dev branch, never completed:
func (sc *serverConn) writeFrame(wm frameWriteMsg) { sc.serveG.check() + + // http2 handlers can conccurently read and write. + // If they already wrote headers, ignore a 100-continue + // header generated by the reader part. + println(fmt.Sprintf("Frame write for %v of type %T", wm.stream, wm.write)) + switch wm.write.(type) { + case *writeResHeaders: + wm.stream.wroteHeaders = true + case write100ContinueHeadersFrame: + if wm.stream.wroteHeaders { + sc.scheduleFrameWrite() + return + } + } +The http2 Server supports an http.Handler concurrently reading the request body while the response body is also being streamed.
But because the Go http servers have always automatically sent 100-continue headers in response to "Expect: 100-continue" in requests, there's a potential bug in Go's http2 server where the automatic
HEADERSframe (with header field:status=100) gets sent AFTER the handler has already started writing, if separate goroutines are reading & writing.Or not. But I don't think there's a test or precautions in the code about it. We might be getting lucky from some other layer.
Investigate. Low priority, though, since handlers don't typically do this, and people don't really use 100-continue.
The text was updated successfully, but these errors were encountered: