fix: close resources on error paths (#7940)

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
 		}
 

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 {

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)

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
 	}
 

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

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
 	}