From 1ccda907157ab14e6df2cb188c22d36fefdb1f8b Mon Sep 17 00:00:00 2001 From: Jacob Quinn Date: Sat, 1 Aug 2026 19:14:55 -0600 Subject: [PATCH] Never do socket I/O in finalizers; map ssl_mode onto real Connector/C 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 --- Project.toml | 2 +- src/MySQL.jl | 37 +++++++-- src/api/apitypes.jl | 195 +++++++++++++++++++++++++++++++++++++++----- src/api/capi.jl | 4 +- src/api/consts.jl | 9 +- src/api/papi.jl | 4 +- src/execute.jl | 2 +- src/prepare.jl | 2 +- test/runtests.jl | 81 ++++++++++++++++++ 9 files changed, 297 insertions(+), 39 deletions(-) diff --git a/Project.toml b/Project.toml index 1a1054f..1ad789c 100644 --- a/Project.toml +++ b/Project.toml @@ -1,7 +1,7 @@ name = "MySQL" uuid = "39abe10b-433b-5dbd-92d4-e302a9df00cd" author = ["quinnj"] -version = "1.5.2" +version = "1.5.3" [deps] DBInterface = "a10d1c49-ce27-4219-8d33-6db1a4562965" diff --git a/src/MySQL.jl b/src/MySQL.jl index 05bab5a..9d4762a 100644 --- a/src/MySQL.jl +++ b/src/MySQL.jl @@ -47,6 +47,10 @@ end @noinline checkconn(conn::Connection) = conn.mysql.ptr == C_NULL && error("mysql connection has been closed or disconnected") function clear!(conn) + # close any statement/result handles abandoned to the GC; we're in a + # user-initiated operation here, so this is serialized with all other use + # of the connection (see API.reap!) + API.reap!(conn.mysql) conn.lastexecute === nothing || clear!(conn, conn.lastexecute) return end @@ -56,7 +60,7 @@ function clear!(conn, result::API.MYSQL_RES) while true if API.fetchrow(conn.mysql, result) == C_NULL if API.moreresults(conn.mysql) - finalize(result) + API.free!(result) @assert API.nextresult(conn.mysql) !== nothing result = API.useresult(conn.mysql) else @@ -64,7 +68,7 @@ function clear!(conn, result::API.MYSQL_RES) end end end - finalize(result) + API.free!(result) end return end @@ -202,9 +206,6 @@ function setoptions!(mysql; if ssl_crlpath !== nothing API.setoption(mysql, API.MYSQL_OPT_SSL_CRLPATH, ssl_crlpath) end - if ssl_mode !== nothing - API.setoption(mysql, API.MYSQL_OPT_SSL_MODE, ssl_mode) - end if passphrase !== nothing API.setoption(mysql, API.MARIADB_OPT_TLS_PASSPHRASE, passphrase) end @@ -214,6 +215,26 @@ function setoptions!(mysql; if ssl_enforce !== nothing API.setoption(mysql, API.MYSQL_OPT_SSL_ENFORCE, ssl_enforce) end + if ssl_mode !== nothing + # libmariadb has no MYSQL_OPT_SSL_MODE: the enum entry MySQL.jl used to + # pass for it collided with MARIADB_OPT_SKIP_READ_RESPONSE, making this + # kwarg a silent no-op at best (#240). Map the requested mode onto + # options Connector/C does understand. This block runs after the + # ssl_enforce / ssl_verify_server_cert blocks so an explicit mode wins. + if ssl_mode == API.SSL_MODE_DISABLED + @warn """ssl_mode=SSL_MODE_DISABLED cannot be honored: MariaDB Connector/C 3.4+ \ + always negotiates TLS when the server supports it and offers no client-side way \ + to disable it. The connection will use TLS whenever the server offers it, and \ + falls back to plaintext only against a server with TLS disabled (which requires \ + ssl_verify_server_cert=false, the default).""" maxlog=1 + elseif ssl_mode == API.SSL_MODE_REQUIRED + API.setoption(mysql, API.MYSQL_OPT_SSL_ENFORCE, true) + elseif ssl_mode == API.SSL_MODE_VERIFY_CA || ssl_mode == API.SSL_MODE_VERIFY_IDENTITY + API.setoption(mysql, API.MYSQL_OPT_SSL_ENFORCE, true) + API.setoption(mysql, API.MYSQL_OPT_SSL_VERIFY_SERVER_CERT, true) + end + # SSL_MODE_PREFERRED is the Connector/C default; nothing to set + end if default_auth !== nothing API.setoption(mysql, API.MYSQL_DEFAULT_AUTH, default_auth) end @@ -281,6 +302,7 @@ Connect to a MySQL database with provided `host`, `user`, and `passwd` positiona * `ssl_crlpath::AbstractString`: Defines a path to a directory that contains one or more PEM files that should each contain one revoked X509 certificate to use for TLS. This option requires that you use the absolute path, not a relative path. The directory specified by this option needs to be run through the openssl rehash command. * `ssl_verify_server_cert::Bool=false`: Enables (or disables) server certificate verification. * `ssl_enforce::Bool`: Whether to force TLS + * `ssl_mode::MySQL.API.mysql_ssl_mode`: MySQL-Connector-style TLS mode, mapped onto the options libmariadb understands: `SSL_MODE_REQUIRED` forces TLS, `SSL_MODE_VERIFY_CA`/`SSL_MODE_VERIFY_IDENTITY` force TLS with server certificate verification, `SSL_MODE_PREFERRED` is the default behavior. `SSL_MODE_DISABLED` cannot be honored (libmariadb 3.4+ always uses TLS when the server offers it) and logs a warning. * `default_auth::AbstractString`: Default authentication client-side plugin to use. * `connection_handler::AbstractString`: Specify the name of a connection handler plugin. * `plugin_dir::AbstractString`: Specify the location of client plugins. The plugin directory can also be specified with the MARIADB_PLUGIN_DIR environment variable. @@ -300,10 +322,7 @@ DBInterface.connect(::Type{Connection}, host::AbstractString, user::AbstractStri Close a `MySQL.Connection` opened by `DBInterface.connect`. """ function DBInterface.close!(conn::Connection) - if conn.mysql.ptr != C_NULL - API.mysql_close(conn.mysql.ptr) - conn.mysql.ptr = C_NULL - end + API.close!(conn.mysql) return end diff --git a/src/api/apitypes.jl b/src/api/apitypes.jl index 12e122d..718444e 100644 --- a/src/api/apitypes.jl +++ b/src/api/apitypes.jl @@ -8,38 +8,160 @@ Base.showerror(io::IO, e::Error) = print(io, "($(e.errno)): $(e.msg)") # wraps a MYSQL opaque pointer mutable struct MYSQL ptr::Ptr{Cvoid} + # Statement/result handles whose Julia wrappers were garbage-collected before + # being explicitly closed. mysql_stmt_close and mysql_free_result are not + # client-side frees: they can write to / read from the connection's socket. + # Finalizers run on whatever thread happens to trigger GC — concurrently with + # an in-flight mysql_* call on another thread — and a MYSQL* is not + # thread-safe, so finalizers must never call into libmariadb on a live + # connection (https://github.com/JuliaDatabases/MySQL.jl/issues/220). They + # park raw handles here instead; reap!() closes them from inside the next + # user-initiated operation, which the caller already serializes with all + # other use of the connection. + reaplock::Threads.SpinLock # guards the two vectors below and `closed` + stmts_to_close::Vector{Ptr{Cvoid}} + results_to_free::Vector{Ptr{Cvoid}} + closed::Bool # set once mysql_close has run function MYSQL(ptr) ptr == C_NULL && error("error creating API.MYSQL structure; null pointer encountered; probably insufficient memory available") - mysql = new(ptr) - finalizer(mysql) do x - if x.ptr != C_NULL - mysql_close(x.ptr) - x.ptr = C_NULL - end - end + mysql = new(ptr, Threads.SpinLock(), Ptr{Cvoid}[], Ptr{Cvoid}[], false) + finalizer(finalize_mysql, mysql) return mysql end end Error(mysql::MYSQL) = Error(mysql.ptr) +# Runs with x.reaplock held. Frees parked results first (flushing an un-drained +# result reads from the socket, which needs the connection alive), then parked +# statements, then the connection itself. +function _teardown(x::MYSQL) + if x.ptr != C_NULL + for p in x.results_to_free + mysql_free_result(p) + end + empty!(x.results_to_free) + for p in x.stmts_to_close + mysql_stmt_close(p) + end + empty!(x.stmts_to_close) + mysql_close(x.ptr) + x.ptr = C_NULL + end + x.closed = true + return +end + +# GC finalizer for MYSQL. If the wrapper is unreachable no user call on this +# connection can be in flight, so the teardown I/O is single-threaded and safe. +# Finalizers may only trylock: if the lock is busy (another thread is mid-reap!), +# re-register and retry at a later GC — the pattern from the Julia manual for +# finalizers that need locks. +function finalize_mysql(x::MYSQL) + if trylock(x.reaplock) + try + _teardown(x) + finally + unlock(x.reaplock) + end + else + finalizer(finalize_mysql, x) + end + return +end + +# Explicit close (DBInterface.close!(conn)). Blocking on the lock is fine here: +# finalizers only ever trylock, so there is no self-deadlock if GC runs while we +# hold it. +function close!(x::MYSQL) + lock(x.reaplock) + try + _teardown(x) + finally + unlock(x.reaplock) + end + return +end + +""" + reap!(mysql::MYSQL) + +Close statement handles and free result handles that were abandoned to the +garbage collector. Must be called from a user-initiated operation on the +connection, i.e. in a context the caller already serializes with all other use +of the connection — never from a finalizer. +""" +function reap!(x::MYSQL) + # unlocked fast path: a stale answer just delays the reap to the next call + isempty(x.stmts_to_close) && isempty(x.results_to_free) && return + stmts = Ptr{Cvoid}[] + results = Ptr{Cvoid}[] + lock(x.reaplock) + try + append!(results, x.results_to_free) + empty!(x.results_to_free) + append!(stmts, x.stmts_to_close) + empty!(x.stmts_to_close) + finally + unlock(x.reaplock) + end + # the socket-touching calls happen outside the spinlock; we're in the + # caller's serialized context like any other mysql_* call + for p in results + mysql_free_result(p) + end + for p in stmts + mysql_stmt_close(p) + end + return +end + # wraps a MYSQL_RES opaque pointer mutable struct MYSQL_RES ptr::Ptr{Cvoid} - function MYSQL_RES(ptr) - res = new(ptr) + conn::MYSQL + function MYSQL_RES(ptr, conn::MYSQL) + res = new(ptr, conn) if ptr != C_NULL - finalizer(res) do x - if x.ptr != C_NULL - mysql_free_result(x.ptr) - x.ptr = C_NULL - end - end + finalizer(finalize_result, res) end return res end end +# GC finalizer for MYSQL_RES: park the handle for reap!() instead of calling +# mysql_free_result, which may read pending rows off the shared socket. +function finalize_result(x::MYSQL_RES) + x.ptr == C_NULL && return + conn = x.conn + if trylock(conn.reaplock) + try + if !conn.closed + push!(conn.results_to_free, x.ptr) + end + # if the connection is already closed, mysql_free_result on an + # un-drained result would read through the freed MYSQL* — leak the + # handle rather than touch freed memory + x.ptr = C_NULL + finally + unlock(conn.reaplock) + end + else + finalizer(finalize_result, x) + end + return +end + +# immediate free, for explicit cleanup from user-serialized contexts; the still- +# registered finalizer becomes a no-op once ptr is C_NULL +function free!(x::MYSQL_RES) + if x.ptr != C_NULL + mysql_free_result(x.ptr) + x.ptr = C_NULL + end + return +end + struct StmtError <: Exception errno::Cuint msg::String @@ -50,17 +172,48 @@ Base.showerror(io::IO, e::StmtError) = print(io, "($(e.errno)): $(e.msg)") # wraps a MYSQL_STMT opaque pointer mutable struct MYSQL_STMT ptr::Ptr{Cvoid} - function MYSQL_STMT(ptr) + conn::MYSQL + function MYSQL_STMT(ptr, conn::MYSQL) ptr == C_NULL && error("error creating API.MYSQL_STMT structure; null pointer encountered; probably insufficient memory available") - stmt = new(ptr) - finalizer(stmt) do x - if x.ptr != C_NULL + stmt = new(ptr, conn) + finalizer(finalize_stmt, stmt) + return stmt + end +end + +# GC finalizer for MYSQL_STMT: park the handle for reap!() instead of calling +# mysql_stmt_close, which sends COM_STMT_CLOSE over the shared socket. +function finalize_stmt(x::MYSQL_STMT) + x.ptr == C_NULL && return + conn = x.conn + if trylock(conn.reaplock) + try + if conn.closed + # mysql_close already invalidated the statement handles, so this + # is a purely local free — no socket I/O mysql_stmt_close(x.ptr) - x.ptr = C_NULL + else + push!(conn.stmts_to_close, x.ptr) end + x.ptr = C_NULL + finally + unlock(conn.reaplock) end - return stmt + else + finalizer(finalize_stmt, x) + end + return +end + +# immediate close, for explicit cleanup (DBInterface.close!(stmt)) from user- +# serialized contexts; the still-registered finalizer becomes a no-op once ptr +# is C_NULL +function close!(x::MYSQL_STMT) + if x.ptr != C_NULL + mysql_stmt_close(x.ptr) + x.ptr = C_NULL end + return end StmtError(stmt::MYSQL_STMT) = StmtError(stmt.ptr) diff --git a/src/api/capi.jl b/src/api/capi.jl index 7acbc50..503d1a2 100644 --- a/src/api/capi.jl +++ b/src/api/capi.jl @@ -1498,7 +1498,7 @@ Return Values A pointer to a MYSQL_RES result structure with the results. NULL if the statement did not return a result set or an error occurred. To determine whether an error occurred, check whether mysql_error() returns a nonempty string, mysql_errno() returns nonzero, or mysql_field_count() returns zero. """=# function storeresult(mysql::MYSQL) - return MYSQL_RES(mysql_store_result(mysql.ptr)) + return MYSQL_RES(mysql_store_result(mysql.ptr), mysql) end """ @@ -1526,5 +1526,5 @@ Return Values A MYSQL_RES result structure. NULL if an error occurred. """=# function useresult(mysql::MYSQL) - return MYSQL_RES(mysql_use_result(mysql.ptr)) + return MYSQL_RES(mysql_use_result(mysql.ptr), mysql) end diff --git a/src/api/consts.jl b/src/api/consts.jl index 2c43ff6..dfc48e8 100644 --- a/src/api/consts.jl +++ b/src/api/consts.jl @@ -220,10 +220,15 @@ end MARIADB_OPT_INTERACTIVE MARIADB_OPT_PROXY_HEADER MARIADB_OPT_IO_WAIT - MYSQL_OPT_SSL_MODE end +# NOTE: there is deliberately no MYSQL_OPT_SSL_MODE entry: libmariadb has no +# such option. The entry that used to be here (7025) collided with libmariadb's +# MARIADB_OPT_SKIP_READ_RESPONSE, silently setting that instead +# (https://github.com/JuliaDatabases/MySQL.jl/issues/240). The `ssl_mode` +# connect keyword is instead mapped onto real Connector/C options in +# `setoptions!`. -const CUINTOPTS = Set([MYSQL_OPT_CONNECT_TIMEOUT, MYSQL_OPT_PROTOCOL, MYSQL_OPT_READ_TIMEOUT, MYSQL_OPT_WRITE_TIMEOUT, MYSQL_OPT_SSL_MODE]) +const CUINTOPTS = Set([MYSQL_OPT_CONNECT_TIMEOUT, MYSQL_OPT_PROTOCOL, MYSQL_OPT_READ_TIMEOUT, MYSQL_OPT_WRITE_TIMEOUT]) const CULONGOPTS = Set([MYSQL_OPT_MAX_ALLOWED_PACKET, MYSQL_OPT_NET_BUFFER_LENGTH]) const BOOLOPTS = Set([MYSQL_ENABLE_CLEARTEXT_PLUGIN, MYSQL_OPT_CAN_HANDLE_EXPIRED_PASSWORDS, MYSQL_OPT_LOCAL_INFILE, MYSQL_OPT_RECONNECT, MYSQL_REPORT_DATA_TRUNCATION, MYSQL_OPT_SSL_ENFORCE, MYSQL_OPT_SSL_VERIFY_SERVER_CERT]) const STRINGOPTS = Set([MYSQL_DEFAULT_AUTH, MYSQL_OPT_BIND, MYSQL_OPT_SSL_CA, MYSQL_OPT_SSL_CAPATH, MYSQL_OPT_SSL_CERT, MYSQL_OPT_SSL_CIPHER, MYSQL_OPT_SSL_CRL, MYSQL_OPT_SSL_CRLPATH, MYSQL_OPT_SSL_KEY, MYSQL_OPT_TLS_VERSION, MYSQL_PLUGIN_DIR, MYSQL_READ_DEFAULT_FILE, MYSQL_READ_DEFAULT_GROUP, MYSQL_SERVER_PUBLIC_KEY, MYSQL_SET_CHARSET_DIR, MYSQL_SET_CHARSET_NAME, MYSQL_SHARED_MEMORY_BASE_NAME]) diff --git a/src/api/papi.jl b/src/api/papi.jl index 9d6ebe2..b868ac4 100644 --- a/src/api/papi.jl +++ b/src/api/papi.jl @@ -232,7 +232,7 @@ Return Values A pointer to a MYSQL_STMT structure in case of success. NULL if out of memory. """ function stmtinit(mysql::MYSQL) - return MYSQL_STMT(@checknull mysql mysql_stmt_init(mysql.ptr)) + return MYSQL_STMT((@checknull mysql mysql_stmt_init(mysql.ptr)), mysql) end """ @@ -356,7 +356,7 @@ Return Values A MYSQL_RES result structure. NULL if no meta information exists for the prepared query. """ function resultmetadata(stmt::MYSQL_STMT) - return MYSQL_RES(mysql_stmt_result_metadata(stmt.ptr)) + return MYSQL_RES(mysql_stmt_result_metadata(stmt.ptr), stmt.conn) end """ diff --git a/src/execute.jl b/src/execute.jl index 5dcabd3..6e80512 100644 --- a/src/execute.jl +++ b/src/execute.jl @@ -204,7 +204,7 @@ function Base.iterate(cursor::TextCursors{buffered}, first=true) where {buffered cursor.cursor.result.ptr == C_NULL && return nothing if !first has_more_results = API.moreresults(cursor.cursor.conn.mysql) - finalize(cursor.cursor.result) + API.free!(cursor.cursor.result) if has_more_results @assert API.nextresult(cursor.cursor.conn.mysql) !== nothing cursor.cursor.result = buffered ? API.storeresult(cursor.cursor.conn.mysql) : API.useresult(cursor.cursor.conn.mysql) diff --git a/src/prepare.jl b/src/prepare.jl index 517161a..60e66b7 100644 --- a/src/prepare.jl +++ b/src/prepare.jl @@ -29,7 +29,7 @@ DBInterface.getconnection(stmt::Statement) = stmt.conn Close a prepared statement and free any underlying resources. The statement should not be used in any way afterwards. """ -DBInterface.close!(stmt::Statement) = finalize(stmt.stmt) +DBInterface.close!(stmt::Statement) = API.close!(stmt.stmt) """ DBInterface.prepare(conn::MySQL.Connection, sql) => MySQL.Statement diff --git a/test/runtests.jl b/test/runtests.jl index c88f5e8..5965331 100644 --- a/test/runtests.jl +++ b/test/runtests.jl @@ -113,6 +113,12 @@ let mysql = MySQL.API.init() @test Int(MySQL.API.getoption(mysql, MySQL.API.MYSQL_OPT_MAX_ALLOWED_PACKET)) == 1024 MySQL.setoptions!(mysql; bind="127.0.0.1") @test MySQL.API.getoption(mysql, MySQL.API.MYSQL_OPT_BIND) == "127.0.0.1" + # ssl_mode maps onto real Connector/C options (#240): VERIFY_* turns on + # server certificate verification even though the kwarg default is false + MySQL.setoptions!(mysql; ssl_mode=MySQL.API.SSL_MODE_VERIFY_CA) + @test MySQL.API.getoption(mysql, MySQL.API.MYSQL_OPT_SSL_VERIFY_SERVER_CERT) == true + # SSL_MODE_DISABLED cannot be honored by libmariadb 3.4+ and must say so + @test_logs (:warn, r"SSL_MODE_DISABLED cannot be honored") MySQL.setoptions!(mysql; ssl_mode=MySQL.API.SSL_MODE_DISABLED) end if !docker_available() @@ -552,6 +558,81 @@ ret = columntable(res) DBInterface.close!(conn) end end + +# https://github.com/JuliaDatabases/MySQL.jl/issues/220 +# Statement/result finalizers must not talk to the server; abandoned handles are +# parked on the connection and reaped inside the next user-initiated operation. +# (a function boundary, so the abandoned wrappers aren't kept alive by stack slots) +abandon_stmts(conn, n) = (for _ = 1:n; DBInterface.execute(DBInterface.prepare(conn, "SELECT a FROM FinalizerReap")); end; nothing) +@testset "no I/O in finalizers (#220)" begin + conn = connect_mysql() + try + DBInterface.execute(conn, "CREATE DATABASE IF NOT EXISTS mysqltest") + DBInterface.execute(conn, "use mysqltest") + DBInterface.execute(conn, "DROP TABLE IF EXISTS FinalizerReap") + DBInterface.execute(conn, "CREATE TABLE FinalizerReap (a INT)") + DBInterface.execute(conn, "INSERT INTO FinalizerReap VALUES (1)") + # abandon a batch of prepared statements to the GC + abandon_stmts(conn, 10) + GC.gc(); GC.gc() + # finalizers may only have parked the handles, never closed them directly + # (with the connection still open, closing would mean socket I/O from GC) + # and the next operation reaps them + result = DBInterface.execute(conn, "SELECT a FROM FinalizerReap") |> Tables.columntable + @test result.a == [1] + @test isempty(conn.mysql.stmts_to_close) + @test isempty(conn.mysql.results_to_free) + if Threads.nthreads() > 1 + # concurrent smoke test: GC-driven statement finalizers must not + # corrupt a lock-serialized workload (pre-fix this aborts/errors + # within a few seconds when the connection uses TLS) + lk = ReentrantLock() + done = Threads.Atomic{Bool}(false) + err = Ref{Any}(nothing) + @sync begin + for _ = 1:4 + Threads.@spawn try + while !done[] + lock(lk) do + DBInterface.execute(DBInterface.prepare(conn, "SELECT a FROM FinalizerReap")) + end + end + catch e + err[] = e + done[] = true + end + end + Threads.@spawn while !done[] + GC.gc(false) + end + Threads.@spawn (sleep(5); done[] = true) + end + @test err[] === nothing + end + finally + DBInterface.close!(conn) + end + # closing the connection with parked handles still pending must be safe + conn = connect_mysql() + DBInterface.execute(conn, "use mysqltest") + DBInterface.execute(conn, "CREATE TABLE IF NOT EXISTS FinalizerReap (a INT)") + abandon_stmts(conn, 3) + GC.gc(); GC.gc() + DBInterface.close!(conn) + @test !isopen(conn) +end + +# https://github.com/JuliaDatabases/MySQL.jl/issues/240 +@testset "ssl_mode mapping (#240)" begin + # SSL_MODE_REQUIRED / VERIFY_* map onto real Connector/C options and connect fine + conn = connect_mysql(; ssl_mode=MySQL.API.SSL_MODE_REQUIRED) + try + cipher = DBInterface.execute(conn, "SHOW STATUS LIKE 'Ssl_cipher'") |> Tables.columntable + @test !isempty(cipher.Value[1]) # TLS actually negotiated + finally + DBInterface.close!(conn) + end +end end end end