Skip to content

Never do socket I/O in finalizers; map ssl_mode onto real Connector/C options - #241

Merged
quinnj merged 1 commit into
mainfrom
jq/no-finalizer-io
Aug 2, 2026
Merged

Never do socket I/O in finalizers; map ssl_mode onto real Connector/C options#241
quinnj merged 1 commit into
mainfrom
jq/no-finalizer-io

Conversation

@quinnj

@quinnj quinnj commented Aug 2, 2026

Copy link
Copy Markdown
Member

Fixes #220. Fixes #240. Related: #234.

Two fixes, both rooted in the investigation written up in this comment on #220 and in #240.

1. Finalizers no longer do socket I/O (#220)

The API.MYSQL_STMT finalizer called mysql_stmt_close, which sends COM_STMT_CLOSE over the connection's socket. Finalizers run on whatever thread triggers GC — concurrently with an in-flight mysql_* call on another thread — and a MYSQL* is not thread-safe, so an abandoned statement's finalizer raced legitimate, even lock-serialized, use of the same connection. Under TLS the two threads interleave inside one SSL*, corrupting OpenSSL state: bad record mac errors, wedged connections, and the double-free aborts reported in #220 (crash reports show the GC-finalizer thread and a mysql_commit thread simultaneously inside ma_tls_close → SSL_free). API.MYSQL_RES (mysql_free_result reads un-fetched rows off the wire) and API.MYSQL (mysql_close sends COM_QUIT) had the same problem.

The invariant this PR establishes: finalizers never touch the socket.

  • MYSQL now owns a small reap queue (Threads.SpinLock + two Vector{Ptr{Cvoid}}). Statement and result wrappers keep a reference to their parent MYSQL.
  • The MYSQL_STMT/MYSQL_RES finalizers only park their raw handle on the queue (no libmariadb call) and are gone. If the connection is already closed, the statement finalizer calls mysql_stmt_close directly — mysql_close has invalidated the handles, so that's a purely local free; an un-drained result in that state is leaked rather than read through a freed MYSQL*.
  • API.reap!(mysql) drains the queue — it runs at the top of clear!(conn), i.e. inside DBInterface.prepare/DBInterface.execute/cursor operations, which the caller already serializes with all other use of the connection. The socket-touching closes happen there, outside the spinlock.
  • The MYSQL finalizer performs the full teardown (free parked results, close parked statements, mysql_close) — safe because an unreachable connection wrapper has no in-flight calls. All lock acquisition in finalizers uses the manual's trylock-or-re-register pattern, so a finalizer can never deadlock against a thread holding the reap lock.
  • Explicit paths (DBInterface.close!(stmt), DBInterface.close!(conn), clear!'s result cleanup) now call immediate API.close!/API.free! instead of finalize(...), preserving their old eager semantics; the still-registered finalizer no-ops once ptr is C_NULL.

Validation

The reproducer from the #220 comment (6 tasks doing prepared-statement work behind one ReentrantLock, one thread applying GC pressure, -t 8, mysql:8 in Docker, TLS on):

  • before: SIGABRT (malloc: double free in SSL_write/SSL_free) or SIGSEGV on every run within 1–4 twenty-second rounds, plus (2026): TLS/SSL error: ssl/tls alert bad record mac rounds;
  • after: 8/8 rounds clean with TLS confirmed active (TLS_AES_256_GCM_SHA384), no errors of any kind.

New tests assert that abandoned statements are parked (not closed mid-GC) and reaped by the next operation, that closing a connection with parked handles is safe, and (when JULIA_NUM_THREADS > 1) run a 5-second lock-serialized concurrency smoke test that aborts the process on the old code.

2. ssl_mode mapped onto options libmariadb actually has (#240)

API.MYSQL_OPT_SSL_MODE does not exist in libmariadb — the enum entry's ordinal (7025) collided with MARIADB_OPT_SKIP_READ_RESPONSE, so ssl_mode=SSL_MODE_DISABLED was a silent no-op and any other mode set skip-read-response to true, corrupting the protocol. The entry is removed from mysql_option (technically breaking for direct API users, but every possible use was a bug), and the ssl_mode keyword now maps onto real Connector/C options:

mode effect
SSL_MODE_DISABLED @warn (libmariadb 3.4+ cannot disable TLS client-side; see #240)
SSL_MODE_PREFERRED no-op (Connector/C default)
SSL_MODE_REQUIRED MYSQL_OPT_SSL_ENFORCE = true
SSL_MODE_VERIFY_CA / SSL_MODE_VERIFY_IDENTITY MYSQL_OPT_SSL_ENFORCE = true + MYSQL_OPT_SSL_VERIFY_SERVER_CERT = true

The mapping runs after the ssl_verify_server_cert/ssl_enforce blocks so an explicit mode wins over the ssl_verify_server_cert=false default from #235. The ssl_mode keyword is now documented in the connect docstring.

One observation for a follow-up decision

While validating on current main I noticed that with the #235 default ssl_verify_server_cert=false, connections to a TLS-capable server negotiate plaintext (libmariadb 3.4's TLS-by-default only engages when certificate verification is on — verified empirically: Ssl_cipher is empty by default, and non-empty with ssl_verify_server_cert=true, ssl_enforce=true, or ssl_mode=SSL_MODE_REQUIRED). This PR leaves that default untouched, but it may be worth calling out in the README/docs since 1.5.1 behavior (TLS on by default with the 3.4 jll) differs.

Version bumped to 1.5.3.

🤖 Generated with Claude Code

… options

MYSQL_STMT/MYSQL_RES finalizers sent COM_STMT_CLOSE / read pending rows over
the connection's socket from whatever thread triggered GC, racing in-flight
mysql_* calls on other threads and corrupting TLS state (double-free aborts,
bad record mac). Finalizers now park raw handles on a connection-owned reap
queue drained inside the next user-initiated (caller-serialized) operation;
connection teardown drains the queue before mysql_close.

MYSQL_OPT_SSL_MODE does not exist in libmariadb (its ordinal collided with
MARIADB_OPT_SKIP_READ_RESPONSE); the ssl_mode keyword is now mapped onto
MYSQL_OPT_SSL_ENFORCE / MYSQL_OPT_SSL_VERIFY_SERVER_CERT, with a warning for
the unimplementable SSL_MODE_DISABLED.

Fixes #220
Fixes #240

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@codecov

codecov Bot commented Aug 2, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 97.11538% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 71.50%. Comparing base (d69e2d6) to head (1ccda90).

Files with missing lines Patch % Lines
src/api/apitypes.jl 96.51% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #241      +/-   ##
==========================================
+ Coverage   69.89%   71.50%   +1.60%     
==========================================
  Files          10       10              
  Lines        1186     1260      +74     
==========================================
+ Hits          829      901      +72     
- Misses        357      359       +2     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@quinnj
quinnj merged commit 6144a81 into main Aug 2, 2026
6 checks passed
@quinnj
quinnj deleted the jq/no-finalizer-io branch August 2, 2026 05:35
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.

ssl_mode maps to libmariadb's MARIADB_OPT_SKIP_READ_RESPONSE: SSL_MODE_DISABLED is a no-op, other modes corrupt the protocol Double free in mariadb

1 participant