From 0bbda5c728e7694ba929aa4fa0c549f1387d3a3f Mon Sep 17 00:00:00 2001 From: yangshenghui <3131207349@qq.com> Date: Sun, 16 Aug 2026 19:02:39 +0800 Subject: [PATCH] fix: close resources on error paths (#7940) --- caddytest/caddytest.go | 2 + cmd/packagesfuncs.go | 1 + cmd/packagesfuncs_test.go | 48 ++++++++++++ .../caddyhttp/reverseproxy/httptransport.go | 7 ++ .../reverseproxy/httptransport_test.go | 74 +++++++++++++++++++ modules/logging/filewriter.go | 10 ++- 6 files changed, 141 insertions(+), 1 deletion(-) create mode 100644 cmd/packagesfuncs_test.go diff --git a/caddytest/caddytest.go b/caddytest/caddytest.go index b2b0766e6..efafbe4a4 100644 --- a/caddytest/caddytest.go +++ b/caddytest/caddytest.go @@ -279,9 +279,11 @@ func validateTestPrerequisites(tc *Tester) error { return err } tc.t.Cleanup(func() { + _ = f.Close() os.Remove(f.Name()) //nolint:gosec // false positive, filename comes from std lib, no path traversal }) if _, err := fmt.Fprintf(f, initConfig, tc.config.AdminPort); err != nil { + _ = f.Close() return err } diff --git a/cmd/packagesfuncs.go b/cmd/packagesfuncs.go index a26919922..53e91f425 100644 --- a/cmd/packagesfuncs.go +++ b/cmd/packagesfuncs.go @@ -288,6 +288,7 @@ func downloadBuild(qs url.Values) (*http.Response, error) { return nil, fmt.Errorf("secure request failed: %v", err) } if resp.StatusCode >= 400 { + defer resp.Body.Close() var details struct { StatusCode int `json:"status_code"` Error struct { diff --git a/cmd/packagesfuncs_test.go b/cmd/packagesfuncs_test.go new file mode 100644 index 000000000..889303114 --- /dev/null +++ b/cmd/packagesfuncs_test.go @@ -0,0 +1,48 @@ +package caddycmd + +import ( + "bytes" + "io" + "net/http" + "net/url" + "testing" +) + +type trackingReadCloser struct { + *bytes.Reader + closed bool +} + +func (r *trackingReadCloser) Close() error { + r.closed = true + return nil +} + +func TestDownloadBuildClosesErrorResponseBody(t *testing.T) { + originalTransport := http.DefaultTransport + t.Cleanup(func() { http.DefaultTransport = originalTransport }) + + body := &trackingReadCloser{Reader: bytes.NewReader([]byte(`{"error":{"message":"bad request","id":"test"}}`))} + http.DefaultTransport = roundTripperFunc(func(req *http.Request) (*http.Response, error) { + return &http.Response{ + StatusCode: http.StatusBadRequest, + Body: body, + Request: req, + }, nil + }) + + if _, err := downloadBuild(url.Values{}); err == nil { + t.Fatal("downloadBuild succeeded, want error") + } + if !body.closed { + t.Fatal("downloadBuild did not close the error response body") + } +} + +type roundTripperFunc func(*http.Request) (*http.Response, error) + +func (f roundTripperFunc) RoundTrip(req *http.Request) (*http.Response, error) { + return f(req) +} + +var _ io.ReadCloser = (*trackingReadCloser)(nil) diff --git a/modules/caddyhttp/reverseproxy/httptransport.go b/modules/caddyhttp/reverseproxy/httptransport.go index d2645deed..bca3322f6 100644 --- a/modules/caddyhttp/reverseproxy/httptransport.go +++ b/modules/caddyhttp/reverseproxy/httptransport.go @@ -296,6 +296,12 @@ func (h *HTTPTransport) NewTransport(caddyCtx caddy.Context) (*http.Transport, e // decide whether to retry a request return nil, DialError{err} } + keepConn := false + defer func() { + if !keepConn { + _ = conn.Close() + } + }() if h.ProxyProtocol != "" { proxyProtocolInfo, ok := caddyhttp.GetVar(ctx, proxyProtocolInfoVarKey).(ProxyProtocolInfo) @@ -363,6 +369,7 @@ func (h *HTTPTransport) NewTransport(caddyCtx caddy.Context) (*http.Transport, e } } + keepConn = true return conn, nil } diff --git a/modules/caddyhttp/reverseproxy/httptransport_test.go b/modules/caddyhttp/reverseproxy/httptransport_test.go index f4b4bf30c..4f34f27c9 100644 --- a/modules/caddyhttp/reverseproxy/httptransport_test.go +++ b/modules/caddyhttp/reverseproxy/httptransport_test.go @@ -3,11 +3,15 @@ package reverseproxy import ( "context" "encoding/json" + "errors" "fmt" + "io" "net" + "net/netip" "net/url" "reflect" "testing" + "time" "github.com/caddyserver/caddy/v2" "github.com/caddyserver/caddy/v2/caddyconfig/caddyfile" @@ -198,6 +202,76 @@ func TestHTTPTransport_DialTLSContext_ProxyProtocol(t *testing.T) { } } +func TestHTTPTransport_DialContext_ProxyProtocolClosesConnectionOnError(t *testing.T) { + ctx, cancel := caddy.NewContext(caddy.Context{Context: context.Background()}) + defer cancel() + + validAddrPort := netip.MustParseAddrPort("192.0.2.1:1234") + tests := []struct { + name string + version string + proxyInfo *ProxyProtocolInfo + }{ + { + name: "missing proxy protocol info", + version: "v1", + }, + { + name: "unexpected proxy protocol version", + version: "v3", + proxyInfo: &ProxyProtocolInfo{AddrPort: validAddrPort}, + }, + { + name: "unexpected remote address type", + version: "v1", + proxyInfo: &ProxyProtocolInfo{}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + listener, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + t.Fatalf("listen: %v", err) + } + t.Cleanup(func() { listener.Close() }) + + readResult := make(chan error, 1) + go func() { + conn, err := listener.Accept() + if err != nil { + readResult <- err + return + } + defer conn.Close() + _ = conn.SetReadDeadline(time.Now().Add(2 * time.Second)) + var buf [1]byte + _, err = conn.Read(buf[:]) + readResult <- err + }() + + dialCtx := context.WithValue(context.Background(), caddyhttp.VarsCtxKey, make(map[string]any)) + if tt.proxyInfo != nil { + caddyhttp.SetVar(dialCtx, proxyProtocolInfoVarKey, *tt.proxyInfo) + } + + rt, err := (&HTTPTransport{ProxyProtocol: tt.version}).NewTransport(ctx) + if err != nil { + t.Fatalf("NewTransport: %v", err) + } + conn, err := rt.DialContext(dialCtx, "tcp", listener.Addr().String()) + if err == nil { + conn.Close() + t.Fatal("DialContext succeeded, want error") + } + + if readErr := <-readResult; !errors.Is(readErr, io.EOF) { + t.Fatalf("server read error = %v, want EOF after client connection close", readErr) + } + }) + } +} + // TestHTTPTransport_DialContext_DialInfoOverride is a regression test for // issue #6447: a `tcp4/`-prefixed upstream silently fell back to plain `tcp` // because dialContext only honored DialInfo for unix networks. PR #7300 widened diff --git a/modules/logging/filewriter.go b/modules/logging/filewriter.go index 2e19ec843..7f287ce42 100644 --- a/modules/logging/filewriter.go +++ b/modules/logging/filewriter.go @@ -224,9 +224,16 @@ func (fw FileWriter) OpenWriter() (io.WriteCloser, error) { if err != nil { return nil, err } + closeFile := true + defer func() { + if closeFile { + _ = file.Close() + } + }() info, err := file.Stat() if roll { - file.Close() // timberjack will reopen it on its own + _ = file.Close() // timberjack will reopen it on its own + closeFile = false } // Ensure already existing files have the right mode, since OpenFile will not set the mode in such case. @@ -244,6 +251,7 @@ func (fw FileWriter) OpenWriter() (io.WriteCloser, error) { // if not rolling, then the plain file handle is all we need if !roll { + closeFile = false return file, nil }