Skip to content

fluent: fix panic on failing TLS connect - #144

Merged
cosmo0920 merged 1 commit into
fluent:masterfrom
mrkamel:fix-tls-connect-typed-nil
Oct 2, 2026
Merged

cosmo0920 merged 1 commit into
fluent:masterfrom
mrkamel:fix-tls-connect-typed-nil

Conversation

@mrkamel

@mrkamel mrkamel commented Oct 1, 2026

Copy link
Copy Markdown

Fix for #119 supported by Claude Code

Problem

tls.DialWithDialer returns a concrete *tls.Conn, so assigning it
straight to f.conn stored a typed nil on failure: f.conn != nil
held while the pointer behind it was nil. Two nil derefs followed, both
reachable by pointing a tls logger at an endpoint that's down:

  • close() called Close() on the nil *tls.Conn.
  • syncConnectWithRetry skipped the reconnect, so syncWriteMessage
    dereferenced it.

Fix

Dial into a local in each branch and assign f.conn only after the dial
succeeds; on error connect() returns early without touching it. The
old implicit "clear f.conn on failure" was dead — all three callers
already enter with it nil. latestReconnectTime is unchanged. The
tcp/unix branches are retargeted for consistency only; they were
never affected.

Testing

TestNoPanicOnFailingTLSConnect points a sync TLS logger at a closed
loopback port and covers both crashes (two Posts, deferred Close).
It panics on the old code, passes with the fix. go test ./fluent/ -race -count=1 passes.

@mrkamel
mrkamel force-pushed the fix-tls-connect-typed-nil branch from e55e47d to b818fd5 Compare October 1, 2026 08:37
Dial into a local in each connect branch and assign f.conn only once the
dial has succeeded.

Fixes fluent#119

Signed-off-by: Benjamin Vetter <benjamin.vetter@nymcard.com>
@mrkamel
mrkamel force-pushed the fix-tls-connect-typed-nil branch from b818fd5 to a5b870f Compare October 1, 2026 08:42
@thaJeztah

Copy link
Copy Markdown
Contributor

cc @cosmo0920 ptal

@cosmo0920

Copy link
Copy Markdown
Contributor

Thanks. It's reasonable change to me.

@cosmo0920
cosmo0920 merged commit 36b2de2 into fluent:master Oct 2, 2026
10 checks passed
@thaJeztah

Copy link
Copy Markdown
Contributor

Doh! Meant to comment on the PR, but did on the ticket 🙈

Thanks @cosmo0920 ! Any chance on a new patch release? Then we could be able to close out moby/moby#47709 on our side

I opened one minor PR to remove uses of the deprecated io/ioutil, and go fix, which should probably be safe to include as well;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants