Fix race condition that was causing segfaults

The sdap_handle might be freed when processing a message.
Rearrange data flow so that the sdap_handle is never used after
a message is processed but a new event (dependent on the handle) is
instead scheduled. If the sdap_handle is freed, the scheduled event
is also removed and not fired

1 files changed, 136 insertions, 80 deletions

diff --git a/server/providers/ldap/sdap_async.c b/server/providers/ldap/sdap_async.c
index d5e42f0c..762cd5ab 100644
--- a/server/providers/ldap/sdap_async.c
+++ b/server/providers/ldap/sdap_async.c
@@ -71,19 +71,24 @@ static int sdap_handle_destructor(void *mem)
 
 static inline void sdap_handle_release(struct sdap_handle *sh)
 {
+    DEBUG(8, ("Trace: sh[%p], connected[%d], ops[%p], fde[%p], ldap[%p]\n",
+              sh, (int)sh->connected, sh->ops, sh->fde, sh->ldap));
+
     if (sh->connected) {
         struct sdap_op *op;
 
+        talloc_zfree(sh->fde);
+
         while (sh->ops) {
             op = sh->ops;
             op->callback(op->data, EIO, NULL);
             talloc_free(op);
         }
 
-        talloc_zfree(sh->fde);
         ldap_unbind_ext(sh->ldap, NULL, NULL);
         sh->connected = false;
         sh->ldap = NULL;
+        sh->ops = NULL;
     }
 }
 
@@ -103,9 +108,79 @@ static int get_fd_from_ldap(LDAP *ldap, int *fd)
 
 /* ==Parse-Results-And-Handle-Disconnections============================== */
 
+static void sdap_process_message(struct sdap_handle *sh, LDAPMessage *msg);
+static void sdap_process_result(struct tevent_context *ev, void *pvt);
+
+static void sdap_ldap_result(struct tevent_context *ev,
+                             struct tevent_fd *fde,
+                             uint16_t flags, void *pvt)
+{
+    sdap_process_result(ev, pvt);
+}
+
+static void sdap_ldap_next_result(struct tevent_context *ev,
+                                  struct tevent_timer *te,
+                                  struct timeval tv, void *pvt)
+{
+    sdap_process_result(ev, pvt);
+}
+
+static void sdap_process_result(struct tevent_context *ev, void *pvt)
+{
+    struct sdap_handle *sh = talloc_get_type(pvt, struct sdap_handle);
+    struct timeval no_timeout = {0, 0};
+    struct tevent_timer *te;
+    LDAPMessage *msg;
+    int ret;
+
+    DEBUG(8, ("Trace: sh[%p], connected[%d], ops[%p], fde[%p], ldap[%p]\n",
+              sh, (int)sh->connected, sh->ops, sh->fde, sh->ldap));
+
+    if (!sh->connected || !sh->ldap) {
+        DEBUG(2, ("ERROR: LDAP connection is not connected!\n"));
+        return;
+    }
+
+    ret = ldap_result(sh->ldap, LDAP_RES_ANY, 0, &no_timeout, &msg);
+    if (ret == 0) {
+        /* this almost always means we have reached the end of
+         * the list of received messages */
+        DEBUG(8, ("Trace: ldap_result found nothing!\n"));
+        return;
+    }
+
+    if (ret == -1) {
+        DEBUG(4, ("ldap_result gave -1, something bad happend!\n"));
+        sdap_handle_release(sh);
+        return;
+    }
+
+    /* We don't know if this will be the last result.
+     *
+     * important: we must do this before actually processing the message
+     * because the message processing might even free the sdap_handler
+     * so it must be the last operation.
+     * FIXME: use tevent_immediate/tevent_queues, when avilable */
+    memset(&no_timeout, 0, sizeof(struct timeval));
+
+    te = tevent_add_timer(ev, sh, no_timeout, sdap_ldap_next_result, sh);
+    if (!te) {
+        DEBUG(1, ("Failed to add critical timer to fetch next result!\n"));
+    }
+
+
+    /* now process this message */
+    sdap_process_message(sh, msg);
+}
+
+/* process a messgae calling the right operation callback.
+ * msg is completely taken care of (including freeeing it)
+ * NOTE: this function may even end up freeing the sdap_handle
+ * so sdap_hanbdle must not be used after this function is called
+ */
 static void sdap_process_message(struct sdap_handle *sh, LDAPMessage *msg)
 {
-    struct sdap_msg *reply = NULL;
+    struct sdap_msg *reply;
     struct sdap_op *op;
     int msgid;
     int msgtype;
@@ -118,97 +193,75 @@ static void sdap_process_message(struct sdap_handle *sh, LDAPMessage *msg)
         return;
     }
 
-    for (op = sh->ops; op; op = op->next) {
-        if (op->msgid == msgid && !op->done) {
-            msgtype = ldap_msgtype(msg);
-
-            switch (msgtype) {
-            case LDAP_RES_SEARCH_ENTRY:
-            case LDAP_RES_SEARCH_REFERENCE:
-                /* more ops to come with this msgid */
-                ret = EOK;
-                break;
-
-            case LDAP_RES_BIND:
-            case LDAP_RES_SEARCH_RESULT:
-            case LDAP_RES_MODIFY:
-            case LDAP_RES_ADD:
-            case LDAP_RES_DELETE:
-            case LDAP_RES_MODDN:
-            case LDAP_RES_COMPARE:
-            case LDAP_RES_EXTENDED:
-            case LDAP_RES_INTERMEDIATE:
-                /* no more results expected with this msgid */
-                op->done = true;
-                ret = EOK;
-                break;
-
-            default:
-                /* unkwon msg type ?? */
-                DEBUG(1, ("Couldn't figure out the msg type! [%0x]\n",
-                          msgtype));
-                ret = EIO;
-            }
-
-            if (ret == EOK) {
-                reply = talloc(op, struct sdap_msg);
-                if (!reply) {
-                    ldap_msgfree(msg);
-                    ret = ENOMEM;
-                } else {
-                    reply->msg = msg;
-                    ret = sdap_msg_attach(reply, msg);
-                    if (ret != EOK) {
-                        ldap_msgfree(msg);
-                        talloc_zfree(reply);
-                    }
-                }
-            }
-
-            op->callback(op->data, ret, reply);
+    msgtype = ldap_msgtype(msg);
 
-            break;
-        }
+    for (op = sh->ops; op; op = op->next) {
+        if (op->msgid == msgid) break;
     }
 
     if (op == NULL) {
         DEBUG(2, ("Unmatched msgid, discarding message (type: %0x)\n",
-                  ldap_msgtype(msg)));
+                  msgtype));
+        ldap_msgfree(msg);
         return;
     }
-}
 
-static void sdap_ldap_results(struct tevent_context *ev,
-                              struct tevent_fd *fde,
-                              uint16_t flags, void *pvt)
-{
-    struct sdap_handle *sh = talloc_get_type(pvt, struct sdap_handle);
-    struct timeval no_timeout = {0, 0};
-    LDAPMessage *msg;
-    int ret;
+    /* shouldn't happen */
+    if (op->done) {
+        DEBUG(2, ("Operation [%p] already handled (type: %0x)\n", op, msgtype));
+        ldap_msgfree(msg);
+        return;
+    }
 
-    while (1) {
-        if (!sh->connected) {
-            DEBUG(2, ("FDE fired but LDAP connection is not connected!\n"));
-            sdap_handle_release(sh);
-            return;
-        }
+    switch (msgtype) {
+    case LDAP_RES_SEARCH_ENTRY:
+    case LDAP_RES_SEARCH_REFERENCE:
+        /* more ops to come with this msgid */
+        /* just ignore */
+        ldap_msgfree(msg);
+        return;
 
-        ret = ldap_result(sh->ldap, LDAP_RES_ANY, 0, &no_timeout, &msg);
-        if (ret == 0) {
-            DEBUG(6, ("FDE fired but ldap_result found nothing!\n"));
-            return;
-        }
+    case LDAP_RES_BIND:
+    case LDAP_RES_SEARCH_RESULT:
+    case LDAP_RES_MODIFY:
+    case LDAP_RES_ADD:
+    case LDAP_RES_DELETE:
+    case LDAP_RES_MODDN:
+    case LDAP_RES_COMPARE:
+    case LDAP_RES_EXTENDED:
+    case LDAP_RES_INTERMEDIATE:
+        /* no more results expected with this msgid */
+        op->done = true;
+        ret = EOK;
+        break;
 
-        if (ret == -1) {
-            DEBUG(4, ("ldap_result gave -1, something bad happend!\n"));
+    default:
+        /* unkwon msg type ?? */
+        DEBUG(1, ("Couldn't figure out the msg type! [%0x]\n", msgtype));
+        ldap_msgfree(msg);
+        return;
+    }
 
-            sdap_handle_release(sh);
-            return;
+    if (ret == EOK) {
+        reply = talloc(op, struct sdap_msg);
+        if (!reply) {
+            ldap_msgfree(msg);
+            ret = ENOMEM;
+        } else {
+            reply->msg = msg;
+            ret = sdap_msg_attach(reply, msg);
+            if (ret != EOK) {
+                ldap_msgfree(msg);
+                talloc_zfree(reply);
+            }
         }
-
-        sdap_process_message(sh, msg);
+    } else {
+        reply = NULL;
     }
+
+    /* must be the last operation as it may end up freeing all memory
+     * including all ops handlers */
+    op->callback(op->data, ret, reply);
 }
 
 static int sdap_install_ldap_callbacks(struct sdap_handle *sh,
@@ -220,9 +273,12 @@ static int sdap_install_ldap_callbacks(struct sdap_handle *sh,
     ret = get_fd_from_ldap(sh->ldap, &fd);
     if (ret) return ret;
 
-    sh->fde = tevent_add_fd(ev, sh, fd, TEVENT_FD_READ, sdap_ldap_results, sh);
+    sh->fde = tevent_add_fd(ev, sh, fd, TEVENT_FD_READ, sdap_ldap_result, sh);
     if (!sh->fde) return ENOMEM;
 
+    DEBUG(8, ("Trace: sh[%p], connected[%d], ops[%p], fde[%p], ldap[%p]\n",
+              sh, (int)sh->connected, sh->ops, sh->fde, sh->ldap));
+
     return EOK;
 }