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

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"

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,15 +60,15 @@ 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
                     break
                 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
 

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)

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

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])

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
 
 """

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)

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