Never do socket I/O in finalizers; map ssl_mode onto real Connector/C options - #241
Merged
Conversation
… 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 Report❌ Patch coverage is
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. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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_STMTfinalizer calledmysql_stmt_close, which sendsCOM_STMT_CLOSEover the connection's socket. Finalizers run on whatever thread triggers GC — concurrently with an in-flightmysql_*call on another thread — and aMYSQL*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 oneSSL*, corrupting OpenSSL state:bad record macerrors, wedged connections, and the double-free aborts reported in #220 (crash reports show the GC-finalizer thread and amysql_committhread simultaneously insidema_tls_close → SSL_free).API.MYSQL_RES(mysql_free_resultreads un-fetched rows off the wire) andAPI.MYSQL(mysql_closesendsCOM_QUIT) had the same problem.The invariant this PR establishes: finalizers never touch the socket.
MYSQLnow owns a small reap queue (Threads.SpinLock+ twoVector{Ptr{Cvoid}}). Statement and result wrappers keep a reference to their parentMYSQL.MYSQL_STMT/MYSQL_RESfinalizers only park their raw handle on the queue (no libmariadb call) and are gone. If the connection is already closed, the statement finalizer callsmysql_stmt_closedirectly —mysql_closehas invalidated the handles, so that's a purely local free; an un-drained result in that state is leaked rather than read through a freedMYSQL*.API.reap!(mysql)drains the queue — it runs at the top ofclear!(conn), i.e. insideDBInterface.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.MYSQLfinalizer 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'strylock-or-re-register pattern, so a finalizer can never deadlock against a thread holding the reap lock.DBInterface.close!(stmt),DBInterface.close!(conn),clear!'s result cleanup) now call immediateAPI.close!/API.free!instead offinalize(...), preserving their old eager semantics; the still-registered finalizer no-ops onceptrisC_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):malloc: double freeinSSL_write/SSL_free) or SIGSEGV on every run within 1–4 twenty-second rounds, plus(2026): TLS/SSL error: ssl/tls alert bad record macrounds;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_modemapped onto options libmariadb actually has (#240)API.MYSQL_OPT_SSL_MODEdoes not exist in libmariadb — the enum entry's ordinal (7025) collided withMARIADB_OPT_SKIP_READ_RESPONSE, sossl_mode=SSL_MODE_DISABLEDwas a silent no-op and any other mode set skip-read-response to true, corrupting the protocol. The entry is removed frommysql_option(technically breaking for directAPIusers, but every possible use was a bug), and thessl_modekeyword now maps onto real Connector/C options:SSL_MODE_DISABLED@warn(libmariadb 3.4+ cannot disable TLS client-side; see #240)SSL_MODE_PREFERREDSSL_MODE_REQUIREDMYSQL_OPT_SSL_ENFORCE = trueSSL_MODE_VERIFY_CA/SSL_MODE_VERIFY_IDENTITYMYSQL_OPT_SSL_ENFORCE = true+MYSQL_OPT_SSL_VERIFY_SERVER_CERT = trueThe mapping runs after the
ssl_verify_server_cert/ssl_enforceblocks so an explicit mode wins over thessl_verify_server_cert=falsedefault from #235. Thessl_modekeyword is now documented in the connect docstring.One observation for a follow-up decision
While validating on current
mainI noticed that with the #235 defaultssl_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_cipheris empty by default, and non-empty withssl_verify_server_cert=true,ssl_enforce=true, orssl_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