[tailscale-cherrypick] net/http: fix two bugs from CL 828504

src/net/http/serve_test.go

@@ -18,6 +18,7 @@ import (
 	"encoding/json"
 	"errors"
 	"fmt"
+	"internal/nettest"
 	"internal/testenv"
 	"io"
 	"log"
@@ -8179,3 +8180,107 @@ func TestServerRequestBodyCloseAfterPartialRead(t *testing.T) {
 		}
 	})
 }
+
+// bespokeTimeoutError is a timeout error that does not wrap
+// os.ErrDeadlineExceeded, as returned by non-standard net.Conn
+// implementations predating the Go 1.15 convention that deadline
+// errors wrap os.ErrDeadlineExceeded.
+type bespokeTimeoutError struct{}
+
+func (bespokeTimeoutError) Error() string   { return "i/o timeout" }
+func (bespokeTimeoutError) Timeout() bool   { return true }
+func (bespokeTimeoutError) Temporary() bool { return true }
+
+// bespokeTimeoutConn wraps a net.Conn, replacing read errors that wrap
+// os.ErrDeadlineExceeded with bespokeTimeoutError. It emulates
+// non-standard net.Conn implementations such as gVisor's gonet.
+type bespokeTimeoutConn struct {
+	net.Conn
+}
+
+func (c bespokeTimeoutConn) Read(p []byte) (int, error) {
+	n, err := c.Conn.Read(p)
+	if errors.Is(err, os.ErrDeadlineExceeded) {
+		err = bespokeTimeoutError{}
+	}
+	return n, err
+}
+
+type bespokeTimeoutListener struct {
+	net.Listener
+}
+
+func (l bespokeTimeoutListener) Accept() (net.Conn, error) {
+	c, err := l.Listener.Accept()
+	if err != nil {
+		return nil, err
+	}
+	return bespokeTimeoutConn{c}, nil
+}
+
+// testServerIdleKeepAlive makes two requests on the same keep-alive
+// connection, idling between them (in the bubble's fake time) long
+// enough for the server's idle buffer release probe (after
+// idleBufsReleaseDelay, 50ms) to time out while the connection sits
+// idle. It checks that both requests succeed and that neither handler
+// sees an already-canceled request context. It must be called in a
+// synctest bubble; wrap adapts the server's listener.
+func testServerIdleKeepAlive(t *testing.T, wrap func(net.Listener) net.Listener) {
+	t.Helper()
+	ctxErr := make(chan error, 1)
+	srv := &Server{Handler: HandlerFunc(func(w ResponseWriter, r *Request) {
+		ctxErr <- r.Context().Err()
+	})}
+	li := nettest.NewListener()
+	srvDone := make(chan struct{})
+	go func() {
+		defer close(srvDone)
+		srv.Serve(wrap(li))
+	}()
+	defer func() {
+		srv.Close()
+		<-srvDone
+	}()
+
+	conn := li.NewConn()
+	defer conn.Close()
+	tc := &http1TestConn{t: t, conn: conn, bufr: bufio.NewReader(conn)}
+	for i := range 2 {
+		if i == 1 {
+			synctest.Sleep(250 * time.Millisecond)
+		}
+		tc.writeMessage(
+			"GET / HTTP/1.1",
+			"Host: foo",
+			"",
+		)
+		if res := tc.readResponse(); res.StatusCode != 200 {
+			t.Errorf("request %d: got status %v; want 200", i, res.Status)
+		}
+		if err := <-ctxErr; err != nil {
+			t.Errorf("request %d: handler's Request.Context().Err() = %v; want nil", i, err)
+		}
+	}
+}
+
+// TestServerIdleKeepAliveRequestContext verifies that the idle buffer
+// release probe between keep-alive requests does not cancel the
+// connection-level context that subsequent requests' contexts derive
+// from.
+func TestServerIdleKeepAliveRequestContext(t *testing.T) {
+	synctest.Test(t, func(t *testing.T) {
+		testServerIdleKeepAlive(t, func(li net.Listener) net.Listener { return li })
+	})
+}
+
+// TestServerIdleKeepAliveNonstandardTimeoutError verifies that an idle
+// HTTP/1 keep-alive connection survives the idle buffer release probe
+// even when the underlying net.Conn returns a timeout error that does
+// not wrap os.ErrDeadlineExceeded.
+func TestServerIdleKeepAliveNonstandardTimeoutError(t *testing.T) {
+	synctest.Test(t, func(t *testing.T) {
+		testServerIdleKeepAlive(t, func(li net.Listener) net.Listener {
+			return bespokeTimeoutListener{li}
+		})
+	})
+}

src/net/http/server.go

@@ -23,7 +23,6 @@ import (
 	"net/textproto"
 	"net/url"
 	urlpkg "net/url"
-	"os"
 	"path"
 	"runtime"
 	"slices"
@@ -711,6 +710,7 @@ type connReader struct {
 	cond    *sync.Cond
 	inRead  bool
 	aborted bool  // set true before conn.rwc deadline is set to past
+	probing bool  // set true during conn.serve's idle probe read, when a timeout is expected
 	remain  int64 // bytes remaining
 }
 
@@ -837,24 +837,51 @@ func (cr *connReader) abortPendingRead() {
 	cr.rwc.SetReadDeadline(time.Time{})
 }
 
+func (cr *connReader) setProbing(v bool) {
+	cr.lock()
+	cr.probing = v
+	cr.unlock()
+}
+
 func (cr *connReader) setReadLimit(remain int64) { cr.remain = remain }
 func (cr *connReader) setInfiniteReadLimit()     { cr.remain = maxInt64 }
 func (cr *connReader) hitReadLimit() bool        { return cr.remain <= 0 }
 
+// isNetTimeoutError reports whether err is a net.Error with Timeout()
+// == true, such as an error from an expired connection deadline.
+// It is used instead of checking errors.Is(err, os.ErrDeadlineExceeded)
+// because non-standard net.Conn implementations may return bespoke
+// timeout errors that don't wrap os.ErrDeadlineExceeded as net package
+// connections have since Go 1.15.
+func isNetTimeoutError(err error) bool {
+	ne, ok := errors.AsType[net.Error](err)
+	return ok && ne.Timeout()
+}
+
 // handleReadErrorLocked is called whenever a Read from the client returns a
 // non-nil error.
 //
 // The provided non-nil err is almost always io.EOF or a "use of
 // closed network connection". In any case, the error is not
 // particularly interesting, except perhaps for debugging during
-// development. Any error means the connection is dead and we should
-// down its context.
+// development. Except for an expected timeout during the serve loop's
+// idle probe read, any error means the connection is dead and we
+// should down its context.
 //
 // The caller must hold connReader.mu.
-func (cr *connReader) handleReadErrorLocked(_ error) {
+func (cr *connReader) handleReadErrorLocked(err error) {
 	if cr.conn == nil {
 		return
 	}
+	// A timeout during conn.serve's idle probe read means only that the
+	// connection has gone idle; it is otherwise fine. In particular,
+	// don't cancel the connection-level context: it is the parent of
+	// every subsequent request's context on this connection, so
+	// canceling it would deliver already-canceled contexts to all
+	// future requests.
+	if cr.probing && isNetTimeoutError(err) {
+		return
+	}
 	cr.conn.cancelCtx()
 	if res := cr.conn.curReq.Load(); res != nil {
 		res.closeNotify()
@@ -2230,8 +2257,10 @@ func (c *conn) serve(ctx context.Context) {
 			shortDeadline = idleDeadline
 		}
 		c.rwc.SetReadDeadline(shortDeadline)
+		c.r.setProbing(true)
 		_, peekErr := c.bufr.Peek(4)
-		if errors.Is(peekErr, os.ErrDeadlineExceeded) && (idleDeadline.IsZero() || time.Now().Before(idleDeadline)) {
+		c.r.setProbing(false)
+		if isNetTimeoutError(peekErr) && (idleDeadline.IsZero() || time.Now().Before(idleDeadline)) {
 			c.rwc.SetReadDeadline(idleDeadline)
 			if c.bufr.Buffered() == 0 && c.bufw.Buffered() == 0 {
 				putBufioReader(c.bufr)