internal/http3: prevent request body read when 100 status was not sent Make HTTP/3 pass TestServerExpect100ContinueUnreadBody that was added in CL 793160. Also add serverRequestReader so the server will always deal with the same req.Body. This makes the code simpler and gets rid of the scattered logic used to coordinate bodyReader and responseWriter. For golang/go#70914 Change-Id: Ic94c222f39e83e121012ddcd2579a2b66a6a6964 Reviewed-on: https://go-review.googlesource.com/c/net/+/798321 Auto-Submit: Nicholas Husin <nsh@golang.org> Reviewed-by: Nicholas Husin <husin@google.com> Reviewed-by: Damien Neil <dneil@google.com> LUCI-TryBot-Result: golang-scoped@luci-project-accounts.iam.gserviceaccount.com <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
diff --git a/internal/http3/body.go b/internal/http3/body.go index e025f5d..c80bc75 100644 --- a/internal/http3/body.go +++ b/internal/http3/body.go
@@ -121,10 +121,6 @@ mu sync.Mutex remain int64 err error - // If not nil, the body contains an "Expect: 100-continue" header, and - // send100Continue should be called when Read is invoked for the first - // time. - send100Continue func() // A map where the key represents the trailer header names we expect. If // there is a HEADERS frame after reading DATA frames to EOF, the value of // the headers will be written here. Keys in the map are assumed to be @@ -141,10 +137,6 @@ // Use a mutex here to provide the same behavior. r.mu.Lock() defer r.mu.Unlock() - if r.send100Continue != nil { - r.send100Continue() - r.send100Continue = nil - } if r.err != nil { return 0, r.err }
diff --git a/internal/http3/server.go b/internal/http3/server.go index bfa31b0..ef7d9a4 100644 --- a/internal/http3/server.go +++ b/internal/http3/server.go
@@ -9,7 +9,6 @@ "crypto/tls" "errors" "fmt" - "io" "maps" "net/http" "net/textproto" @@ -544,21 +543,10 @@ } } - var body io.ReadCloser contentLength := int64(-1) if n, err := strconv.Atoi(header.Get("Content-Length")); err == nil { contentLength = int64(n) } - if contentLength != 0 || len(reqInfo.Trailer) != 0 { - body = &bodyReader{ - st: st, - remain: contentLength, - trailer: reqInfo.Trailer, - filterTrailer: true, - } - } else { - body = http.NoBody - } req := &http.Request{ Proto: "HTTP/3.0", @@ -569,11 +557,9 @@ Trailer: reqInfo.Trailer, ProtoMajor: 3, RemoteAddr: sc.qconn.RemoteAddr().String(), - Body: body, Header: header, ContentLength: contentLength, } - defer req.Body.Close() rw := &responseWriter{ st: st, @@ -589,10 +575,21 @@ enc: &sc.enc, }, } - if reqInfo.NeedsContinue { - req.Body.(*bodyReader).send100Continue = func() { - rw.WriteHeader(100) + + if contentLength != 0 || len(reqInfo.Trailer) != 0 { + req.Body = &serverRequestReader{ + rw: rw, + br: bodyReader{ + st: st, + remain: contentLength, + trailer: reqInfo.Trailer, + filterTrailer: true, + }, + needsContinue: reqInfo.NeedsContinue, } + defer req.Body.Close() + } else { + req.Body = http.NoBody } // TODO: handle panic coming from the HTTP handler. @@ -644,8 +641,8 @@ trailer http.Header bb bodyBuffer wroteHeader bool // Non-1xx header has been (logically) written. - statusCode int // Status of the response that will be sent in HEADERS frame. - statusCodeSet bool // Status of the response has been set via a call to WriteHeader. + statusCode int // Non-1xx status of the response that will be sent in HEADERS frame. Zero means none has been set. + sent100 bool // Status 100 has been sent by the server. cannotHaveBody bool // Response should not have a body (e.g. response to a HEAD request). bodyLenLeft int // How much of the content body is left to be sent, set via "Content-Length" header. -1 if unknown. } @@ -726,6 +723,12 @@ if rw.wroteHeader { return } + if statusCode == 100 { + if rw.sent100 { + return + } + rw.sent100 = true + } encHeaders := rw.bw.enc.encode(func(f func(itype indexType, name, value string)) { f(mayIndex, ":status", strconv.Itoa(statusCode)) for name, values := range rw.headers { @@ -774,7 +777,7 @@ // TODO: handle sending informational status headers (e.g. 103). rw.mu.Lock() defer rw.mu.Unlock() - if rw.statusCodeSet { + if rw.statusCode != 0 { return } checkWriteHeaderCode(statusCode) @@ -789,7 +792,6 @@ // Non-informational headers should only be set once, and should be // buffered. - rw.statusCodeSet = true rw.statusCode = statusCode rw.snapHeaders = rw.headers.Clone() if n, err := strconv.Atoi(rw.Header().Get("Content-Length")); err == nil { @@ -960,3 +962,42 @@ // we have chosen not to do so for now as Content-Length is not very // important for HTTP/3, and such inconsistent behavior might be confusing. } + +// serverRequestReader wraps around bodyReader, allowing Read and Close calls +// done from within a server handler to coordinate correctly with the +// responseWriter; for example, sending status 100 on Read when appropriate. +type serverRequestReader struct { + rw *responseWriter + br bodyReader + needsContinue bool +} + +// maybeSendContinue attempts to send a 100 Continue status code. It +// ensures that status 100 will only be sent once and when appropriate. If a +// non-1xx header has been set before 100 was ever set, it also ensures that +// all subsequent Read will fail. +func (srr *serverRequestReader) maybeSendContinue() { + if !srr.needsContinue { + return + } + srr.rw.mu.Lock() + defer srr.rw.mu.Unlock() + if srr.rw.sent100 { + return + } + if srr.rw.statusCode != 0 { + srr.br.Close() + return + } + srr.rw.writeHeaderLocked(100) + srr.rw.st.Flush() +} + +func (srr *serverRequestReader) Read(p []byte) (int, error) { + srr.maybeSendContinue() + return srr.br.Read(p) +} + +func (srr *serverRequestReader) Close() error { + return srr.br.Close() +}
diff --git a/internal/http3/server_test.go b/internal/http3/server_test.go index 138a878..0e9c973 100644 --- a/internal/http3/server_test.go +++ b/internal/http3/server_test.go
@@ -682,6 +682,38 @@ }) } +func TestServerExpect100ContinueSentManually(t *testing.T) { + synctest.Test(t, func(t *testing.T) { + ts := newTestServer(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(100) + body, err := io.ReadAll(r.Body) // Should not send another 100. + if err != nil { + t.Fatal(err) + } + w.Write(body) + })) + tc := ts.connect() + tc.greet() + + // Client sends an Expect: 100-continue request. + reqStream := tc.newStream(streamTypeRequest) + reqStream.writeHeaders(requestHeader(http.Header{ + "expect": {"100-continue"}, + })) + + // Send the body once the server responds with HTTP status 100. + reqStream.wantSomeHeaders(http.Header{":status": {"100"}}) + body := []byte("body that will be echoed back") + reqStream.writeData(body) + reqStream.CloseWrite() + + // Verify that the server responds with 200, rather than another 100. + reqStream.wantSomeHeaders(http.Header{":status": {"200"}}) + reqStream.wantData(body) + reqStream.wantClosed("request is complete") + }) +} + func TestServerExpect100ContinueRejected(t *testing.T) { synctest.Test(t, func(t *testing.T) { rejectBody := []byte("not allowed") @@ -705,14 +737,17 @@ }) } -func TestServerNoExpect100ContinueAfterNormalResponse(t *testing.T) { +func TestServer100ContinueBodyReadAfterFinalResponse(t *testing.T) { synctest.Test(t, func(t *testing.T) { ts := newTestServer(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(200) w.(http.Flusher).Flush() - // This should not cause an HTTP 100 status to be sent since we + // Read should not cause an HTTP 100 status to be sent since we // have sent an HTTP 200 response already. - io.ReadAll(r.Body) + // Read should also return an error and should not hang. + if _, err := io.ReadAll(r.Body); err == nil { + t.Errorf("got %v, want an error", err) + } })) tc := ts.connect() tc.greet() @@ -722,12 +757,6 @@ reqStream.writeHeaders(requestHeader(http.Header{ "expect": {"100-continue"}, })) - // Client sends a body prematurely. This should not happen, unless a - // client misbehaves. We do so here anyways so the server handler can - // read the request body without hanging, which would normally cause an - // HTTP 100 to be sent. - reqStream.writeData([]byte("some body")) - reqStream.CloseWrite() // Verify that no HTTP 100 was sent. reqStream.wantSomeHeaders(http.Header{":status": {"200"}}) @@ -735,6 +764,36 @@ }) } +func TestServer100ContinueBodyReadAfter100AndFinalResponse(t *testing.T) { + synctest.Test(t, func(t *testing.T) { + body := []byte("client body") + ts := newTestServer(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(100) + w.WriteHeader(200) + w.(http.Flusher).Flush() + // Allow Read to succeed since the handler has sent 100 prior to 200. + if gotBody, err := io.ReadAll(r.Body); err != nil || string(gotBody) != string(body) { + t.Errorf("io.ReadAll(r.Body) = %v, %v; want %v, nil", gotBody, err, body) + } + })) + tc := ts.connect() + tc.greet() + + // Client sends an Expect: 100-continue request. + reqStream := tc.newStream(streamTypeRequest) + reqStream.writeHeaders(requestHeader(http.Header{ + "expect": {"100-continue"}, + })) + + // Send the body once the server responds with HTTP status 100. + reqStream.wantSomeHeaders(http.Header{":status": {"100"}}) + reqStream.writeData(body) + reqStream.CloseWrite() + reqStream.wantSomeHeaders(http.Header{":status": {"200"}}) + reqStream.wantClosed("request is complete") + }) +} + func TestServerHandlerReadReqWithNoBody(t *testing.T) { synctest.Test(t, func(t *testing.T) { serverBody := []byte("hello from server!")