v4: Protect LDAP connections from being freed when there are outstanding queries...

* Use correct DN string in debug message

* Use query type to identify LDAP searches

* Protect fr_ldap_connection_t from being freed if there are pending queries

* Add watcher checking for outstanding queries when an LDAP connection closes

* Alter _ldap_query_free() to free the associated connection if it is closed ...

... and has no other queries associated with it

* Queries are removed from the pending tree by the query destructor

* Improve handling of un-expected results retrieved from libldap

* Don't free LDAP results just because an LDAP error was returned ...

... if the caller wants the results, it may want to query them
regardless of success or failure.

* Add debug message for auth bind reject

* Free the bind_auth_ctx when we've finished with it

* Improved version of _ldap_bind_auth_io_read()

- handle fr_ldap_result() returning no result
- handle LDAP_PROC_REJECT
- use a normal while loop

diff --git a/src/lib/ldap/base.c b/src/lib/ldap/base.c
index a280848ec17663d81595f048d79e15721acb13c5..aec9c1cfa4e009378f7fe0070b3a4b57bd7c34eb 100644 (file)
--- a/src/lib/ldap/base.c
+++ b/src/lib/ldap/base.c
@@ -450,7 +450,7 @@ fr_ldap_rcode_t fr_ldap_result(LDAPMessage **result, LDAPControl ***ctrls,
                if (status != LDAP_PROC_SUCCESS) break;
        }
 
-       if (*result_p && ((status < 0) || !result)) {
+       if (*result_p && (!result)) {
                ldap_msgfree(*result_p);
                *result_p = NULL;
        }
@@ -1036,11 +1036,18 @@ int fr_ldap_global_config(int debug_level, char const *tls_random_file)
 
 /** Free any libldap structures when an fr_ldap_query_t is freed
  *
+ * It is also possible that the connection used for this query is now closed,
+ * in that instance we free it here.
  */
 static int _ldap_query_free(fr_ldap_query_t *query)
 {
        int     i;
 
+       /*
+        *      Remove the query from the tree of outstanding queries
+        */
+       if (query->ldap_conn) fr_rb_remove(query->ldap_conn->queries, query);
+
        /*
         *      Free any results which were retrieved
         */
@@ -1071,6 +1078,15 @@ static int _ldap_query_free(fr_ldap_query_t *query)
 
        fr_dlist_talloc_free(&query->referrals);
 
+       /*
+        *      If the connection this query was using has no pending queries and
+        *      is no-longer associated with a fr_connection_t then free it
+        */
+       if ((query->ldap_conn) && (query->ldap_conn->conn == NULL) &&
+           (fr_rb_num_elements(query->ldap_conn->queries) == 0)) {
+               talloc_free(query->ldap_conn);
+       }
+
        return 0;
 }
 

diff --git a/src/lib/ldap/bind.c b/src/lib/ldap/bind.c
index 58ea321f07ea25923b3918d51d2ea05723d6f6b8..7fa7f9d8d3f0f0291af03bf3d695ba0843a56e5c 100644 (file)
--- a/src/lib/ldap/bind.c
+++ b/src/lib/ldap/bind.c
@@ -286,6 +286,7 @@ static unlang_action_t ldap_async_auth_bind_results(rlm_rcode_t *p_result, UNUSE
                RETURN_MODULE_DISALLOW;
 
        case LDAP_PROC_REJECT:
+               RDEBUG2("Bind as user \"%s\" rejected", bind_ctx->bind_dn);
                RETURN_MODULE_REJECT;
 
        case LDAP_PROC_BAD_DN:
@@ -298,6 +299,8 @@ static unlang_action_t ldap_async_auth_bind_results(rlm_rcode_t *p_result, UNUSE
                RETURN_MODULE_FAIL;
 
        }
+
+       talloc_free(bind_auth_ctx);
 }
 
 /** Signal an outstanding LDAP bind request to cancel
@@ -311,6 +314,8 @@ static void ldap_async_auth_bind_cancel(UNUSED request_t *request, fr_state_sign
 
        ldap_abandon_ext(bind_auth_ctx->bind_ctx->c->handle, bind_auth_ctx->msgid, NULL, NULL);
        fr_rb_remove(bind_auth_ctx->thread->binds, bind_auth_ctx);
+
+       talloc_free(bind_auth_ctx);
 }
 
 /** Initiate an async LDAP bind for authentication

diff --git a/src/lib/ldap/connection.c b/src/lib/ldap/connection.c
index b53382e4597514fb69dccc5dfdbb1e93653432ec..cb88bd3ac0758a5faefc97fc2ff3b2331cae35f3 100644 (file)
--- a/src/lib/ldap/connection.c
+++ b/src/lib/ldap/connection.c
@@ -239,6 +239,11 @@ static void _ldap_connection_close(UNUSED fr_event_list_t *el, void *h, UNUSED v
  */
 static int _ldap_connection_free(fr_ldap_connection_t *c)
 {
+       /*
+        *      If there are any pending queries, don't free
+        */
+       if ((c->queries) && (fr_rb_num_elements(c->queries) > 0)) return -1;
+
        talloc_free_children(c);        /* Force inverted free order */
 
        fr_ldap_control_clear(c);
@@ -290,6 +295,23 @@ fr_ldap_connection_t *fr_ldap_connection_alloc(TALLOC_CTX *ctx)
        return c;
 }
 
+/** Watcher for LDAP connections being closed
+ *
+ * If there are any outstanding queries on the connection then
+ * re-parent the connection to the NULL ctx so that it remains
+ * until all the queries have been dealt with.
+ */
+static void _ldap_connection_close_watch(fr_connection_t *conn, UNUSED fr_connection_state_t prev,
+                                        UNUSED fr_connection_state_t state, void *uctx)
+{
+       fr_ldap_connection_t    *ldap_conn = talloc_get_type_abort(uctx, fr_ldap_connection_t);
+
+       if (fr_rb_num_elements(ldap_conn->queries) == 0) return;
+
+       talloc_reparent(conn, NULL, ldap_conn);
+       ldap_conn->conn = NULL;
+}
+
 /** (Re-)Initialises the libldap side of the connection handle
  *
  *  The first ldap state transition is either:
@@ -343,6 +365,8 @@ static fr_connection_state_t _ldap_connection_init(void **h, fr_connection_t *co
        c = fr_ldap_connection_alloc(conn);
        c->conn = conn;
 
+       fr_connection_add_watch_pre(conn, FR_CONNECTION_STATE_CLOSED, _ldap_connection_close_watch, false, c);
+
        /*
         *      Configure/allocate the libldap handle
         */
@@ -485,7 +509,6 @@ static void ldap_request_cancel_mux(fr_trunk_connection_t *tconn, fr_connection_
        while ((fr_trunk_connection_pop_cancellation(&treq, tconn)) == 0) {
                query = treq->preq;
                ldap_abandon_ext(ldap_conn->handle, query->msgid, NULL, NULL);
-               fr_rb_remove(ldap_conn->queries, query);
 
                fr_trunk_request_signal_cancel_complete(treq);
 
@@ -791,6 +814,18 @@ static void ldap_trunk_request_demux(UNUSED fr_trunk_connection_t *tconn, fr_con
                if (!query) {
                        WARN("Ignoring msgid %i - doesn't match any outstanding queries (it may have been cancelled)",
                              find.msgid);
+                       ldap_msgfree(result);
+                       continue;
+               }
+
+               /*
+                *      This really shouldn't happen - as we only retrieve complete sets of results -
+                *      but as the query data structure will last until its results are fully handled
+                *      better to have this safety check here.
+                */
+               if (query->ret != LDAP_RESULT_PENDING) {
+                       WARN("Received results for msgid %i which has already been handled - ignoring", find.msgid);
+                       ldap_msgfree(result);
                        continue;
                }
 
@@ -807,7 +842,8 @@ static void ldap_trunk_request_demux(UNUSED fr_trunk_connection_t *tconn, fr_con
 
                switch (rcode) {
                case LDAP_PROC_SUCCESS:
-                       query->ret = ((!query->mods) && (ldap_count_entries(ldap_conn->handle, result) == 0)) ?
+                       query->ret = ((query->type == LDAP_REQUEST_SEARCH) &&
+                                     (ldap_count_entries(ldap_conn->handle, result) == 0)) ?
                                     LDAP_RESULT_NO_RESULT : LDAP_RESULT_SUCCESS;
                        break;
 
@@ -852,7 +888,7 @@ static void ldap_trunk_request_demux(UNUSED fr_trunk_connection_t *tconn, fr_con
                        break;
 
                case LDAP_PROC_BAD_DN:
-                       ROPTIONAL(RDEBUG2, DEBUG2, "DN %s does not exist", query->ldap_url->lud_dn);
+                       ROPTIONAL(RDEBUG2, DEBUG2, "DN %s does not exist", query->dn);
                        query->ret = LDAP_RESULT_BAD_DN;
                        break;
 
@@ -886,9 +922,8 @@ static void ldap_trunk_request_demux(UNUSED fr_trunk_connection_t *tconn, fr_con
                if (query->parser) query->parser(query, result);
 
                /*
-                *      Remove the query from the outstanding list and tidy up
+                *      Mark the trunk request as complete and set the request as runnable
                 */
-               fr_rb_remove(ldap_conn->queries, query);
                fr_trunk_request_signal_complete(query->treq);
                if (query->request) unlang_interpret_mark_runnable(query->request);
 

diff --git a/src/modules/rlm_ldap/rlm_ldap.c b/src/modules/rlm_ldap/rlm_ldap.c
index 0871940ce6a551d49374e3d01f4cabfddf7ad9d1..f8c7d641d12087173c4b5d330df9edc9b0a6b798 100644 (file)
--- a/src/modules/rlm_ldap/rlm_ldap.c
+++ b/src/modules/rlm_ldap/rlm_ldap.c
@@ -616,45 +616,61 @@ static void _ldap_bind_auth_io_read(UNUSED fr_event_list_t *el, UNUSED int fd, U
 
        int                     ret;
 
-next_result:
-       /*
-        *      Fetch the next LDAP result which has been fully received
-        */
-       ret = fr_ldap_result(&result, NULL, ldap_conn, LDAP_RES_ANY, LDAP_MSG_ALL, NULL, fr_time_delta_wrap(10));
+       do {
+               /*
+                *      Fetch the next LDAP result which has been fully received
+                */
+               ret = fr_ldap_result(&result, NULL, ldap_conn, LDAP_RES_ANY, LDAP_MSG_ALL, NULL, fr_time_delta_wrap(10));
 
-       /*
-        *      Timeout in this case really means no results to read - we've
-        *      handled everything that was available
-        */
-       if (ret == LDAP_PROC_TIMEOUT) return;
+               /*
+                *      Timeout in this case really means no results to read - we've
+                *      handled everything that was available
+                */
+               if (ret == LDAP_PROC_TIMEOUT) return;
 
-       find.msgid = ldap_msgid(result);
-       bind_auth_ctx = fr_rb_find(thread->binds, &find);
+               /*
+                *      If there is no result don't try to process one
+                */
+               if (!result) return;
 
-       if (!bind_auth_ctx) {
-               WARN("Ignoring bind result msgid %i - doesn't match any outstanidng binds", find.msgid);
-               goto next_result;
-       }
+               find.msgid = ldap_msgid(result);
+               bind_auth_ctx = fr_rb_find(thread->binds, &find);
 
-       /*
-        *      Remove from the list of pending bind requests
-        */
-       fr_rb_remove(bind_auth_ctx->thread->binds, bind_auth_ctx);
+               if (!bind_auth_ctx) {
+                       WARN("Ignoring bind result msgid %i - doesn't match any outstanidng binds", find.msgid);
+                       ldap_msgfree(result);
+                       continue;
+               }
+
+               /*
+                *      Remove from the list of pending bind requests
+                */
+               fr_rb_remove(bind_auth_ctx->thread->binds, bind_auth_ctx);
 
-       bind_auth_ctx->ret = ret;
+               bind_auth_ctx->ret = ret;
 
-       switch (ret) {
-       case LDAP_PROC_SUCCESS:
-       case LDAP_PROC_NOT_PERMITTED:
-               break;
+               switch (ret) {
+               /*
+                *      Accept or reject will be SUCCESS, NOT_PERMITTED or REJECT
+                */
+               case LDAP_PROC_SUCCESS:
+               case LDAP_PROC_NOT_PERMITTED:
+               case LDAP_PROC_REJECT:
+                       break;
 
-       default:
-               fr_ldap_state_error(bind_auth_ctx->bind_ctx->c);        /* Restart the connection state machine */
-               break;
-       }
-       unlang_interpret_mark_runnable(bind_auth_ctx->request);
+               default:
+                       ERROR("LDAP connection returned an error - restarting the connection");
+                       fr_ldap_state_error(bind_auth_ctx->bind_ctx->c);        /* Restart the connection state machine */
+                       break;
+               }
+               unlang_interpret_mark_runnable(bind_auth_ctx->request);
+
+               /*
+                *      Clear up the libldap results
+                */
+               ldap_msgfree(result);
 
-       goto next_result;
+       } while (1);
 }
 
 /** Watch callback to add fd read callback to LDAP connection