fix: self-connect fd leak, udp callback panic, and mTLS that does not verify clients (#123, #125, #127) by NeverENG · Pull Request #145 · AlexStocks/getty

NeverENG · 2026-09-12T16:04:20Z

Fixes #123. Fixes #125. Fixes #127.

Three code-review findings, all small, all silent from the caller's side, each with a regression test that fails on master.

#123 `server.accept()` leaked the fd of a self-connect

if gxnet.IsSameAddr(conn.RemoteAddr(), conn.LocalAddr()) {
	log.Warnf("conn.localAddr{%s} == conn.RemoteAddr{%s}", ...)
	return nil, perrors.WithStack(errSelfConnect)   // conn was never closed
}

The connection had already been accepted, so it owns a descriptor; the accept loop just continues, and that descriptor leaks for the life of the process. The client side closes before reporting errSelfConnect; the server side now does too.

#125 two of the three process-killing panics

udp endpoint, newSession callback error (runUDPEventLoop):

if err = newSession(ss); err != nil {
	_ = conn.Close()
	panic(err.Error())      // inside a goroutine RunEventLoop spawned
}

A panic in a goroutine the library started cannot be recovered by any caller, so a transient error in a user callback (dependency not ready, validation failure) took the process down. It now logs and stops serving, which is what the tcp accept path does.

client with sslEnabled and no tlsConfigBuilder: dialTCP calls c.tlsConfigBuilder.BuildTlsConfig(), and dialTCP runs inside the reconnect goroutine. newClient only validated number/addr, so the combination surfaced as a nil-pointer panic from an unrelated place. It is now rejected in newClient, synchronously, before anything is started:

panic: client type:TCP_CLIENT, sslEnabled is true but no tlsConfigBuilder was supplied; use WithClientTlsConfigBuilder

The third item of #125 (the WSS Serve panic) is already fixed on master - the current code logs and skips http.ErrServerClosed instead of panicking - so it was not touched here.

#127 mTLS was configuration theatre

config.ClientCAs = certPool
config.ClientAuth = tls.RequireAnyClientCert      // demands a cert, verifies nothing

crypto/tls only consults ClientCAs from VerifyClientCertIfGiven upwards, so with a trust collection configured certPool was dead configuration: any self-signed certificate completed the handshake. It is now tls.RequireAndVerifyClientCert, which is what the WSS server path already used. The same builder also left MinVersion at 0 while the client builder has always set TLS 1.2, so the server accepted TLS 1.0/1.1; that is fixed too.

Tests

All five fail on master:

--- FAIL: TestAcceptClosesSelfConnect
    accept() left the self-connect connection open: the fd is leaked

--- FAIL: TestUDPNewSessionErrorDoesNotPanic
    panic: callback failed                     (the goroutine panic took the test binary down)

--- FAIL: TestNewClientSSLRequiresTLSConfigBuilder
    NewTCPClient(sslEnabled, no tlsConfigBuilder) did not panic

--- FAIL: TestServerTLSConfigBuilderVerifiesClientCertificate
    ClientAuth = RequireAnyClientCert, want RequireAndVerifyClientCert: ClientCAs is not consulted otherwise

--- FAIL: TestServerTLSConfigBuilderWithoutTrustCollection
    MinVersion = 0, want TLS 1.2 (771)

go test ./transport -race, make test, make check-fmt and make lint are green on this branch.

Summary by CodeRabbit

Bug Fixes
- Invalid TLS client configurations now fail immediately with a clear error.
- TCP self-connections are closed properly to prevent resource leaks.
- UDP session initialization errors no longer crash the process and instead shut down cleanly.
Security
- TLS connections now require TLS 1.2 or later.
- Configured client certificates are fully verified against the trusted certificate collection.

…g the process, and verify mTLS clients Three findings from code review, all in the accept / TLS paths. AlexStocks#123 server.accept() detected a self-connect, logged it and returned without closing the connection it had just accepted. The accept loop simply continues, so that descriptor leaked for the life of the process. The client side already closes before it reports errSelfConnect. AlexStocks#125 (two of its three items; the WSS Serve panic was already gone from master) - a udp endpoint's newSession callback returning an error panicked inside the goroutine RunEventLoop spawned, and nothing can recover a panic there, so a transient error in a user callback killed the process. It now logs and stops serving, like the tcp accept path does. - sslEnabled without a tlsConfigBuilder reached dialTCP, which runs inside the reconnect goroutine, turning a configuration mistake into a nil-pointer panic reported from somewhere else entirely. newClient rejects that combination up front, naming the option that is missing. AlexStocks#127 with a trust collection configured, ServerTlsConfigBuilder still asked for a client certificate with RequireAnyClientCert - which demands a certificate but verifies nothing - so ClientCAs was dead configuration and any self-signed certificate completed the handshake. It now requires and verifies against the collection, the way the WSS server path already did, and the server config gets the TLS 1.2 floor the client builder has always had. Five regression tests, every one of them failing on master: TestAcceptClosesSelfConnect accept() left the self-connect connection open: the fd is leaked TestUDPNewSessionErrorDoesNotPanic panic: callback failed (the goroutine panic took the test binary down) TestNewClientSSLRequiresTLSConfigBuilder NewTCPClient(sslEnabled, no tlsConfigBuilder) did not panic TestServerTLSConfigBuilderVerifiesClientCertificate ClientAuth = RequireAnyClientCert, want RequireAndVerifyClientCert TestServerTLSConfigBuilderWithoutTrustCollection MinVersion = 0, want TLS 1.2

coderabbitai · 2026-09-12T16:04:30Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: d5735ee0-dadf-46a2-b629-2439d2c5b037

📥 Commits

Reviewing files that changed from the base of the PR and between 4207b65 and c590f5e.

📒 Files selected for processing (6)

transport/client.go
transport/client_test.go
transport/server.go
transport/server_test.go
transport/tls.go
transport/tls_test.go

Included review availability: Your plan provides up to 4 included reviews per hour; 2 remain after this review.

📝 Walkthrough

Walkthrough

The transport layer now validates missing TLS client builders, closes self-connect connections, handles UDP session errors without panics, and enforces stronger server TLS settings. Regression tests cover each behavior.

Changes

Transport reliability and TLS security

Layer / File(s)	Summary
Client TLS configuration validation `transport/client.go`, `transport/client_test.go`	SSL client construction now panics synchronously when `tlsConfigBuilder` is missing. The regression test checks the panic message.
Server connection and callback error handling `transport/server.go`, `transport/server_test.go`	Self-connect connections are closed before returning `errSelfConnect`. UDP session initialization errors are logged, the connection is closed, and the event loop exits without panicking.
Server TLS verification and protocol floor `transport/tls.go`, `transport/tls_test.go`	Server TLS now requires TLS 1.2 or newer. Configured trust collections use `RequireAndVerifyClientCert`; servers without one retain `RequireAnyClientCert`.

Priority: ⬆️ High

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix · Severity of issue fixed: High

Suggested reviewers: alexstocks

Merge Risk: ⚪ Minimal · up to c590f

The transport fixes and TLS hardening changes are covered by regression tests, with no concrete unresolved merge risk identified.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 9 functions across 6 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarizes the primary fixes: the self-connect file descriptor leak, the UDP callback panic, and client certificate verification for mTLS. It is specific and directly related to the …
Linked Issues check	✅ Passed	The changes satisfy the coding requirements in [`#123`], [`#125`], and [`#127`]. `server.accept` closes self-connect connections before returning `errSelfConnect`. UDP session callback errors now log, close…
Out of Scope Changes check	✅ Passed	The changed production files implement the three linked issues. The added test helpers and regression tests directly verify the required behavior. No unrelated production behavior or unrelated files a…

Fix all pre-merge checks with AI

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

Navigation Menu

fix: self-connect fd leak, udp callback panic, and mTLS that does not verify clients (#123, #125, #127) - #145

fix: self-connect fd leak, udp callback panic, and mTLS that does not verify clients (#123, #125, #127)#145
NeverENG wants to merge 1 commit into
AlexStocks:masterfrom
NeverENG:fix/misc-123-125-127

NeverENG commented Sep 12, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Sep 12, 2026 •

edited

Loading

Walkthrough

Changes

❌ Failed checks (1 warning)

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

NeverENG commented Sep 12, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

#123 server.accept() leaked the fd of a self-connect

#125 two of the three process-killing panics

#127 mTLS was configuration theatre

Tests

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Sep 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

❌ Failed checks (1 warning)

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

NeverENG commented Sep 12, 2026 •

edited by coderabbitai Bot

Loading

#123 `server.accept()` leaked the fd of a self-connect

coderabbitai Bot commented Sep 12, 2026 •

edited

Loading