net/http: prevent server from draining request body if status 100 was not sent

When a client sends a request with "Expect: 100-continue", it waits for
the server to reply with a status 100 response before transmitting the
request body.

In some cases, a server might reject the request early, or otherwise do
not read the request body in order to generate its responses. In such
scenarios, we need to make sure that the server will not try to drain
the request body. This ensures that the server will not hang, waiting
for a request body that will never arrive.

Fixes #75933

Change-Id: Ice63b2fadfc2a72b825fb97975a2bd116a6a6964
Reviewed-on: https://go-review.googlesource.com/c/go/+/793160
LUCI-TryBot-Result: golang-scoped@luci-project-accounts.iam.gserviceaccount.com <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Damien Neil <dneil@google.com>
Reviewed-by: Nicholas Husin <husin@google.com>

src/net/http/serve_test.go

diff --git a/src/net/http/serve_test.go b/src/net/http/serve_test.go
index f8710af..1205e8b 100644
--- a/src/net/http/serve_test.go
+++ b/src/net/http/serve_test.go
@@ -7555,6 +7555,36 @@
 	readyc <- struct{}{} // server finishes reading from the request body
 }
 
+// Issue 75933.
+func TestServerExpect100ContinueUnreadBody(t *testing.T) {
+	run(t, testServerExpect100ContinueUnreadBody)
+}
+func testServerExpect100ContinueUnreadBody(t *testing.T, mode testMode) {
+	cst := newClientServerTest(t, mode, HandlerFunc(func(w ResponseWriter, r *Request) {
+		w.WriteHeader(StatusOK)
+		// Make sure that Read after not sending status 100 does not hang.
+		// TODO: Read in this situation should return an error.
+		io.ReadAll(r.Body)
+	}))
+
+	req, _ := NewRequest("POST", cst.ts.URL, strings.NewReader("some body"))
+	req.Header.Set("Expect", "100-continue")
+
+	// Set a short timeout on the client to catch the hang quickly.
+	cst.c.Timeout = 2 * time.Second
+	cst.tr.ExpectContinueTimeout = 10 * time.Second
+
+	resp, err := cst.c.Do(req)
+	if err != nil {
+		t.Fatalf("Request failed: %v (likely due to hang)", err)
+	}
+	defer resp.Body.Close()
+
+	if resp.StatusCode != StatusOK {
+		t.Errorf("expected 200 OK, got %v", resp.Status)
+	}
+}
+
 func TestInvalidChunkedBodies(t *testing.T) {
 	for _, test := range []struct {
 		name string

src/net/http/server.go

diff --git a/src/net/http/server.go b/src/net/http/server.go
index 1ef5621..058c42d 100644
--- a/src/net/http/server.go
+++ b/src/net/http/server.go
@@ -570,11 +570,30 @@
 	}
 }
 
-// disableWriteContinue stops Request.Body.Read from sending an automatic 100-Continue.
-// If a 100-Continue is being written, it waits for it to complete before continuing.
-func (w *response) disableWriteContinue() {
+// disableWriteContinue stops Request.Body.Read from sending an automatic
+// 100 Continue. As the name implies, it is only useful when the request
+// expects a 100 Continue and the body is wrapped in an expectContinueReader;
+// otherwise, it is a no-op.
+// If a 100-Continue is being written, it waits for it to complete before
+// continuing. If skipDrain is true, it also prevents the server from draining
+// the request body and flags the connection to be closed after the reply, as
+// the client will never send the body.
+func (w *response) disableWriteContinue(skipDrain bool) {
+	ecr, ok := w.reqBody.(*expectContinueReader)
+	if !ok {
+		return
+	}
 	w.writeContinueMu.Lock()
-	w.canWriteContinue.Store(false)
+	if w.canWriteContinue.Load() {
+		w.canWriteContinue.Store(false)
+		if skipDrain {
+			// Make sure that the connection will not be reused by sending
+			// "Connection: close" header in the response.
+			w.closeAfterReply = true
+			// Ensure that the body will not be drained in Close.
+			ecr.closed.Store(true)
+		}
+	}
 	w.writeContinueMu.Unlock()
 }
 
@@ -983,7 +1002,12 @@
 }
 
 func (ecr *expectContinueReader) Close() error {
-	ecr.closed.Store(true)
+	if ecr.resp.canWriteContinue.Load() {
+		ecr.resp.disableWriteContinue(true)
+	}
+	if ecr.closed.Swap(true) {
+		return nil
+	}
 	return ecr.readCloser.Close()
 }
 
@@ -1185,10 +1209,12 @@
 	}
 	checkWriteHeaderCode(code)
 
-	if code < 101 || code > 199 {
-		// Sending a 100 Continue or any non-1xx header disables the
-		// automatically-sent 100 Continue from Request.Body.Read.
-		w.disableWriteContinue()
+	// Sending a 100 Continue or any non-1XX header disables the
+	// automatically-sent 100 Continue from Request.Body.Read. If it is a final
+	// response (200 or higher), we skip draining the request body, which the
+	// client will never send.
+	if code == 100 || code >= 200 {
+		w.disableWriteContinue(code >= 200)
 	}
 
 	// Handle informational headers.
@@ -1663,7 +1689,7 @@
 
 	if w.canWriteContinue.Load() {
 		// Body reader wants to write 100 Continue but hasn't yet. Tell it not to.
-		w.disableWriteContinue()
+		w.disableWriteContinue(true)
 	}
 
 	if !w.wroteHeader {
@@ -1701,6 +1727,10 @@
 
 	w.conn.r.abortPendingRead()
 
+	if w.canWriteContinue.Load() {
+		w.disableWriteContinue(true)
+	}
+
 	// Close the body (regardless of w.closeAfterReply) so we can
 	// re-use its bufio.Reader later safely.
 	w.reqBody.Close()
@@ -1931,7 +1961,7 @@
 		}
 		if inFlightResponse != nil {
 			inFlightResponse.cancelCtx()
-			inFlightResponse.disableWriteContinue()
+			inFlightResponse.disableWriteContinue(true)
 		}
 		if !c.hijacked() {
 			if inFlightResponse != nil {
@@ -2094,6 +2124,7 @@
 				// Wrap the Body reader with one that replies on the connection
 				req.Body = &expectContinueReader{readCloser: req.Body, resp: w}
 				w.canWriteContinue.Store(true)
+				w.reqBody = req.Body
 			}
 		} else if req.Header.get("Expect") != "" {
 			w.sendExpectationFailed()
@@ -2257,7 +2288,7 @@
 	if w.handlerDone.Load() {
 		panic("net/http: Hijack called after ServeHTTP finished")
 	}
-	w.disableWriteContinue()
+	w.disableWriteContinue(false)
 	if w.wroteHeader {
 		w.cw.flush()
 	}