On Fri, 2012-03-02 at 13:52 -0500, Stephen Gallagher wrote:
> This function alters the memory hierarchy of the be_req
> to ensure memory safety during shutdown. It creates a
> spy on the be_cli object so that it will free the be_req
> if the client is freed.
> 
> It is generally allocated atop the private data context
> for the appropriate back-end against which it is being
> filed.
> 
> This should resolve https://fedorahosted.org/sssd/ticket/1226

Simo performed the review on IRC and found one issue (the destructors
should not be set until both spies are active) and made a minor
improvement request.

New patch attached.
From a48db16970e9d2b9eb14a186cd178680a18940b2 Mon Sep 17 00:00:00 2001
From: Stephen Gallagher <sgall...@redhat.com>
Date: Fri, 2 Mar 2012 13:50:03 -0500
Subject: [PATCH] DP: Reorganize memory hierarchy of requests

This function alters the memory hierarchy of the be_req
to ensure memory safety during shutdown. It creates a
spy on the be_cli object so that it will free the be_req
if the client is freed.

It is generally allocated atop the private data context
for the appropriate back-end against which it is being
filed.

https://fedorahosted.org/sssd/ticket/1226
---
 src/providers/data_provider_be.c |  134 +++++++++++++++++++++++++++++++-------
 1 files changed, 110 insertions(+), 24 deletions(-)

diff --git a/src/providers/data_provider_be.c b/src/providers/data_provider_be.c
index 695dee0e5b558dd17c9cb0672473e650578def51..c7bf2f0840dcef40406c7efd13f820901d1a2e4f 100644
--- a/src/providers/data_provider_be.c
+++ b/src/providers/data_provider_be.c
@@ -132,28 +132,114 @@ static void be_async_req_handler(struct tevent_context *ev,
     async_req->fn(async_req->req);
 }
 
-static int be_file_request(struct be_ctx *ctx,
-                           be_req_fn_t fn,
-                           struct be_req *req)
+struct be_spy {
+    TALLOC_CTX *freectx;
+    struct be_spy *double_agent;
+};
+
+static int be_spy_destructor(struct be_spy *spy)
 {
+    /* If there's a double_agent, set its
+     * freectx to NULL so that we don't
+     * try to loop. When that spy fires,
+     * it will just be a no-op.
+     */
+    spy->double_agent->freectx = NULL;
+    talloc_zfree(spy->freectx);
+    return 0;
+}
+
+static errno_t be_spy_create(TALLOC_CTX *mem_ctx, struct be_req *be_req)
+{
+    errno_t ret;
+    struct be_spy *cli_spy = NULL;
+    struct be_spy *req_spy = NULL;
+
+    /* Attach a spy for the be_client so that if it dies,
+     * we can free the be_req automatically.
+     */
+    cli_spy = talloc_zero(be_req->becli, struct be_spy);
+    if (!cli_spy) {
+        ret = ENOMEM;
+        goto done;
+    }
+    cli_spy->freectx = mem_ctx;
+    talloc_set_destructor(cli_spy, be_spy_destructor);
+
+    /* Also create a spy on the be_req so that we
+     * can free the other spy when the be_req
+     * completes successfully.
+     */
+    req_spy = talloc_zero(be_req, struct be_spy);
+    if (!req_spy) {
+        ret = ENOMEM;
+        goto done;
+    }
+    req_spy->freectx = cli_spy;
+    talloc_set_destructor(req_spy, be_spy_destructor);
+
+    /* Create paired spy links to prevent loops */
+    cli_spy->double_agent = req_spy;
+    req_spy->double_agent = cli_spy;
+
+    /* Now create the destructors that will actually free
+     * the opposing spies.
+     */
+    talloc_set_destructor(cli_spy, be_spy_destructor);
+    talloc_set_destructor(req_spy, be_spy_destructor);
+
+
+    /* Now steal the be_req onto the mem_ctx so that it
+     * will be guaranteed that this data will be
+     * available for the full duration of execution.
+     */
+    talloc_steal(mem_ctx, be_req);
+
+    ret = EOK;
+done:
+    if (ret != EOK) {
+        talloc_free(cli_spy);
+        talloc_free(req_spy);
+    }
+    return ret;
+}
+
+/* This function alters the memory hierarchy of the be_req
+ * to ensure memory safety during shutdown. It creates a
+ * spy on the be_cli object so that it will free the be_req
+ * if the client is freed.
+ *
+ * It is generally allocated atop the private data context
+ * for the appropriate back-end against which it is being
+ * filed.
+ */
+static errno_t be_file_request(TALLOC_CTX *mem_ctx,
+                               struct be_req *be_req,
+                               be_req_fn_t fn)
+{
+    errno_t ret;
     struct be_async_req *areq;
     struct tevent_timer *te;
     struct timeval tv;
 
-    if (!fn || !req) return EINVAL;
+    if (!fn || !be_req) return EINVAL;
 
-    areq = talloc(req, struct be_async_req);
+    ret = be_spy_create(mem_ctx, be_req);
+    if (ret != EOK) return ret;
+
+    areq = talloc(be_req, struct be_async_req);
     if (!areq) {
         return ENOMEM;
     }
     areq->fn = fn;
-    areq->req = req;
+    areq->req = be_req;
 
     /* fire immediately */
     tv.tv_sec = 0;
     tv.tv_usec = 0;
 
-    te = tevent_add_timer(ctx->ev, req, tv, be_async_req_handler, areq);
+    te = tevent_add_timer(be_req->be_ctx->ev, be_req,
+                          tv, be_async_req_handler, areq);
     if (te == NULL) {
         return EIO;
     }
@@ -446,9 +532,9 @@ static int be_get_account_info(DBusMessage *message, struct sbus_connection *con
 
     /* process request */
 
-    ret = be_file_request(becli->bectx,
-                          becli->bectx->bet_info[BET_ID].bet_ops->handler,
-                          be_req);
+    ret = be_file_request(becli->bectx->bet_info[BET_ID].pvt_bet_data,
+                          be_req,
+                          becli->bectx->bet_info[BET_ID].bet_ops->handler);
     if (ret != EOK) {
         err_maj = DP_ERR_FATAL;
         err_min = ret;
@@ -606,9 +692,9 @@ static int be_pam_handler(DBusMessage *message, struct sbus_connection *conn)
 
     be_req->req_data = pd;
 
-    ret = be_file_request(becli->bectx,
-                          becli->bectx->bet_info[target].bet_ops->handler,
-                          be_req);
+    ret = be_file_request(becli->bectx->bet_info[target].pvt_bet_data,
+                          be_req,
+                          becli->bectx->bet_info[target].bet_ops->handler);
     if (ret != EOK) {
         DEBUG(7, ("be_file_request failed.\n"));
         goto done;
@@ -811,9 +897,9 @@ static int be_sudo_handler(DBusMessage *message, struct sbus_connection *conn)
         goto fail;
     }
 
-    ret = be_file_request(be_cli->bectx,
-                          be_cli->bectx->bet_info[BET_SUDO].bet_ops->handler,
-                          be_req);
+    ret = be_file_request(be_cli->bectx->bet_info[BET_SUDO].pvt_bet_data,
+                          be_req,
+                          be_cli->bectx->bet_info[BET_SUDO].bet_ops->handler);
     if (ret != EOK) {
         DEBUG(SSSDBG_CRIT_FAILURE, ("be_file_request failed.\n"));
         err_msg = "Cannot file back end request";
@@ -974,9 +1060,9 @@ static int be_autofs_handler(DBusMessage *message, struct sbus_connection *conn)
         goto done;
     }
 
-    ret = be_file_request(be_cli->bectx,
-                          be_cli->bectx->bet_info[BET_AUTOFS].bet_ops->handler,
-                          be_req);
+    ret = be_file_request(be_cli->bectx->bet_info[BET_AUTOFS].pvt_bet_data,
+                          be_req,
+                          be_cli->bectx->bet_info[BET_AUTOFS].bet_ops->handler);
     if (ret != EOK) {
         DEBUG(SSSDBG_CRIT_FAILURE, ("be_file_request failed.\n"));
         err_maj = DP_ERR_FATAL;
@@ -1196,9 +1282,9 @@ static int be_host_handler(DBusMessage *message, struct sbus_connection *conn)
 
     /* process request */
 
-    ret = be_file_request(becli->bectx,
-                          becli->bectx->bet_info[BET_HOSTID].bet_ops->handler,
-                          be_req);
+    ret = be_file_request(becli->bectx->bet_info[BET_HOSTID].pvt_bet_data,
+                          be_req,
+                          becli->bectx->bet_info[BET_HOSTID].bet_ops->handler);
     if (ret != EOK) {
         err_maj = DP_ERR_FATAL;
         err_min = ret;
@@ -1345,8 +1431,8 @@ static errno_t be_file_check_online_request(struct be_req *req)
     req->be_ctx->offstat.went_offline = time(NULL);
     reset_fo(req->be_ctx);
 
-    ret = be_file_request(req->be_ctx,
-              req->be_ctx->bet_info[BET_ID].bet_ops->check_online, req);
+    ret = be_file_request(req->be_ctx, req,
+                          req->be_ctx->bet_info[BET_ID].bet_ops->check_online);
     if (ret != EOK) {
         DEBUG(1, ("be_file_request failed.\n"));
     }
-- 
1.7.7.6

Attachment: signature.asc
Description: This is a digitally signed message part

_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://fedorahosted.org/mailman/listinfo/sssd-devel

Reply via email to