connection: expose is_closed so a closed connection's handle can't be used

The close callback frees the handle, but is_closed lived in the private struct so drivers had no way to tell a live connection from a dead one. A mysql connection dying mid-query aborted on talloc_get_type_abort() in sql_error(), reached from sql_get_map_list_resume() after the trunk had already closed the connection.

rlm_sql_print_error() is the only caller of driver->sql_error, and every driver reads the errors off the handle, so the check goes there once and reports "Connection closed" rather than in each driver.

diff --git a/src/lib/server/connection.c b/src/lib/server/connection.c
index a26000308af2f615efd3a976bb2ba1e344aad173..c44d83d4584d0d64d84daa5aadb70991a53cd64e 100644 (file)
--- a/src/lib/server/connection.c
+++ b/src/lib/server/connection.c
@@ -90,7 +90,6 @@ struct connection_s {
        void                    *uctx;                  //!< User data.
 
        void                    *in_handler;            //!< Connection is currently in a callback.
-       bool                    is_closed;              //!< The close callback has previously been called.
        bool                    processing_signals;     //!< Processing deferred signals, don't let the deferred
                                                        ///< signal processor be called multiple times.
 
@@ -683,11 +682,11 @@ static void connection_state_enter_closed(connection_t *conn)
         *      the FAILED state.  Eventually the connection is
         *      shutdown, and enter_shutdown calls this function.
         */
-       if (conn->close && !conn->is_closed) {
+       if (conn->close && !conn->pub.is_closed) {
                HANDLER_BEGIN(conn, conn->close);
                DEBUG4("Calling close(el=%p, h=%p, uctx=%p)", conn->pub.el, conn->pub.h, conn->uctx);
                conn->close(conn->pub.el, conn->pub.h, conn->uctx);
-               conn->is_closed = true;         /* Ensure close doesn't get called twice if the connection is freed */
+               conn->pub.is_closed = true;             /* Ensure close doesn't get called twice if the connection is freed */
                HANDLER_END(conn);
 
                /*
@@ -697,7 +696,7 @@ static void connection_state_enter_closed(connection_t *conn)
                 */
                if (conn->pub.state != CONNECTION_STATE_CLOSED) return;
        } else {
-               conn->is_closed = true;
+               conn->pub.is_closed = true;
        }
        WATCH_POST(conn);
 }
@@ -953,7 +952,7 @@ static void connection_state_enter_timeout(connection_t *conn)
  */
 static void connection_state_enter_halted(connection_t *conn)
 {
-       fr_assert(conn->is_closed);
+       fr_assert(conn->pub.is_closed);
 
        switch (conn->pub.state) {
        case CONNECTION_STATE_INIT:
@@ -1135,13 +1134,13 @@ static void connection_state_enter_init(connection_t *conn)
 
        switch (ret) {
        case CONNECTION_STATE_CONNECTING:
-               conn->is_closed = false;        /* We now have a handle */
+               conn->pub.is_closed = false;    /* We now have a handle */
                WATCH_POST(conn);               /* Only call if we successfully initialised the handle */
                connection_state_enter_connecting(conn);
                return;
 
        case CONNECTION_STATE_CONNECTED:
-               conn->is_closed = false;        /* We now have a handle */
+               conn->pub.is_closed = false;    /* We now have a handle */
                WATCH_POST(conn);               /* Only call if we successfully initialised the handle */
                connection_state_enter_connected(conn);
                return;
@@ -1326,7 +1325,7 @@ void connection_signal_shutdown(connection_t *conn)
        case CONNECTION_STATE_TIMEOUT:
        case CONNECTION_STATE_FAILED:
                connection_state_enter_closed(conn);
-               fr_assert(conn->is_closed);
+               fr_assert(conn->pub.is_closed);
 
        FALL_THROUGH;
        case CONNECTION_STATE_CLOSED:
@@ -1380,8 +1379,8 @@ void connection_signal_halt(connection_t *conn)
        case CONNECTION_STATE_SHUTDOWN:
        case CONNECTION_STATE_TIMEOUT:
        case CONNECTION_STATE_FAILED:
-               if (!conn->is_closed) connection_state_enter_closed(conn);
-               fr_assert(conn->is_closed);
+               if (!conn->pub.is_closed) connection_state_enter_closed(conn);
+               fr_assert(conn->pub.is_closed);
                connection_state_enter_halted(conn);
                break;
 
@@ -1589,7 +1588,8 @@ connection_t *connection_alloc(TALLOC_CTX *ctx, fr_event_list_t *el,
                .pub = {
                        .id = id,
                        .state = CONNECTION_STATE_HALTED,
-                       .el = el
+                       .el = el,
+                       .is_closed = true               /* Starts closed */
                },
                .reconnection_delay = conf->reconnection_delay,
                .connection_timeout = conf->connection_timeout,
@@ -1598,7 +1598,6 @@ connection_t *connection_alloc(TALLOC_CTX *ctx, fr_event_list_t *el,
                .close = funcs->close,
                .failed = funcs->failed,
                .shutdown = funcs->shutdown,
-               .is_closed = true,              /* Starts closed */
                .triggers = conf->triggers,
                .trigger_args = conf->trigger_args,
                .trigger_cs = conf->trigger_cs,

diff --git a/src/lib/server/connection.h b/src/lib/server/connection.h
index 8bf24101eea075f47004b512555f0a217d6ffe7a..f28c66dd1eb4706253c88649b54e18b6b7f84809 100644 (file)
--- a/src/lib/server/connection.h
+++ b/src/lib/server/connection.h
@@ -75,6 +75,10 @@ struct connection_pub_s {
        void                    * _CONST h;             //!< Connection handle
        fr_event_list_t         * _CONST el;            //!< Event list for timers and I/O events.
 
+       bool                    _CONST is_closed;       //!< The close callback has previously been called,
+                                                       ///< so the handle is gone.  Check this before using
+                                                       ///< anything that lives on the connection.
+
        uint64_t _CONST                 reconnected;    //!< How many times we've attempted to establish or
                                                        ///< re-establish this connection.
        uint64_t _CONST                 timed_out;      //!< How many times has this connection timed out when

diff --git a/src/modules/rlm_sql/sql.c b/src/modules/rlm_sql/sql.c
index 06199e730f9709e285665ceedf8f50b68a6fd015..bc30fc0e35ce32e30a4485b0d590d8916b5d7cd1 100644 (file)
--- a/src/modules/rlm_sql/sql.c
+++ b/src/modules/rlm_sql/sql.c
@@ -124,7 +124,20 @@ void rlm_sql_print_error(rlm_sql_t const *inst, request_t *request, fr_sql_query
        char const      *driver = inst->driver_submodule->name;
        sql_log_entry_t log[20];
        size_t          num, i;
-       TALLOC_CTX      *log_ctx = talloc_new(NULL);
+       TALLOC_CTX      *log_ctx;
+
+       /*
+        *      Every driver reads the errors off the connection handle, which the
+        *      trunk frees when it closes the connection.  A query that failed
+        *      because its connection died is resumed after that has happened,
+        *      so there is nothing left to ask the driver for.
+        */
+       if (!query_ctx->tconn || query_ctx->tconn->conn->is_closed) {
+               ROPTIONAL(RERROR, ERROR, "%s: Connection closed", driver);
+               return;
+       }
+
+       log_ctx = talloc_new(NULL);
 
        num = (inst->driver->sql_error)(log_ctx, log, (NUM_ELEMENTS(log)), query_ctx);
        if (num == 0) {