Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 11 additions & 5 deletions conn.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,11 +22,8 @@ import (
"github.com/ClickHouse/clickhouse-go/v2/lib/proto"
)

func dial(ctx context.Context, addr string, num int, opt *Options) (*connect, error) {
var (
err error
conn net.Conn
)
func dial(ctx context.Context, addr string, num int, opt *Options) (c *connect, err error) {
var conn net.Conn

switch {
case opt.DialContext != nil:
Expand All @@ -44,6 +41,15 @@ func dial(ctx context.Context, addr string, num int, opt *Options) (*connect, er
return nil, err

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Nit — dial-error return still leaks if a custom DialContext returns both a conn and an error

This return sits above the new defer, so a user-supplied opt.DialContext that returns a non-nil net.Conn alongside a non-nil error still leaks the fd — the exact failure mode this PR closes everywhere else in the function.

Stdlib dialers never do this, so it's defensive only, but it's two lines and it makes the guarantee unconditional.

Suggested change
return nil, err
if conn != nil {
_ = conn.Close()
}
return nil, err

}

// Close the freshly dialed connection if any later setup step (compression,
// JWT, handshake, addendum) fails, otherwise the socket and its fd leak on
// every failed connection attempt.
defer func() {
if err != nil {
_ = conn.Close()
}
}()

// Get base logger and enrich with connection-specific context
baseLogger := opt.logger()
logger := prepareConnLogger(baseLogger, num, conn.RemoteAddr().String(), "native")
Expand Down
42 changes: 42 additions & 0 deletions conn_fd_leak_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
package clickhouse

import (
"context"
"net"
"sync/atomic"
"testing"

"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)

type closeTrackingConn struct {
net.Conn
closed *atomic.Bool
}

func (c *closeTrackingConn) Close() error {
c.closed.Store(true)

return c.Conn.Close()
}

// TestDialClosesConnectionOnSetupFailure ensures dial does not leak the underlying
// socket (and its file descriptor) when a post-dial setup step fails. The server
// side of the pipe is closed up front so the handshake fails.
func TestDialClosesConnectionOnSetupFailure(t *testing.T) {
client, server := net.Pipe()
require.NoError(t, server.Close())

var closed atomic.Bool
tracked := &closeTrackingConn{Conn: client, closed: &closed}

_, err := dial(context.Background(), "127.0.0.1:9000", 1, &Options{
DialContext: func(_ context.Context, _ string) (net.Conn, error) {
return tracked, nil
},
})

require.Error(t, err)
assert.True(t, closed.Load(), "dial must close the connection when a post-dial setup step fails")
}
Loading