net/http: disable 100 continue status after handler finished

When client supplies "Expect: 100-continue" header,
server wraps request body into expectContinueReader
that writes 100 Continue status on the first body read.

When handler acts as a reverse proxy and passes incoming
request (or body) to the client (or transport) it may happen
that request body is read after handler exists which may
cause nil pointer panic on connection write if server
already closed the connection.

This change disables write of 100 Continue status by expectContinueReader
after handler finished and before connection is closed.

It also fixes racy access to w.wroteContinue.

Fixes #53808
Updates #46866

Author: AlexanderYastrebov

src/net/http/clientserver_test.go

@@ -1754,3 +1754,28 @@ func testEarlyHintsRequest(t *testing.T, mode testMode) {
 		t.Errorf("Read body %q; want Hello", body)
 	}
 }
+
+// Issue 53808
+func TestServerReadAfterHandlerDone100Continue(t *testing.T) {
+	run(t, testServerReadAfterHandlerDone100Continue)
+}
+func testServerReadAfterHandlerDone100Continue(t *testing.T, mode testMode) {
+	readyc := make(chan struct{})
+	cst := newClientServerTest(t, mode, HandlerFunc(func(w ResponseWriter, r *Request) {
+		go func() {
+			<-readyc
+			io.ReadAll(r.Body)
+			<-readyc
+		}()
+	}))
+
+	req, _ := NewRequest("GET", cst.ts.URL, strings.NewReader("body"))
+	req.Header.Set("Expect", "100-continue")
+	res, err := cst.c.Do(req)
+	if err != nil {
+		t.Fatalf("Get(%q) = %v", cst.ts.URL, err)
+	}
+	res.Body.Close()
+	readyc <- struct{}{} // server starts reading from the request body
+	readyc <- struct{}{} // server finishes reading from the request body
+}

src/net/http/server.go

@@ -425,19 +425,14 @@ type response struct {
 	reqBody          io.ReadCloser
 	cancelCtx        context.CancelFunc // when ServeHTTP exits
 	wroteHeader      bool               // a non-1xx header has been (logically) written
-	wroteContinue    bool               // 100 Continue response was written
 	wants10KeepAlive bool               // HTTP/1.0 w/ Connection "keep-alive"
 	wantsClose       bool               // HTTP request has Connection "close"
 
-	// canWriteContinue is an atomic boolean that says whether or
-	// not a 100 Continue header can be written to the
-	// connection.
-	// writeContinueMu must be held while writing the header.
-	// These two fields together synchronize the body reader (the
-	// expectContinueReader, which wants to write 100 Continue)
-	// against the main writer.
-	canWriteContinue atomic.Bool
-	writeContinueMu  sync.Mutex
+	// writeContinue synchronizes the body reader (the
+	// expectContinueReader, which wants to write 100 Continue
+	// to the connection) against the main writer.
+	writeContinue sync.Once
+	wroteContinue atomic.Bool // 100 Continue response was written
 
 	w  *bufio.Writer // buffers output in chunks to chunkWriter
 	cw chunkWriter
@@ -917,15 +912,12 @@ func (ecr *expectContinueReader) Read(p []byte) (n int, err error) {
 		return 0, ErrBodyReadAfterClose
 	}
 	w := ecr.resp
-	if !w.wroteContinue && w.canWriteContinue.Load() && !w.conn.hijacked() {
-		w.wroteContinue = true
-		w.writeContinueMu.Lock()
-		if w.canWriteContinue.Load() {
+	if !w.conn.hijacked() {
+		w.writeContinue.Do(func() {
+			w.wroteContinue.Store(true)
 			w.conn.bufw.WriteString("HTTP/1.1 100 Continue\r\n\r\n")
 			w.conn.bufw.Flush()
-			w.canWriteContinue.Store(false)
-		}
-		w.writeContinueMu.Unlock()
+		})
 	}
 	n, err = ecr.readCloser.Read(p)
 	if err == io.EOF {
@@ -1165,10 +1157,8 @@ func (w *response) WriteHeader(code int) {
 	// so it takes the non-informational path.
 	if code >= 100 && code <= 199 && code != StatusSwitchingProtocols {
 		// Prevent a potential race with an automatically-sent 100 Continue triggered by Request.Body.Read()
-		if code == 100 && w.canWriteContinue.Load() {
-			w.writeContinueMu.Lock()
-			w.canWriteContinue.Store(false)
-			w.writeContinueMu.Unlock()
+		if code == 100 {
+			w.disableContinue()
 		}
 
 		writeStatusLine(w.conn.bufw, w.req.ProtoAtLeast(1, 1), code, w.statusBuf[:])
@@ -1383,7 +1373,7 @@ func (cw *chunkWriter) writeHeader(p []byte) {
 
 		switch bdy := w.req.Body.(type) {
 		case *expectContinueReader:
-			if bdy.resp.wroteContinue {
+			if bdy.resp.wroteContinue.Load() {
 				discard = true
 			}
 		case *body:
@@ -1625,15 +1615,7 @@ func (w *response) write(lenData int, dataB []byte, dataS string) (n int, err er
 		return 0, ErrHijacked
 	}
 
-	if w.canWriteContinue.Load() {
-		// Body reader wants to write 100 Continue but hasn't yet.
-		// Tell it not to. The store must be done while holding the lock
-		// because the lock makes sure that there is not an active write
-		// this very moment.
-		w.writeContinueMu.Lock()
-		w.canWriteContinue.Store(false)
-		w.writeContinueMu.Unlock()
-	}
+	w.disableContinue()
 
 	if !w.wroteHeader {
 		w.WriteHeader(StatusOK)
@@ -1679,6 +1661,11 @@ func (w *response) finishRequest() {
 	}
 }
 
+// disableContinue tells body reader not to write 100 Continue.
+func (w *response) disableContinue() {
+	w.writeContinue.Do(func() {})
+}
+
 // shouldReuseConnection reports whether the underlying TCP connection can be reused.
 // It must only be called after the handler is done executing.
 func (w *response) shouldReuseConnection() bool {
@@ -1905,6 +1892,7 @@ func (c *conn) serve(ctx context.Context) {
 			if inFlightResponse != nil {
 				inFlightResponse.conn.r.abortPendingRead()
 				inFlightResponse.reqBody.Close()
+				inFlightResponse.disableContinue()
 			}
 			c.close()
 			c.setState(c.rwc, StateClosed, runHooks)
@@ -2016,7 +2004,6 @@ func (c *conn) serve(ctx context.Context) {
 			if req.ProtoAtLeast(1, 1) && req.ContentLength != 0 {
 				// Wrap the Body reader with one that replies on the connection
 				req.Body = &expectContinueReader{readCloser: req.Body, resp: w}
-				w.canWriteContinue.Store(true)
 			}
 		} else if req.Header.get("Expect") != "" {
 			w.sendExpectationFailed()
@@ -2046,6 +2033,7 @@ func (c *conn) serve(ctx context.Context) {
 			return
 		}
 		w.finishRequest()
+		w.disableContinue()
 		c.rwc.SetWriteDeadline(time.Time{})
 		if !w.shouldReuseConnection() {
 			if w.requestBodyLimitHit || w.closedRequestBodyEarly() {

src/net/http/transport_test.go

@@ -26,6 +26,7 @@ import (
 	"log"
 	mrand "math/rand"
 	"net"
+	"net/http"
 	. "net/http"
 	"net/http/httptest"
 	"net/http/httptrace"
@@ -6901,19 +6902,31 @@ func testHandlerAbortRacesBodyRead(t *testing.T, mode testMode) {
 		panic(ErrAbortHandler)
 	})).ts
 
+	newRequest := func() *http.Request {
+		const reqLen = 6 * 1024 * 1024
+		req, _ := NewRequest("POST", ts.URL, &io.LimitedReader{R: neverEnding('x'), N: reqLen})
+		req.ContentLength = reqLen
+		return req
+	}
+
 	var wg sync.WaitGroup
 	for i := 0; i < 2; i++ {
 		wg.Add(1)
 		go func() {
 			defer wg.Done()
 			for j := 0; j < 10; j++ {
-				const reqLen = 6 * 1024 * 1024
-				req, _ := NewRequest("POST", ts.URL, &io.LimitedReader{R: neverEnding('x'), N: reqLen})
-				req.ContentLength = reqLen
+				req := newRequest()
 				resp, _ := ts.Client().Transport.RoundTrip(req)
 				if resp != nil {
 					resp.Body.Close()
 				}
+
+				req = newRequest()
+				req.Header.Set("Expect", "100-continue")
+				resp, _ = ts.Client().Transport.RoundTrip(req)
+				if resp != nil {
+					resp.Body.Close()
+				}
 			}
 		}()
 	}