From 129d0cb543e1e15cdea706dd7230ee90c8d54446 Mon Sep 17 00:00:00 2001 From: Peter Beard Date: Tue, 28 Oct 2025 10:26:26 -0600 Subject: net/http/cgi: accept INCLUDED as protocol for server side includes The existing protocol check for fcgi/cgi requests did not properly account for Apache SSI (Server-Side Includes) SERVER_PROTOCOL value of INCLUDED. Added check for well-known INCLUDED value for proper implementation of the CGI Spec as specified in RFC 3875 - section 4.1.16. The SERVER_PROTOCOL section of the specification is outlined at https://www.rfc-editor.org/rfc/rfc3875.html#section-4.1.16 Fixes #70416 Change-Id: I129e606147e16d1daefb49ed6c13a561a88ddeb6 Reviewed-on: https://go-review.googlesource.com/c/go/+/715680 Reviewed-by: Damien Neil LUCI-TryBot-Result: Go LUCI Reviewed-by: Junyang Shao Reviewed-by: Sean Liao Auto-Submit: Sean Liao --- src/net/http/cgi/child.go | 7 +++++-- src/net/http/cgi/child_test.go | 22 ++++++++++++++++++++++ 2 files changed, 27 insertions(+), 2 deletions(-) (limited to 'src/net') diff --git a/src/net/http/cgi/child.go b/src/net/http/cgi/child.go index e29fe20d7d..466d42c08e 100644 --- a/src/net/http/cgi/child.go +++ b/src/net/http/cgi/child.go @@ -57,8 +57,11 @@ func RequestFromMap(params map[string]string) (*http.Request, error) { r.Proto = params["SERVER_PROTOCOL"] var ok bool - r.ProtoMajor, r.ProtoMinor, ok = http.ParseHTTPVersion(r.Proto) - if !ok { + if r.Proto == "INCLUDED" { + // SSI (Server Side Include) use case + // CGI Specification RFC 3875 - section 4.1.16 + r.ProtoMajor, r.ProtoMinor = 1, 0 + } else if r.ProtoMajor, r.ProtoMinor, ok = http.ParseHTTPVersion(r.Proto); !ok { return nil, errors.New("cgi: invalid SERVER_PROTOCOL version") } diff --git a/src/net/http/cgi/child_test.go b/src/net/http/cgi/child_test.go index 18cf789bd5..f901bec1a8 100644 --- a/src/net/http/cgi/child_test.go +++ b/src/net/http/cgi/child_test.go @@ -154,6 +154,28 @@ func TestRequestWithoutRemotePort(t *testing.T) { } } +// CGI Specification RFC 3875 - section 4.1.16 +// INCLUDED value for SERVER_PROTOCOL must be treated as an HTTP/1.0 request +func TestIncludedServerProtocol(t *testing.T) { + env := map[string]string{ + "REQUEST_METHOD": "GET", + "SERVER_PROTOCOL": "INCLUDED", + } + req, err := RequestFromMap(env) + if req.Proto != "INCLUDED" { + t.Errorf("unexpected change to SERVER_PROTOCOL") + } + if major := req.ProtoMajor; major != 1 { + t.Errorf("ProtoMajor: got %d, want %d", major, 1) + } + if minor := req.ProtoMinor; minor != 0 { + t.Errorf("ProtoMinor: got %d, want %d", minor, 0) + } + if err != nil { + t.Fatalf("expected INCLUDED to be treated as HTTP/1.0 request") + } +} + func TestResponse(t *testing.T) { var tests = []struct { name string -- cgit v1.3-6-g1900 From 17a02b910697800032aa686548992a554e8e9d82 Mon Sep 17 00:00:00 2001 From: 1911860538 Date: Mon, 10 Nov 2025 11:17:01 +0000 Subject: net/http: remove unused isLitOrSingle and isNotToken isLitOrSingle and isNotToken are private and unused. Change-Id: I07718d4496e92d5f75ed74986e174a8aa1f70a88 GitHub-Last-Rev: 722c4dccd85dca5d28a52e95a4f9efbab2b11807 GitHub-Pull-Request: golang/go#76216 Reviewed-on: https://go-review.googlesource.com/c/go/+/718700 Reviewed-by: Damien Neil Auto-Submit: Sean Liao Reviewed-by: Junyang Shao Reviewed-by: Sean Liao LUCI-TryBot-Result: Go LUCI --- src/net/http/http.go | 4 ---- src/net/http/pattern.go | 8 -------- 2 files changed, 12 deletions(-) (limited to 'src/net') diff --git a/src/net/http/http.go b/src/net/http/http.go index e7959fa3b6..d346e60646 100644 --- a/src/net/http/http.go +++ b/src/net/http/http.go @@ -119,10 +119,6 @@ func removeEmptyPort(host string) string { return host } -func isNotToken(r rune) bool { - return !httpguts.IsTokenRune(r) -} - // isToken reports whether v is a valid token (https://www.rfc-editor.org/rfc/rfc2616#section-2.2). func isToken(v string) bool { // For historical reasons, this function is called ValidHeaderFieldName (see issue #67031). diff --git a/src/net/http/pattern.go b/src/net/http/pattern.go index 8fd120e777..a5063807c6 100644 --- a/src/net/http/pattern.go +++ b/src/net/http/pattern.go @@ -394,14 +394,6 @@ func inverseRelationship(r relationship) relationship { } } -// isLitOrSingle reports whether the segment is a non-dollar literal or a single wildcard. -func isLitOrSingle(seg segment) bool { - if seg.wild { - return !seg.multi - } - return seg.s != "/" -} - // describeConflict returns an explanation of why two patterns conflict. func describeConflict(p1, p2 *pattern) string { mrel := p1.compareMethods(p2) -- cgit v1.3-6-g1900 From cb0d9980f5721715ebb73dd2e580eaa11c2ddee2 Mon Sep 17 00:00:00 2001 From: "Nicholas S. Husin" Date: Fri, 14 Nov 2025 16:11:23 -0500 Subject: net/http: do not discard body content when closing it within request handlers (*body).Close() internally tries to discard the content of a request body up to 256 KB. We rely on this behavior to allow connection re-use, by calling (*body).Close() when our request handler exits. Unfortunately, this causes an unfortunate side-effect where we would prematurely try to discard a body content when (*body).Close() is called from within a request handler. There should not be a good reason for (*body).Close() to do this when called from within a request handler. As such, this CL modifies (*body).Close() to not discard body contents when called from within a request handler. Note that when a request handler exits, it will still try to discard the body content for connection re-use. For #75933 Change-Id: I71d2431a540579184066dd35d3da49d6c85c3daf Reviewed-on: https://go-review.googlesource.com/c/go/+/720380 LUCI-TryBot-Result: Go LUCI Reviewed-by: Nicholas Husin Reviewed-by: Damien Neil --- src/net/http/serve_test.go | 188 ++++++++++++++++++++++++--------------------- src/net/http/server.go | 31 +++++--- src/net/http/transfer.go | 109 ++++++++++++++------------ 3 files changed, 185 insertions(+), 143 deletions(-) (limited to 'src/net') diff --git a/src/net/http/serve_test.go b/src/net/http/serve_test.go index 4a16ba02af..af15593c35 100644 --- a/src/net/http/serve_test.go +++ b/src/net/http/serve_test.go @@ -2150,118 +2150,137 @@ func TestServerUnreadRequestBodyLarge(t *testing.T) { } } -type handlerBodyCloseTest struct { +type bodyDiscardTest struct { bodySize int bodyChunked bool reqConnClose bool - wantEOFSearch bool // should Handler's Body.Close do Reads, looking for EOF? - wantNextReq bool // should it find the next request on the same conn? + shouldDiscardBody bool // should the handler discard body after it exits? } -func (t handlerBodyCloseTest) connectionHeader() string { +func (t bodyDiscardTest) connectionHeader() string { if t.reqConnClose { return "Connection: close\r\n" } return "" } -var handlerBodyCloseTests = [...]handlerBodyCloseTest{ - // Small enough to slurp past to the next request + - // has Content-Length. +var bodyDiscardTests = [...]bodyDiscardTest{ + // Have: + // - Small body. + // - Content-Length defined. + // Should: + // - Discard remaining body. 0: { - bodySize: 20 << 10, - bodyChunked: false, - reqConnClose: false, - wantEOFSearch: true, - wantNextReq: true, + bodySize: 20 << 10, + bodyChunked: false, + reqConnClose: false, + shouldDiscardBody: true, }, - // Small enough to slurp past to the next request + - // is chunked. + // Have: + // - Small body. + // - Chunked (no Content-Length defined). + // Should: + // - Discard remaining body. 1: { - bodySize: 20 << 10, - bodyChunked: true, - reqConnClose: false, - wantEOFSearch: true, - wantNextReq: true, + bodySize: 20 << 10, + bodyChunked: true, + reqConnClose: false, + shouldDiscardBody: true, }, - // Small enough to slurp past to the next request + - // has Content-Length + - // declares Connection: close (so pointless to read more). + // Have: + // - Small body. + // - Content-Length defined. + // - Connection: close. + // Should: + // - Not discard remaining body (no point as Connection: close). 2: { - bodySize: 20 << 10, - bodyChunked: false, - reqConnClose: true, - wantEOFSearch: false, - wantNextReq: false, + bodySize: 20 << 10, + bodyChunked: false, + reqConnClose: true, + shouldDiscardBody: false, }, - // Small enough to slurp past to the next request + - // declares Connection: close, - // but chunked, so it might have trailers. - // TODO: maybe skip this search if no trailers were declared - // in the headers. + // Have: + // - Small body. + // - Chunked (no Content-Length defined). + // - Connection: close. + // Should: + // - Discard remaining body (chunked, so it might have trailers). + // + // TODO: maybe skip this if no trailers were declared in the headers. 3: { - bodySize: 20 << 10, - bodyChunked: true, - reqConnClose: true, - wantEOFSearch: true, - wantNextReq: false, + bodySize: 20 << 10, + bodyChunked: true, + reqConnClose: true, + shouldDiscardBody: true, }, - // Big with Content-Length, so give up immediately if we know it's too big. + // Have: + // - Large body. + // - Content-Length defined. + // Should: + // - Not discard remaining body (we know it is too large from Content-Length). 4: { - bodySize: 1 << 20, - bodyChunked: false, // has a Content-Length - reqConnClose: false, - wantEOFSearch: false, - wantNextReq: false, + bodySize: 1 << 20, + bodyChunked: false, + reqConnClose: false, + shouldDiscardBody: false, }, - // Big chunked, so read a bit before giving up. + // Have: + // - Large body. + // - Chunked (no Content-Length defined). + // Should: + // - Discard remaining body (chunked, so we try up to a limit before giving up). 5: { - bodySize: 1 << 20, - bodyChunked: true, - reqConnClose: false, - wantEOFSearch: true, - wantNextReq: false, + bodySize: 1 << 20, + bodyChunked: true, + reqConnClose: false, + shouldDiscardBody: true, }, - // Big with Connection: close, but chunked, so search for trailers. - // TODO: maybe skip this search if no trailers were declared - // in the headers. + // Have: + // - Large body. + // - Content-Length defined. + // - Connection: close. + // Should: + // - Not discard remaining body (Connection: Close, and Content-Length is too large). 6: { - bodySize: 1 << 20, - bodyChunked: true, - reqConnClose: true, - wantEOFSearch: true, - wantNextReq: false, + bodySize: 1 << 20, + bodyChunked: false, + reqConnClose: true, + shouldDiscardBody: false, }, - - // Big with Connection: close, so don't do any reads on Close. - // With Content-Length. + // Have: + // - Large body. + // - Chunked (no Content-Length defined). + // - Connection: close. + // Should: + // - Discard remaining body (chunked, so it might have trailers). + // + // TODO: maybe skip this if no trailers were declared in the headers. 7: { - bodySize: 1 << 20, - bodyChunked: false, - reqConnClose: true, - wantEOFSearch: false, - wantNextReq: false, + bodySize: 1 << 20, + bodyChunked: true, + reqConnClose: true, + shouldDiscardBody: true, }, } -func TestHandlerBodyClose(t *testing.T) { +func TestBodyDiscard(t *testing.T) { setParallel(t) if testing.Short() && testenv.Builder() == "" { t.Skip("skipping in -short mode") } - for i, tt := range handlerBodyCloseTests { - testHandlerBodyClose(t, i, tt) + for i, tt := range bodyDiscardTests { + testBodyDiscard(t, i, tt) } } -func testHandlerBodyClose(t *testing.T, i int, tt handlerBodyCloseTest) { +func testBodyDiscard(t *testing.T, i int, tt bodyDiscardTest) { conn := new(testConn) body := strings.Repeat("x", tt.bodySize) if tt.bodyChunked { @@ -2275,12 +2294,12 @@ func testHandlerBodyClose(t *testing.T, i int, tt handlerBodyCloseTest) { cw.Close() conn.readBuf.WriteString("\r\n") } else { - conn.readBuf.Write([]byte(fmt.Sprintf( + conn.readBuf.Write(fmt.Appendf(nil, "POST / HTTP/1.1\r\n"+ "Host: test\r\n"+ tt.connectionHeader()+ "Content-Length: %d\r\n"+ - "\r\n", len(body)))) + "\r\n", len(body))) conn.readBuf.Write([]byte(body)) } if !tt.reqConnClose { @@ -2295,26 +2314,23 @@ func testHandlerBodyClose(t *testing.T, i int, tt handlerBodyCloseTest) { } ls := &oneConnListener{conn} - var numReqs int - var size0, size1 int + var initialSize, closedSize, exitSize int go Serve(ls, HandlerFunc(func(rw ResponseWriter, req *Request) { - numReqs++ - if numReqs == 1 { - size0 = readBufLen() - req.Body.Close() - size1 = readBufLen() - } + initialSize = readBufLen() + req.Body.Close() + closedSize = readBufLen() })) <-conn.closec - if numReqs < 1 || numReqs > 2 { - t.Fatalf("%d. bug in test. unexpected number of requests = %d", i, numReqs) + exitSize = readBufLen() + + if initialSize != closedSize { + t.Errorf("%d. Close() within request handler should be a no-op, but body size went from %d to %d", i, initialSize, closedSize) } - didSearch := size0 != size1 - if didSearch != tt.wantEOFSearch { - t.Errorf("%d. did EOF search = %v; want %v (size went from %d to %d)", i, didSearch, !didSearch, size0, size1) + if tt.shouldDiscardBody && closedSize <= exitSize { + t.Errorf("%d. want body content to be discarded upon request handler exit, but size went from %d to %d", i, closedSize, exitSize) } - if tt.wantNextReq && numReqs != 2 { - t.Errorf("%d. numReq = %d; want 2", i, numReqs) + if !tt.shouldDiscardBody && closedSize != exitSize { + t.Errorf("%d. want body content to not be discarded upon request handler exit, but size went from %d to %d", i, closedSize, exitSize) } } diff --git a/src/net/http/server.go b/src/net/http/server.go index 02554d1a20..5d8e576f71 100644 --- a/src/net/http/server.go +++ b/src/net/http/server.go @@ -1077,9 +1077,6 @@ func (c *conn) readRequest(ctx context.Context) (w *response, err error) { req.ctx = ctx req.RemoteAddr = c.remoteAddr req.TLS = c.tlsState - if body, ok := req.Body.(*body); ok { - body.doEarlyClose = true - } // Adjust the read deadline if necessary. if !hdrDeadline.Equal(wholeReqDeadline) { @@ -1711,9 +1708,11 @@ func (w *response) finishRequest() { w.conn.r.abortPendingRead() - // Close the body (regardless of w.closeAfterReply) so we can - // re-use its bufio.Reader later safely. - w.reqBody.Close() + // Try to discard the body (regardless of w.closeAfterReply), so we can + // potentially reuse it in the same connection. + if b, ok := w.reqBody.(*body); ok { + b.tryDiscardBody() + } if w.req.MultipartForm != nil { w.req.MultipartForm.RemoveAll() @@ -1741,16 +1740,16 @@ func (w *response) shouldReuseConnection() bool { return false } - if w.closedRequestBodyEarly() { + if w.didIncompleteDiscard() { return false } return true } -func (w *response) closedRequestBodyEarly() bool { +func (w *response) didIncompleteDiscard() bool { body, ok := w.req.Body.(*body) - return ok && body.didEarlyClose() + return ok && body.didIncompleteDiscard() } func (w *response) Flush() { @@ -2106,6 +2105,18 @@ func (c *conn) serve(ctx context.Context) { // But we're not going to implement HTTP pipelining because it // was never deployed in the wild and the answer is HTTP/2. inFlightResponse = w + // Ensure that Close() invocations within request handlers do not + // discard the body. + if b, ok := w.reqBody.(*body); ok { + b.mu.Lock() + b.inRequestHandler = true + b.mu.Unlock() + defer func() { + b.mu.Lock() + b.inRequestHandler = false + b.mu.Unlock() + }() + } serverHandler{c.server}.ServeHTTP(w, w.req) inFlightResponse = nil w.cancelCtx() @@ -2116,7 +2127,7 @@ func (c *conn) serve(ctx context.Context) { w.finishRequest() c.rwc.SetWriteDeadline(time.Time{}) if !w.shouldReuseConnection() { - if w.requestBodyLimitHit || w.closedRequestBodyEarly() { + if w.requestBodyLimitHit || w.didIncompleteDiscard() { c.closeWriteAndWait() } return diff --git a/src/net/http/transfer.go b/src/net/http/transfer.go index 675551287f..9b972aaa44 100644 --- a/src/net/http/transfer.go +++ b/src/net/http/transfer.go @@ -809,17 +809,17 @@ func fixTrailer(header Header, chunked bool) (Header, error) { // Close ensures that the body has been fully read // and then reads the trailer if necessary. type body struct { - src io.Reader - hdr any // non-nil (Response or Request) value means read trailer - r *bufio.Reader // underlying wire-format reader for the trailer - closing bool // is the connection to be closed after reading body? - doEarlyClose bool // whether Close should stop early - - mu sync.Mutex // guards following, and calls to Read and Close - sawEOF bool - closed bool - earlyClose bool // Close called and we didn't read to the end of src - onHitEOF func() // if non-nil, func to call when EOF is Read + src io.Reader + hdr any // non-nil (Response or Request) value means read trailer + r *bufio.Reader // underlying wire-format reader for the trailer + closing bool // is the connection to be closed after reading body? + + mu sync.Mutex // guards following, and calls to Read and Close + sawEOF bool + closed bool + incompleteDiscard bool // if true, we failed to discard the body content completely + inRequestHandler bool // used so Close() calls from within request handlers do not discard body + onHitEOF func() // if non-nil, func to call when EOF is Read } // ErrBodyReadAfterClose is returned when reading a [Request] or [Response] @@ -968,51 +968,69 @@ func (b *body) unreadDataSizeLocked() int64 { return -1 } +// shouldDiscardBodyLocked determines whether a body needs to discard its +// content or not. +// b.mu must be held. +func (b *body) shouldDiscardBodyLocked() bool { + // Already saw EOF. No point in discarding the body. + if b.sawEOF { + return false + } + // No trailer and closing the connection next. No point in discarding the + // body since it will not be reusable. + if b.hdr == nil && b.closing { + return false + } + return true +} + +// tryDiscardBody attempts to discard the body content. If the body cannot be +// discarded completely, b.incompleteDiscard will be set to true. +func (b *body) tryDiscardBody() { + b.mu.Lock() + defer b.mu.Unlock() + if !b.shouldDiscardBodyLocked() { + return + } + // Read up to maxPostHandlerReadBytes bytes of the body, looking for EOF + // (and trailers), so we can re-use this connection. + if lr, ok := b.src.(*io.LimitedReader); ok && lr.N > maxPostHandlerReadBytes { + // There was a declared Content-Length, and we have more bytes + // remaining than our maxPostHandlerReadBytes tolerance. So, give up. + b.incompleteDiscard = true + return + } + // Consume the body, which will also lead to us reading the trailer headers + // after the body, if present. + n, err := io.CopyN(io.Discard, bodyLocked{b}, maxPostHandlerReadBytes) + if err == io.EOF { + err = nil + } + if n == maxPostHandlerReadBytes || err != nil { + b.incompleteDiscard = true + } +} + func (b *body) Close() error { b.mu.Lock() defer b.mu.Unlock() if b.closed { return nil } - var err error - switch { - case b.sawEOF: - // Already saw EOF, so no need going to look for it. - case b.hdr == nil && b.closing: - // no trailer and closing the connection next. - // no point in reading to EOF. - case b.doEarlyClose: - // Read up to maxPostHandlerReadBytes bytes of the body, looking - // for EOF (and trailers), so we can re-use this connection. - if lr, ok := b.src.(*io.LimitedReader); ok && lr.N > maxPostHandlerReadBytes { - // There was a declared Content-Length, and we have more bytes remaining - // than our maxPostHandlerReadBytes tolerance. So, give up. - b.earlyClose = true - } else { - var n int64 - // Consume the body, or, which will also lead to us reading - // the trailer headers after the body, if present. - n, err = io.CopyN(io.Discard, bodyLocked{b}, maxPostHandlerReadBytes) - if err == io.EOF { - err = nil - } - if n == maxPostHandlerReadBytes { - b.earlyClose = true - } - } - default: - // Fully consume the body, which will also lead to us reading - // the trailer headers after the body, if present. - _, err = io.Copy(io.Discard, bodyLocked{b}) - } b.closed = true + if !b.shouldDiscardBodyLocked() || b.inRequestHandler { + return nil + } + // Fully consume the body, which will also lead to us reading + // the trailer headers after the body, if present. + _, err := io.Copy(io.Discard, bodyLocked{b}) return err } -func (b *body) didEarlyClose() bool { +func (b *body) didIncompleteDiscard() bool { b.mu.Lock() defer b.mu.Unlock() - return b.earlyClose + return b.incompleteDiscard } // bodyRemains reports whether future Read calls might @@ -1036,9 +1054,6 @@ type bodyLocked struct { } func (bl bodyLocked) Read(p []byte) (n int, err error) { - if bl.b.closed { - return 0, ErrBodyReadAfterClose - } return bl.b.readLocked(p) } -- cgit v1.3-6-g1900 From 2cf9d4b62f167cbef01469d625dabefdd783c0e8 Mon Sep 17 00:00:00 2001 From: "Nicholas S. Husin" Date: Tue, 18 Nov 2025 12:32:44 -0500 Subject: Revert "net/http: do not discard body content when closing it within request handlers" This reverts commit cb0d9980f5721715ebb73dd2e580eaa11c2ddee2. Reason for revert: the old behavior seems to be relied on by current users, e.g. https://github.com/connectrpc/connect-go/blob/cb2e11fb88c9a61804043355a619c12d4a30a1a5/protocol_connect.go#L837. For #75933 Change-Id: I996280238e5c70a8d760a0b31e3a13c6a44b8616 Reviewed-on: https://go-review.googlesource.com/c/go/+/721761 LUCI-TryBot-Result: Go LUCI Reviewed-by: Damien Neil Auto-Submit: Nicholas Husin Reviewed-by: Nicholas Husin --- src/net/http/serve_test.go | 188 +++++++++++++++++++++------------------------ src/net/http/server.go | 31 +++----- src/net/http/transfer.go | 109 ++++++++++++-------------- 3 files changed, 143 insertions(+), 185 deletions(-) (limited to 'src/net') diff --git a/src/net/http/serve_test.go b/src/net/http/serve_test.go index af15593c35..4a16ba02af 100644 --- a/src/net/http/serve_test.go +++ b/src/net/http/serve_test.go @@ -2150,137 +2150,118 @@ func TestServerUnreadRequestBodyLarge(t *testing.T) { } } -type bodyDiscardTest struct { +type handlerBodyCloseTest struct { bodySize int bodyChunked bool reqConnClose bool - shouldDiscardBody bool // should the handler discard body after it exits? + wantEOFSearch bool // should Handler's Body.Close do Reads, looking for EOF? + wantNextReq bool // should it find the next request on the same conn? } -func (t bodyDiscardTest) connectionHeader() string { +func (t handlerBodyCloseTest) connectionHeader() string { if t.reqConnClose { return "Connection: close\r\n" } return "" } -var bodyDiscardTests = [...]bodyDiscardTest{ - // Have: - // - Small body. - // - Content-Length defined. - // Should: - // - Discard remaining body. +var handlerBodyCloseTests = [...]handlerBodyCloseTest{ + // Small enough to slurp past to the next request + + // has Content-Length. 0: { - bodySize: 20 << 10, - bodyChunked: false, - reqConnClose: false, - shouldDiscardBody: true, + bodySize: 20 << 10, + bodyChunked: false, + reqConnClose: false, + wantEOFSearch: true, + wantNextReq: true, }, - // Have: - // - Small body. - // - Chunked (no Content-Length defined). - // Should: - // - Discard remaining body. + // Small enough to slurp past to the next request + + // is chunked. 1: { - bodySize: 20 << 10, - bodyChunked: true, - reqConnClose: false, - shouldDiscardBody: true, + bodySize: 20 << 10, + bodyChunked: true, + reqConnClose: false, + wantEOFSearch: true, + wantNextReq: true, }, - // Have: - // - Small body. - // - Content-Length defined. - // - Connection: close. - // Should: - // - Not discard remaining body (no point as Connection: close). + // Small enough to slurp past to the next request + + // has Content-Length + + // declares Connection: close (so pointless to read more). 2: { - bodySize: 20 << 10, - bodyChunked: false, - reqConnClose: true, - shouldDiscardBody: false, + bodySize: 20 << 10, + bodyChunked: false, + reqConnClose: true, + wantEOFSearch: false, + wantNextReq: false, }, - // Have: - // - Small body. - // - Chunked (no Content-Length defined). - // - Connection: close. - // Should: - // - Discard remaining body (chunked, so it might have trailers). - // - // TODO: maybe skip this if no trailers were declared in the headers. + // Small enough to slurp past to the next request + + // declares Connection: close, + // but chunked, so it might have trailers. + // TODO: maybe skip this search if no trailers were declared + // in the headers. 3: { - bodySize: 20 << 10, - bodyChunked: true, - reqConnClose: true, - shouldDiscardBody: true, + bodySize: 20 << 10, + bodyChunked: true, + reqConnClose: true, + wantEOFSearch: true, + wantNextReq: false, }, - // Have: - // - Large body. - // - Content-Length defined. - // Should: - // - Not discard remaining body (we know it is too large from Content-Length). + // Big with Content-Length, so give up immediately if we know it's too big. 4: { - bodySize: 1 << 20, - bodyChunked: false, - reqConnClose: false, - shouldDiscardBody: false, + bodySize: 1 << 20, + bodyChunked: false, // has a Content-Length + reqConnClose: false, + wantEOFSearch: false, + wantNextReq: false, }, - // Have: - // - Large body. - // - Chunked (no Content-Length defined). - // Should: - // - Discard remaining body (chunked, so we try up to a limit before giving up). + // Big chunked, so read a bit before giving up. 5: { - bodySize: 1 << 20, - bodyChunked: true, - reqConnClose: false, - shouldDiscardBody: true, + bodySize: 1 << 20, + bodyChunked: true, + reqConnClose: false, + wantEOFSearch: true, + wantNextReq: false, }, - // Have: - // - Large body. - // - Content-Length defined. - // - Connection: close. - // Should: - // - Not discard remaining body (Connection: Close, and Content-Length is too large). + // Big with Connection: close, but chunked, so search for trailers. + // TODO: maybe skip this search if no trailers were declared + // in the headers. 6: { - bodySize: 1 << 20, - bodyChunked: false, - reqConnClose: true, - shouldDiscardBody: false, + bodySize: 1 << 20, + bodyChunked: true, + reqConnClose: true, + wantEOFSearch: true, + wantNextReq: false, }, - // Have: - // - Large body. - // - Chunked (no Content-Length defined). - // - Connection: close. - // Should: - // - Discard remaining body (chunked, so it might have trailers). - // - // TODO: maybe skip this if no trailers were declared in the headers. + + // Big with Connection: close, so don't do any reads on Close. + // With Content-Length. 7: { - bodySize: 1 << 20, - bodyChunked: true, - reqConnClose: true, - shouldDiscardBody: true, + bodySize: 1 << 20, + bodyChunked: false, + reqConnClose: true, + wantEOFSearch: false, + wantNextReq: false, }, } -func TestBodyDiscard(t *testing.T) { +func TestHandlerBodyClose(t *testing.T) { setParallel(t) if testing.Short() && testenv.Builder() == "" { t.Skip("skipping in -short mode") } - for i, tt := range bodyDiscardTests { - testBodyDiscard(t, i, tt) + for i, tt := range handlerBodyCloseTests { + testHandlerBodyClose(t, i, tt) } } -func testBodyDiscard(t *testing.T, i int, tt bodyDiscardTest) { +func testHandlerBodyClose(t *testing.T, i int, tt handlerBodyCloseTest) { conn := new(testConn) body := strings.Repeat("x", tt.bodySize) if tt.bodyChunked { @@ -2294,12 +2275,12 @@ func testBodyDiscard(t *testing.T, i int, tt bodyDiscardTest) { cw.Close() conn.readBuf.WriteString("\r\n") } else { - conn.readBuf.Write(fmt.Appendf(nil, + conn.readBuf.Write([]byte(fmt.Sprintf( "POST / HTTP/1.1\r\n"+ "Host: test\r\n"+ tt.connectionHeader()+ "Content-Length: %d\r\n"+ - "\r\n", len(body))) + "\r\n", len(body)))) conn.readBuf.Write([]byte(body)) } if !tt.reqConnClose { @@ -2314,23 +2295,26 @@ func testBodyDiscard(t *testing.T, i int, tt bodyDiscardTest) { } ls := &oneConnListener{conn} - var initialSize, closedSize, exitSize int + var numReqs int + var size0, size1 int go Serve(ls, HandlerFunc(func(rw ResponseWriter, req *Request) { - initialSize = readBufLen() - req.Body.Close() - closedSize = readBufLen() + numReqs++ + if numReqs == 1 { + size0 = readBufLen() + req.Body.Close() + size1 = readBufLen() + } })) <-conn.closec - exitSize = readBufLen() - - if initialSize != closedSize { - t.Errorf("%d. Close() within request handler should be a no-op, but body size went from %d to %d", i, initialSize, closedSize) + if numReqs < 1 || numReqs > 2 { + t.Fatalf("%d. bug in test. unexpected number of requests = %d", i, numReqs) } - if tt.shouldDiscardBody && closedSize <= exitSize { - t.Errorf("%d. want body content to be discarded upon request handler exit, but size went from %d to %d", i, closedSize, exitSize) + didSearch := size0 != size1 + if didSearch != tt.wantEOFSearch { + t.Errorf("%d. did EOF search = %v; want %v (size went from %d to %d)", i, didSearch, !didSearch, size0, size1) } - if !tt.shouldDiscardBody && closedSize != exitSize { - t.Errorf("%d. want body content to not be discarded upon request handler exit, but size went from %d to %d", i, closedSize, exitSize) + if tt.wantNextReq && numReqs != 2 { + t.Errorf("%d. numReq = %d; want 2", i, numReqs) } } diff --git a/src/net/http/server.go b/src/net/http/server.go index 5d8e576f71..02554d1a20 100644 --- a/src/net/http/server.go +++ b/src/net/http/server.go @@ -1077,6 +1077,9 @@ func (c *conn) readRequest(ctx context.Context) (w *response, err error) { req.ctx = ctx req.RemoteAddr = c.remoteAddr req.TLS = c.tlsState + if body, ok := req.Body.(*body); ok { + body.doEarlyClose = true + } // Adjust the read deadline if necessary. if !hdrDeadline.Equal(wholeReqDeadline) { @@ -1708,11 +1711,9 @@ func (w *response) finishRequest() { w.conn.r.abortPendingRead() - // Try to discard the body (regardless of w.closeAfterReply), so we can - // potentially reuse it in the same connection. - if b, ok := w.reqBody.(*body); ok { - b.tryDiscardBody() - } + // Close the body (regardless of w.closeAfterReply) so we can + // re-use its bufio.Reader later safely. + w.reqBody.Close() if w.req.MultipartForm != nil { w.req.MultipartForm.RemoveAll() @@ -1740,16 +1741,16 @@ func (w *response) shouldReuseConnection() bool { return false } - if w.didIncompleteDiscard() { + if w.closedRequestBodyEarly() { return false } return true } -func (w *response) didIncompleteDiscard() bool { +func (w *response) closedRequestBodyEarly() bool { body, ok := w.req.Body.(*body) - return ok && body.didIncompleteDiscard() + return ok && body.didEarlyClose() } func (w *response) Flush() { @@ -2105,18 +2106,6 @@ func (c *conn) serve(ctx context.Context) { // But we're not going to implement HTTP pipelining because it // was never deployed in the wild and the answer is HTTP/2. inFlightResponse = w - // Ensure that Close() invocations within request handlers do not - // discard the body. - if b, ok := w.reqBody.(*body); ok { - b.mu.Lock() - b.inRequestHandler = true - b.mu.Unlock() - defer func() { - b.mu.Lock() - b.inRequestHandler = false - b.mu.Unlock() - }() - } serverHandler{c.server}.ServeHTTP(w, w.req) inFlightResponse = nil w.cancelCtx() @@ -2127,7 +2116,7 @@ func (c *conn) serve(ctx context.Context) { w.finishRequest() c.rwc.SetWriteDeadline(time.Time{}) if !w.shouldReuseConnection() { - if w.requestBodyLimitHit || w.didIncompleteDiscard() { + if w.requestBodyLimitHit || w.closedRequestBodyEarly() { c.closeWriteAndWait() } return diff --git a/src/net/http/transfer.go b/src/net/http/transfer.go index 9b972aaa44..675551287f 100644 --- a/src/net/http/transfer.go +++ b/src/net/http/transfer.go @@ -809,17 +809,17 @@ func fixTrailer(header Header, chunked bool) (Header, error) { // Close ensures that the body has been fully read // and then reads the trailer if necessary. type body struct { - src io.Reader - hdr any // non-nil (Response or Request) value means read trailer - r *bufio.Reader // underlying wire-format reader for the trailer - closing bool // is the connection to be closed after reading body? - - mu sync.Mutex // guards following, and calls to Read and Close - sawEOF bool - closed bool - incompleteDiscard bool // if true, we failed to discard the body content completely - inRequestHandler bool // used so Close() calls from within request handlers do not discard body - onHitEOF func() // if non-nil, func to call when EOF is Read + src io.Reader + hdr any // non-nil (Response or Request) value means read trailer + r *bufio.Reader // underlying wire-format reader for the trailer + closing bool // is the connection to be closed after reading body? + doEarlyClose bool // whether Close should stop early + + mu sync.Mutex // guards following, and calls to Read and Close + sawEOF bool + closed bool + earlyClose bool // Close called and we didn't read to the end of src + onHitEOF func() // if non-nil, func to call when EOF is Read } // ErrBodyReadAfterClose is returned when reading a [Request] or [Response] @@ -968,69 +968,51 @@ func (b *body) unreadDataSizeLocked() int64 { return -1 } -// shouldDiscardBodyLocked determines whether a body needs to discard its -// content or not. -// b.mu must be held. -func (b *body) shouldDiscardBodyLocked() bool { - // Already saw EOF. No point in discarding the body. - if b.sawEOF { - return false - } - // No trailer and closing the connection next. No point in discarding the - // body since it will not be reusable. - if b.hdr == nil && b.closing { - return false - } - return true -} - -// tryDiscardBody attempts to discard the body content. If the body cannot be -// discarded completely, b.incompleteDiscard will be set to true. -func (b *body) tryDiscardBody() { - b.mu.Lock() - defer b.mu.Unlock() - if !b.shouldDiscardBodyLocked() { - return - } - // Read up to maxPostHandlerReadBytes bytes of the body, looking for EOF - // (and trailers), so we can re-use this connection. - if lr, ok := b.src.(*io.LimitedReader); ok && lr.N > maxPostHandlerReadBytes { - // There was a declared Content-Length, and we have more bytes - // remaining than our maxPostHandlerReadBytes tolerance. So, give up. - b.incompleteDiscard = true - return - } - // Consume the body, which will also lead to us reading the trailer headers - // after the body, if present. - n, err := io.CopyN(io.Discard, bodyLocked{b}, maxPostHandlerReadBytes) - if err == io.EOF { - err = nil - } - if n == maxPostHandlerReadBytes || err != nil { - b.incompleteDiscard = true - } -} - func (b *body) Close() error { b.mu.Lock() defer b.mu.Unlock() if b.closed { return nil } - b.closed = true - if !b.shouldDiscardBodyLocked() || b.inRequestHandler { - return nil + var err error + switch { + case b.sawEOF: + // Already saw EOF, so no need going to look for it. + case b.hdr == nil && b.closing: + // no trailer and closing the connection next. + // no point in reading to EOF. + case b.doEarlyClose: + // Read up to maxPostHandlerReadBytes bytes of the body, looking + // for EOF (and trailers), so we can re-use this connection. + if lr, ok := b.src.(*io.LimitedReader); ok && lr.N > maxPostHandlerReadBytes { + // There was a declared Content-Length, and we have more bytes remaining + // than our maxPostHandlerReadBytes tolerance. So, give up. + b.earlyClose = true + } else { + var n int64 + // Consume the body, or, which will also lead to us reading + // the trailer headers after the body, if present. + n, err = io.CopyN(io.Discard, bodyLocked{b}, maxPostHandlerReadBytes) + if err == io.EOF { + err = nil + } + if n == maxPostHandlerReadBytes { + b.earlyClose = true + } + } + default: + // Fully consume the body, which will also lead to us reading + // the trailer headers after the body, if present. + _, err = io.Copy(io.Discard, bodyLocked{b}) } - // Fully consume the body, which will also lead to us reading - // the trailer headers after the body, if present. - _, err := io.Copy(io.Discard, bodyLocked{b}) + b.closed = true return err } -func (b *body) didIncompleteDiscard() bool { +func (b *body) didEarlyClose() bool { b.mu.Lock() defer b.mu.Unlock() - return b.incompleteDiscard + return b.earlyClose } // bodyRemains reports whether future Read calls might @@ -1054,6 +1036,9 @@ type bodyLocked struct { } func (bl bodyLocked) Read(p []byte) (n int, err error) { + if bl.b.closed { + return 0, ErrBodyReadAfterClose + } return bl.b.readLocked(p) } -- cgit v1.3-6-g1900 From a49b0302d0e1d97b67a5f3f3beceafdcbc4c2ef0 Mon Sep 17 00:00:00 2001 From: Damien Neil Date: Wed, 19 Nov 2025 11:25:49 -0800 Subject: net/http: correctly close fake net.Conns Fix an inverted test in fakeNetConn.Close that caused closing a connection to not break the other half of the connection. Change-Id: I4e53f78402f8e503c749d57f294a4524abdccfb5 Reviewed-on: https://go-review.googlesource.com/c/go/+/722220 Reviewed-by: Nicholas Husin Reviewed-by: Nicholas Husin Auto-Submit: Damien Neil LUCI-TryBot-Result: Go LUCI --- src/net/http/netconn_test.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) (limited to 'src/net') diff --git a/src/net/http/netconn_test.go b/src/net/http/netconn_test.go index 52b8069f8b..c5fd61289f 100644 --- a/src/net/http/netconn_test.go +++ b/src/net/http/netconn_test.go @@ -180,9 +180,10 @@ func (c *fakeNetConn) Close() error { c.loc.unlock() // Remote half of the connection reads EOF after reading any remaining data. c.rem.lock() - if c.rem.readErr != nil { + if c.rem.readErr == nil { c.rem.readErr = io.EOF } + c.rem.writeErr = net.ErrClosed c.rem.unlock() if c.autoWait { synctest.Wait() -- cgit v1.3-6-g1900 From ca37d24e0b9369b8086959df5bc230b38bf98636 Mon Sep 17 00:00:00 2001 From: Damien Neil Date: Wed, 19 Nov 2025 14:35:20 -0800 Subject: net/http: drop unused "broken" field from persistConn Change-Id: Ic65cf98c090c73299b5e88e642e91139315d8e52 Reviewed-on: https://go-review.googlesource.com/c/go/+/722221 Auto-Submit: Damien Neil LUCI-TryBot-Result: Go LUCI Reviewed-by: Nicholas Husin Reviewed-by: Nicholas Husin --- src/net/http/transport.go | 2 -- 1 file changed, 2 deletions(-) (limited to 'src/net') diff --git a/src/net/http/transport.go b/src/net/http/transport.go index a560765d33..4e6b07f34d 100644 --- a/src/net/http/transport.go +++ b/src/net/http/transport.go @@ -2110,7 +2110,6 @@ type persistConn struct { numExpectedResponses int closed error // set non-nil when conn is closed, before closech is closed canceledErr error // set non-nil if conn is canceled - broken bool // an error has happened on this connection; marked broken so it's not reused. reused bool // whether conn has had successful request/response and is being reused. // mutateHeaderFunc is an optional func to modify extra // headers on each outbound request before it's written. (the @@ -2925,7 +2924,6 @@ func (pc *persistConn) closeLocked(err error) { if err == nil { panic("nil error") } - pc.broken = true if pc.closed == nil { pc.closed = err pc.t.decConnsPerHost(pc.cacheKey) -- cgit v1.3-6-g1900