>From bb37d24e2c864bdbc6a214eb888449ef5c3f92d3 Mon Sep 17 00:00:00 2001
From: Simo Sorce <ssorce@redhat.com>
Date: Tue, 13 Apr 2010 12:19:25 -0400
Subject: [PATCH] Avoid freeing sdap_handle too early

Within sdap_release_handle() some callbacks can use sdap_mark_offline()
or try to restart calls. Make sure they do not end up freeing the
sdap_handle prematurely and cause access to freed memory later.

Always just mark the sdap_handle as not connected and let later _send()
functions to take care of freeing the handle before reconnecting.

Introduce restart functions to avoid calling _send() functions in _done()
functions error paths as this would have the same effect as directly freeing
the sdap_handle and cause access to freed memory in sdap_handle_release()

By freeing sdap_handle only in the connection _recv() function we
guarantee it can never be done within sdap_handle_release() but only
in a following event.

In any case prevent freeing the sdap_handle by failing in the
destructor if we are trying to recurse.
---
 src/providers/data_provider_be.c           |    2 +-
 src/providers/dp_backend.h                 |    4 +
 src/providers/ipa/ipa_access.c             |    8 --
 src/providers/ldap/ldap_common.c           |    2 +-
 src/providers/ldap/ldap_id.c               |   56 ++++++++++----
 src/providers/ldap/ldap_id_enum.c          |  111 ++++++++++++++++++++++++----
 src/providers/ldap/sdap.h                  |    3 +
 src/providers/ldap/sdap_async.c            |   66 +++++++++++-----
 src/providers/ldap/sdap_async_connection.c |    3 +
 9 files changed, 192 insertions(+), 63 deletions(-)

diff --git a/src/providers/data_provider_be.c b/src/providers/data_provider_be.c
index 239b92a..591edb3 100644
--- a/src/providers/data_provider_be.c
+++ b/src/providers/data_provider_be.c
@@ -405,7 +405,7 @@ static int be_get_account_info(DBusMessage *message, struct sbus_connection *con
     }
 
     /* process request */
-    be_req = talloc(becli, struct be_req);
+    be_req = talloc_zero(becli, struct be_req);
     if (!be_req) {
         err_maj = DP_ERR_FATAL;
         err_min = ENOMEM;
diff --git a/src/providers/dp_backend.h b/src/providers/dp_backend.h
index f1069d0..d657af1 100644
--- a/src/providers/dp_backend.h
+++ b/src/providers/dp_backend.h
@@ -103,6 +103,8 @@ struct bet_ops {
     be_req_fn_t finalize;
 };
 
+#define MAX_BE_REQ_RESTARTS 2
+
 struct be_req {
     struct be_client *becli;
     struct be_ctx *be_ctx;
@@ -110,6 +112,8 @@ struct be_req {
 
     be_async_callback_t fn;
     void *pvt;
+
+    int restarts;
 };
 
 struct be_acct_req {
diff --git a/src/providers/ipa/ipa_access.c b/src/providers/ipa/ipa_access.c
index a7058ba..d88e5c9 100644
--- a/src/providers/ipa/ipa_access.c
+++ b/src/providers/ipa/ipa_access.c
@@ -325,10 +325,6 @@ static struct tevent_req *hbac_get_host_info_send(TALLOC_CTX *memctx,
     }
 
     if (sdap_ctx->gsh == NULL || ! sdap_ctx->gsh->connected) {
-        if (sdap_ctx->gsh != NULL) {
-            talloc_zfree(sdap_ctx->gsh);
-        }
-
         subreq = sdap_cli_connect_send(state, ev, sdap_ctx->opts,
                                        sdap_ctx->be, sdap_ctx->service, NULL);
         if (!subreq) {
@@ -780,10 +776,6 @@ static struct tevent_req *hbac_get_rules_send(TALLOC_CTX *memctx,
     }
 
     if (sdap_ctx->gsh == NULL || ! sdap_ctx->gsh->connected) {
-        if (sdap_ctx->gsh != NULL) {
-            talloc_zfree(sdap_ctx->gsh);
-        }
-
         subreq = sdap_cli_connect_send(state, ev, sdap_ctx->opts,
                                        sdap_ctx->be, sdap_ctx->service, NULL);
         if (!subreq) {
diff --git a/src/providers/ldap/ldap_common.c b/src/providers/ldap/ldap_common.c
index b5765c2..90ec7e2 100644
--- a/src/providers/ldap/ldap_common.c
+++ b/src/providers/ldap/ldap_common.c
@@ -359,7 +359,7 @@ void sdap_mark_offline(struct sdap_id_ctx *ctx)
     if (ctx->gsh) {
         /* make sure we mark the connection as gone when we go offline so that
          * we do not try to reuse a bad connection by mistale later */
-        talloc_zfree(ctx->gsh);
+        ctx->gsh->connected = false;
     }
 
     be_mark_offline(ctx->be);
diff --git a/src/providers/ldap/ldap_id.c b/src/providers/ldap/ldap_id.c
index 15e2f30..a2217da 100644
--- a/src/providers/ldap/ldap_id.c
+++ b/src/providers/ldap/ldap_id.c
@@ -99,8 +99,6 @@ struct tevent_req *users_get_send(TALLOC_CTX *memctx,
 
     if (!sdap_connected(ctx)) {
 
-        if (ctx->gsh) talloc_zfree(ctx->gsh);
-
         /* FIXME: add option to decide if tls should be used
          * or SASL/GSSAPI, etc ... */
         subreq = sdap_cli_connect_send(state, ev, ctx->opts,
@@ -299,8 +297,6 @@ struct tevent_req *groups_get_send(TALLOC_CTX *memctx,
 
     if (!sdap_connected(ctx)) {
 
-        if (ctx->gsh) talloc_zfree(ctx->gsh);
-
         /* FIXME: add option to decide if tls should be used
          * or SASL/GSSAPI, etc ... */
         subreq = sdap_cli_connect_send(state, ev, ctx->opts,
@@ -465,8 +461,6 @@ static struct tevent_req *groups_by_user_send(TALLOC_CTX *memctx,
 
     if (!sdap_connected(ctx)) {
 
-        if (ctx->gsh) talloc_zfree(ctx->gsh);
-
         /* FIXME: add option to decide if tls should be used
          * or SASL/GSSAPI, etc ... */
         subreq = sdap_cli_connect_send(state, ev, ctx->opts,
@@ -654,13 +648,43 @@ void sdap_account_info_handler(struct be_req *breq)
     if (ret != EOK) return sdap_handler_done(breq, DP_ERR_FATAL, ret, err);
 }
 
+static void sdap_account_info_immediate(struct tevent_context *ctx,
+                                        struct tevent_immediate *im,
+                                        void *private_data)
+{
+    struct be_req *breq = talloc_get_type(private_data, struct be_req);
+
+    sdap_account_info_handler(breq);
+}
+
+static int sdap_account_info_restart(struct be_req *breq)
+{
+    struct tevent_immediate *im;
+
+    breq->restarts++;
+    if (breq->restarts > MAX_BE_REQ_RESTARTS) {
+        return ELOOP;
+    }
+
+    im = tevent_create_immediate(breq);
+    if (!im) {
+        return ENOMEM;
+    }
+
+    /* schedule a completely new event to avoid deep recursions */
+    tevent_schedule_immediate(im, breq->be_ctx->ev,
+                              sdap_account_info_immediate, breq);
+
+    return EOK;
+}
+
 static void sdap_account_info_users_done(struct tevent_req *req)
 {
     struct be_req *breq = tevent_req_callback_data(req, struct be_req);
     struct sdap_id_ctx *ctx;
     int dp_err = DP_ERR_OK;
     const char *error = NULL;
-    int ret;
+    int ret, err;
 
     ret = users_get_recv(req);
     talloc_zfree(req);
@@ -674,9 +698,9 @@ static void sdap_account_info_users_done(struct tevent_req *req)
             ctx = talloc_get_type(breq->be_ctx->bet_info[BET_ID].pvt_bet_data,
                                   struct sdap_id_ctx);
             if (sdap_check_gssapi_reconnect(ctx)) {
-                talloc_zfree(ctx->gsh);
-                sdap_account_info_handler(breq);
-                return;
+                ctx->gsh->connected = false;
+                err = sdap_account_info_restart(breq);
+                if (err == EOK) return;
             }
             sdap_mark_offline(ctx);
         }
@@ -691,7 +715,7 @@ static void sdap_account_info_groups_done(struct tevent_req *req)
     struct sdap_id_ctx *ctx;
     int dp_err = DP_ERR_OK;
     const char *error = NULL;
-    int ret;
+    int ret, err;
 
     ret = groups_get_recv(req);
     talloc_zfree(req);
@@ -705,9 +729,9 @@ static void sdap_account_info_groups_done(struct tevent_req *req)
             ctx = talloc_get_type(breq->be_ctx->bet_info[BET_ID].pvt_bet_data,
                                   struct sdap_id_ctx);
             if (sdap_check_gssapi_reconnect(ctx)) {
-                talloc_zfree(ctx->gsh);
-                sdap_account_info_handler(breq);
-                return;
+                ctx->gsh->connected = false;
+                err = sdap_account_info_restart(breq);
+                if (err == EOK) return;
             }
             sdap_mark_offline(ctx);
         }
@@ -736,8 +760,8 @@ static void sdap_account_info_initgr_done(struct tevent_req *req)
             ctx = talloc_get_type(breq->be_ctx->bet_info[BET_ID].pvt_bet_data,
                                   struct sdap_id_ctx);
             if (sdap_check_gssapi_reconnect(ctx)) {
-                talloc_zfree(ctx->gsh);
-                sdap_account_info_handler(breq);
+                ctx->gsh->connected = false;
+                sdap_account_info_restart(breq);
                 return;
             }
             sdap_mark_offline(ctx);
diff --git a/src/providers/ldap/ldap_id_enum.c b/src/providers/ldap/ldap_id_enum.c
index bc06e8b..d86b38d 100644
--- a/src/providers/ldap/ldap_id_enum.c
+++ b/src/providers/ldap/ldap_id_enum.c
@@ -143,12 +143,14 @@ int ldap_id_enumerate_set_timer(struct sdap_id_ctx *ctx, struct timeval tv)
     return EOK;
 }
 
+#define MAX_ENUM_RESTARTS 3
 
 struct global_enum_state {
     struct tevent_context *ev;
     struct sdap_id_ctx *ctx;
 
     bool purge;
+    int restarts;
 };
 
 static struct tevent_req *enum_users_send(TALLOC_CTX *memctx,
@@ -162,6 +164,8 @@ static struct tevent_req *enum_groups_send(TALLOC_CTX *memctx,
                                           bool purge);
 static void ldap_id_enum_groups_done(struct tevent_req *subreq);
 static void ldap_id_enum_cleanup_done(struct tevent_req *subreq);
+static int ldap_id_enum_users_restart(struct tevent_req *req);
+static int ldap_id_enum_groups_restart(struct tevent_req *req);
 
 static struct tevent_req *ldap_id_enumerate_send(struct tevent_context *ev,
                                                  struct sdap_id_ctx *ctx)
@@ -175,6 +179,7 @@ static struct tevent_req *ldap_id_enumerate_send(struct tevent_context *ev,
 
     state->ev = ev;
     state->ctx = ctx;
+    state->restarts = 0;
 
     ctx->last_enum = tevent_timeval_current();
 
@@ -203,6 +208,7 @@ static void ldap_id_enum_users_done(struct tevent_req *subreq)
                                                  struct global_enum_state);
     enum tevent_req_state tstate;
     uint64_t err = 0;
+    int ret;
 
     if (tevent_req_is_error(subreq, &tstate, &err)) {
         if (tstate != TEVENT_REQ_USER_ERROR) {
@@ -228,12 +234,9 @@ fail:
                   (int)err, strerror(err)));
 
         if (sdap_check_gssapi_reconnect(state->ctx)) {
-            talloc_zfree(state->ctx->gsh);
-            subreq = enum_users_send(state, state->ev, state->ctx, state->purge);
-            if (subreq != NULL) {
-                tevent_req_set_callback(subreq, ldap_id_enum_users_done, req);
-                return;
-            }
+            state->ctx->gsh->connected = false;
+            ret = ldap_id_enum_users_restart(req);
+            if (ret == EOK) return;
         }
         sdap_mark_offline(state->ctx);
     }
@@ -250,6 +253,7 @@ static void ldap_id_enum_groups_done(struct tevent_req *subreq)
                                                  struct global_enum_state);
     enum tevent_req_state tstate;
     uint64_t err = 0;
+    int ret;
 
     if (tevent_req_is_error(subreq, &tstate, &err)) {
         if (tstate != TEVENT_REQ_USER_ERROR) {
@@ -278,12 +282,9 @@ static void ldap_id_enum_groups_done(struct tevent_req *subreq)
 fail:
     /* check if credentials are expired otherwise go offline on failures */
     if (sdap_check_gssapi_reconnect(state->ctx)) {
-        talloc_zfree(state->ctx->gsh);
-        subreq = enum_groups_send(state, state->ev, state->ctx, state->purge);
-        if (subreq != NULL) {
-            tevent_req_set_callback(subreq, ldap_id_enum_groups_done, req);
-            return;
-        }
+        state->ctx->gsh->connected = false;
+        ret = ldap_id_enum_groups_restart(req);
+        if (ret == EOK) return;
     }
     sdap_mark_offline(state->ctx);
     DEBUG(1, ("Failed to enumerate groups (%d [%s]), retrying later!\n",
@@ -299,6 +300,88 @@ static void ldap_id_enum_cleanup_done(struct tevent_req *subreq)
     tevent_req_done(req);
 }
 
+static void ldap_id_enum_users_immediate(struct tevent_context *ctx,
+                                         struct tevent_immediate *im,
+                                         void *private_data)
+{
+    struct tevent_req *req = talloc_get_type(private_data,
+                                             struct tevent_req);
+    struct global_enum_state *state = tevent_req_data(req,
+                                                 struct global_enum_state);
+    struct tevent_req *subreq;
+
+    subreq = enum_users_send(state, state->ev, state->ctx, state->purge);
+    if (subreq == NULL) {
+        tevent_req_error(req, ENOMEM);
+        return;
+    }
+    tevent_req_set_callback(subreq, ldap_id_enum_users_done, req);
+}
+
+static int ldap_id_enum_users_restart(struct tevent_req *req)
+{
+    struct global_enum_state *state = tevent_req_data(req,
+                                                 struct global_enum_state);
+    struct tevent_immediate *im;
+
+    state->restarts++;
+    if (state->restarts < MAX_ENUM_RESTARTS) {
+        return ELOOP;
+    }
+
+    im = tevent_create_immediate(req);
+    if (!im) {
+        return ENOMEM;
+    }
+
+    /* schedule a completely new event to avoid deep recursions */
+    tevent_schedule_immediate(im, state->ev,
+                              ldap_id_enum_users_immediate, req);
+
+    return EOK;
+}
+
+static void ldap_id_enum_groups_immediate(struct tevent_context *ctx,
+                                          struct tevent_immediate *im,
+                                          void *private_data)
+{
+    struct tevent_req *req = talloc_get_type(private_data,
+                                             struct tevent_req);
+    struct global_enum_state *state = tevent_req_data(req,
+                                                 struct global_enum_state);
+    struct tevent_req *subreq;
+
+    subreq = enum_groups_send(state, state->ev, state->ctx, state->purge);
+    if (subreq == NULL) {
+        tevent_req_error(req, ENOMEM);
+        return;
+    }
+    tevent_req_set_callback(subreq, ldap_id_enum_groups_done, req);
+}
+
+static int ldap_id_enum_groups_restart(struct tevent_req *req)
+{
+    struct global_enum_state *state = tevent_req_data(req,
+                                                 struct global_enum_state);
+    struct tevent_immediate *im;
+
+    state->restarts++;
+    if (state->restarts < MAX_ENUM_RESTARTS) {
+        return ELOOP;
+    }
+
+    im = tevent_create_immediate(req);
+    if (!im) {
+        return ENOMEM;
+    }
+
+    /* schedule a completely new event to avoid deep recursions */
+    tevent_schedule_immediate(im, state->ev,
+                              ldap_id_enum_groups_immediate, req);
+
+    return EOK;
+}
+
 
 /* ==User-Enumeration===================================================== */
 
@@ -357,8 +440,6 @@ static struct tevent_req *enum_users_send(TALLOC_CTX *memctx,
 
     if (!sdap_connected(ctx)) {
 
-        if (ctx->gsh) talloc_zfree(ctx->gsh);
-
         /* FIXME: add option to decide if tls should be used
          * or SASL/GSSAPI, etc ... */
         subreq = sdap_cli_connect_send(state, ev, ctx->opts,
@@ -512,8 +593,6 @@ static struct tevent_req *enum_groups_send(TALLOC_CTX *memctx,
 
     if (!sdap_connected(ctx)) {
 
-        if (ctx->gsh) talloc_zfree(ctx->gsh);
-
         /* FIXME: add option to decide if tls should be used
          * or SASL/GSSAPI, etc ... */
         subreq = sdap_cli_connect_send(state, ev, ctx->opts,
diff --git a/src/providers/ldap/sdap.h b/src/providers/ldap/sdap.h
index f0e345e..b229b7d 100644
--- a/src/providers/ldap/sdap.h
+++ b/src/providers/ldap/sdap.h
@@ -78,6 +78,9 @@ struct sdap_handle {
 #endif
 
     struct sdap_op *ops;
+
+    bool lock;
+    bool release;
 };
 
 struct sdap_service {
diff --git a/src/providers/ldap/sdap_async.c b/src/providers/ldap/sdap_async.c
index 15f3048..7920266 100644
--- a/src/providers/ldap/sdap_async.c
+++ b/src/providers/ldap/sdap_async.c
@@ -84,43 +84,67 @@ static int sdap_handle_destructor(void *mem)
 {
     struct sdap_handle *sh = talloc_get_type(mem, struct sdap_handle);
 
+    /* if the structure is currently locked, then mark it to be released
+     * and prevent talloc from freeing the memory */
+    if (sh->lock) {
+        sh->release = true;
+        return -1;
+    }
+
     sdap_handle_release(sh);
-
     return 0;
 }
 
 static void sdap_handle_release(struct sdap_handle *sh)
 {
-    DEBUG(8, ("Trace: sh[%p], connected[%d], ops[%p], ldap[%p]\n",
-              sh, (int)sh->connected, sh->ops, sh->ldap));
+    struct sdap_op *op;
 
-    if (sh->connected) {
-        struct sdap_op *op;
+    DEBUG(8, ("Trace: sh[%p], connected[%d], ops[%p], ldap[%p], "
+              "lock[%d], release[%d]\n",
+              sh, (int)sh->connected, sh->ops, sh->ldap,
+              (int)sh->lock, (int)sh->release));
+
+    if (sh->lock) return;
+    sh->lock = true;
+
+    /* make sure nobody tries to reuse this connection from now on */
+    sh->connected = false;
 
 #ifdef HAVE_LDAP_CONNCB
+    if (sh->conncb) {
         /* remove all related fd events from the event loop */
         talloc_zfree(sh->conncb->lc_arg);
+    }
 #else
-        talloc_zfree(sh->fde);
+    talloc_zfree(sh->fde);
 #endif
 
-        while (sh->ops) {
-            op = sh->ops;
-            op->callback(op, NULL, EIO, op->data);
-            /* calling the callback may result in freeing the op */
-            /* check if it is still the same or avoid freeing */
-            if (op == sh->ops) talloc_free(op);
-        }
+    while (sh->ops) {
+        op = sh->ops;
+        op->callback(op, NULL, EIO, op->data);
+        /* calling the callback may result in freeing the op */
+        /* check if it is still the same or avoid freeing */
+        if (op == sh->ops) talloc_free(op);
+    }
 
-        if (sh->ldap) {
-            ldap_unbind_ext(sh->ldap, NULL, NULL);
-        }
-#ifdef HAVE_LDAP_CONNCB
-        talloc_zfree(sh->conncb);
-#endif
-        sh->connected = false;
+    if (sh->ldap) {
+        ldap_unbind_ext(sh->ldap, NULL, NULL);
         sh->ldap = NULL;
-        sh->ops = NULL;
+    }
+#ifdef HAVE_LDAP_CONNCB
+    talloc_zfree(sh->conncb);
+#endif
+
+    /* ok, we have done the job, unlock now */
+    sh->lock = false;
+
+    /* finally if a destructor was ever called, free sh before
+     * exiting */
+    if (sh->release) {
+        /* neutralize the destructor as we already handled
+         * all was needed to be released */
+        talloc_set_destructor((TALLOC_CTX *)sh, NULL);
+        talloc_free(sh);
     }
 }
 
diff --git a/src/providers/ldap/sdap_async_connection.c b/src/providers/ldap/sdap_async_connection.c
index 2acbbb8..f202b70 100644
--- a/src/providers/ldap/sdap_async_connection.c
+++ b/src/providers/ldap/sdap_async_connection.c
@@ -1157,6 +1157,9 @@ int sdap_cli_connect_recv(struct tevent_req *req,
     }
 
     if (gsh) {
+        if (*gsh) {
+            talloc_zfree(*gsh);
+        }
         *gsh = talloc_steal(memctx, state->sh);
         if (!*gsh) {
             return ENOMEM;
-- 
1.6.6.1

