From 7ece0bc4b566ab0b7b5596924983d3a84c372836 Mon Sep 17 00:00:00 2001

From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina@redhat.com>

Date: Thu, 16 Aug 2018 13:17:13 +0200

Subject: [PATCH 34/47] sbus: replace sbus_message_bound_ref with

 sbus_message_bound_steal

The memory context used to new message reference accidentally overwrote

the one use by the initial sbus_message_bound call. This caused a memory

leak of message as its reference counter got increased but number of

talloc contexts bound this this message decreased at the same time.

Fixing this is non-trival and it would require separate data slot for

each reference. Because we do not have any existing use case for this

and we use it only as an equivalent of talloc_steal it is better to

provide a real equivalent for this talloc function.

Resolves:

https://pagure.io/SSSD/sssd/issue/3810

Reviewed-by: Jakub Hrozek <jhrozek@redhat.com>

(cherry picked from commit ca50c40511f08c0f7c786598e5793a06789c6cce)

---

 src/responder/ifp/ifp_iface/sbus_ifp_client_sync.c |  4 +-

 src/sbus/codegen/templates/client_async.c.tpl      |  4 +-

 src/sbus/codegen/templates/client_sync.c.tpl       |  4 +-

 src/sbus/interface_dbus/sbus_dbus_client_async.c   |  8 ++--

 src/sbus/interface_dbus/sbus_dbus_client_sync.c    |  8 ++--

 src/sbus/request/sbus_message.c                    | 51 +++++++++++++++++-----

 src/sbus/request/sbus_request.c                    | 10 ++---

 src/sbus/request/sbus_request_call.c               |  5 +--

 src/sbus/sbus_message.h                            |  8 +---

 src/sbus/sync/sbus_sync_call.c                     |  5 +--

 10 files changed, 65 insertions(+), 42 deletions(-)

diff --git a/src/responder/ifp/ifp_iface/sbus_ifp_client_sync.c b/src/responder/ifp/ifp_iface/sbus_ifp_client_sync.c

index 4859b93ea8fe793f1cca3712663aedd25de25a86..1f0a8e367905e20e921e9a31714b9c7de53f47cd 100644

--- a/src/responder/ifp/ifp_iface/sbus_ifp_client_sync.c

+++ b/src/responder/ifp/ifp_iface/sbus_ifp_client_sync.c

@@ -526,9 +526,9 @@ sbus_method_in_sas_out_raw

         goto done;

     }

-    ret = sbus_message_bound_ref(mem_ctx, reply);

+    ret = sbus_message_bound_steal(mem_ctx, reply);

     if (ret != EOK) {

-        DEBUG(SSSDBG_CRIT_FAILURE, "Unable to bound message [%d]: %s\n",

+        DEBUG(SSSDBG_CRIT_FAILURE, "Unable to steal message [%d]: %s\n",

               ret, sss_strerror(ret));

         goto done;

     }

diff --git a/src/sbus/codegen/templates/client_async.c.tpl b/src/sbus/codegen/templates/client_async.c.tpl

index 6ffb4f83c77bd33653011bfcf5008ce86a89e099..e16ce42c7f97e3b4b564570fb73faaa9a5c274c8 100644

--- a/src/sbus/codegen/templates/client_async.c.tpl

+++ b/src/sbus/codegen/templates/client_async.c.tpl

@@ -193,9 +193,9 @@

             return EINVAL;

         }

-        ret = sbus_message_bound_ref(mem_ctx, state->reply);

+        ret = sbus_message_bound_steal(mem_ctx, state->reply);

         if (ret != EOK) {

-            DEBUG(SSSDBG_CRIT_FAILURE, "Unable to bound message [%d]: %s\n",

+            DEBUG(SSSDBG_CRIT_FAILURE, "Unable to steal message [%d]: %s\n",

                   ret, sss_strerror(ret));

             return ret;

         }

diff --git a/src/sbus/codegen/templates/client_sync.c.tpl b/src/sbus/codegen/templates/client_sync.c.tpl

index 30fa009fe6f010483ff58d369451c272dfdbd3ec..fe9a3a4726014aa2bcb221a1bbcc949f7d900237 100644

--- a/src/sbus/codegen/templates/client_sync.c.tpl

+++ b/src/sbus/codegen/templates/client_sync.c.tpl

@@ -110,9 +110,9 @@

             goto done;

         }

-        ret = sbus_message_bound_ref(mem_ctx, reply);

+        ret = sbus_message_bound_steal(mem_ctx, reply);

         if (ret != EOK) {

-            DEBUG(SSSDBG_CRIT_FAILURE, "Unable to bound message [%d]: %s\n",

+            DEBUG(SSSDBG_CRIT_FAILURE, "Unable to steal message [%d]: %s\n",

                   ret, sss_strerror(ret));

             goto done;

         }

diff --git a/src/sbus/interface_dbus/sbus_dbus_client_async.c b/src/sbus/interface_dbus/sbus_dbus_client_async.c

index 9dbd72cedc95e328d6659283e959c554c39797dc..0060e8b91d5d0c2073558818bd529fda9c97b3f8 100644

--- a/src/sbus/interface_dbus/sbus_dbus_client_async.c

+++ b/src/sbus/interface_dbus/sbus_dbus_client_async.c

@@ -301,9 +301,9 @@ sbus_method_in_s_out_raw_recv

         return EINVAL;

     }

-    ret = sbus_message_bound_ref(mem_ctx, state->reply);

+    ret = sbus_message_bound_steal(mem_ctx, state->reply);

     if (ret != EOK) {

-        DEBUG(SSSDBG_CRIT_FAILURE, "Unable to bound message [%d]: %s\n",

+        DEBUG(SSSDBG_CRIT_FAILURE, "Unable to steal message [%d]: %s\n",

               ret, sss_strerror(ret));

         return ret;

     }

@@ -513,9 +513,9 @@ sbus_method_in_ss_out_raw_recv

         return EINVAL;

     }

-    ret = sbus_message_bound_ref(mem_ctx, state->reply);

+    ret = sbus_message_bound_steal(mem_ctx, state->reply);

     if (ret != EOK) {

-        DEBUG(SSSDBG_CRIT_FAILURE, "Unable to bound message [%d]: %s\n",

+        DEBUG(SSSDBG_CRIT_FAILURE, "Unable to steal message [%d]: %s\n",

               ret, sss_strerror(ret));

         return ret;

     }

diff --git a/src/sbus/interface_dbus/sbus_dbus_client_sync.c b/src/sbus/interface_dbus/sbus_dbus_client_sync.c

index a0473cd377e97021acea594b48b52f4aa565bad9..3ab0aab452d6b1acb702d577087b1c9fd50b4340 100644

--- a/src/sbus/interface_dbus/sbus_dbus_client_sync.c

+++ b/src/sbus/interface_dbus/sbus_dbus_client_sync.c

@@ -101,9 +101,9 @@ sbus_method_in_s_out_raw

         goto done;

     }

-    ret = sbus_message_bound_ref(mem_ctx, reply);

+    ret = sbus_message_bound_steal(mem_ctx, reply);

     if (ret != EOK) {

-        DEBUG(SSSDBG_CRIT_FAILURE, "Unable to bound message [%d]: %s\n",

+        DEBUG(SSSDBG_CRIT_FAILURE, "Unable to steal message [%d]: %s\n",

               ret, sss_strerror(ret));

         goto done;

     }

@@ -159,9 +159,9 @@ sbus_method_in_ss_out_raw

         goto done;

     }

-    ret = sbus_message_bound_ref(mem_ctx, reply);

+    ret = sbus_message_bound_steal(mem_ctx, reply);

     if (ret != EOK) {

-        DEBUG(SSSDBG_CRIT_FAILURE, "Unable to bound message [%d]: %s\n",

+        DEBUG(SSSDBG_CRIT_FAILURE, "Unable to steal message [%d]: %s\n",

               ret, sss_strerror(ret));

         goto done;

     }

diff --git a/src/sbus/request/sbus_message.c b/src/sbus/request/sbus_message.c

index 7314fd724dd3daec520ba0d1fdd2974995446e8c..90c6df40c7882e1f7232d718f8b4a9d1626f755d 100644

--- a/src/sbus/request/sbus_message.c

+++ b/src/sbus/request/sbus_message.c

@@ -29,8 +29,9 @@

 #include "sbus/interface/sbus_iterator_writers.h"

 /* Data slot that is used for message data. The slot is shared for all

- * messages. */

-dbus_int32_t data_slot = -1;

+ * messages, i.e. when a data slot is allocated all messages have the

+ * slot available. */

+dbus_int32_t global_data_slot = -1;

 struct sbus_talloc_msg {

     DBusMessage *msg;

@@ -48,7 +49,7 @@ static int sbus_talloc_msg_destructor(struct sbus_talloc_msg *talloc_msg)

     /* There may exist more references to this message but this talloc

      * context is no longer valid. We remove dbus message data to invoke

      * dbus destructor now. */

-    dbus_message_set_data(talloc_msg->msg, data_slot, NULL, NULL);

+    dbus_message_set_data(talloc_msg->msg, global_data_slot, NULL, NULL);

     dbus_message_unref(talloc_msg->msg);

     return 0;

 }

@@ -60,7 +61,7 @@ static void sbus_msg_data_destructor(void *ctx)

     talloc_msg = talloc_get_type(ctx, struct sbus_talloc_msg);

     /* Decrement ref counter on data slot. */

-    dbus_message_free_data_slot(&data_slot);

+    dbus_message_free_data_slot(&global_data_slot);

     if (!talloc_msg->in_talloc_destructor) {

         /* References to this message dropped to zero but through

@@ -100,7 +101,8 @@ sbus_message_bound(TALLOC_CTX *mem_ctx, DBusMessage *msg)

     /* Allocate a dbus message data slot that will contain pointer to the

      * talloc context so we can pick up cases when the dbus message is

      * freed through dbus api. */

-    bret = dbus_message_allocate_data_slot(&data_slot);

+

+    bret = dbus_message_allocate_data_slot(&global_data_slot);

     if (!bret) {

         DEBUG(SSSDBG_CRIT_FAILURE, "Unable to allocate data slot!\n");

         talloc_free(talloc_msg);

@@ -108,11 +110,11 @@ sbus_message_bound(TALLOC_CTX *mem_ctx, DBusMessage *msg)

     }

     free_fn = sbus_msg_data_destructor;

-    bret = dbus_message_set_data(msg, data_slot, talloc_msg, free_fn);

+    bret = dbus_message_set_data(msg, global_data_slot, talloc_msg, free_fn);

     if (!bret) {

         DEBUG(SSSDBG_CRIT_FAILURE, "Unable to set message data!\n");

         talloc_free(talloc_msg);

-        dbus_message_free_data_slot(&data_slot);

+        dbus_message_free_data_slot(&global_data_slot);

         return ENOMEM;

     }

@@ -125,15 +127,44 @@ sbus_message_bound(TALLOC_CTX *mem_ctx, DBusMessage *msg)

 }

 errno_t

-sbus_message_bound_ref(TALLOC_CTX *mem_ctx, DBusMessage *msg)

+sbus_message_bound_steal(TALLOC_CTX *mem_ctx, DBusMessage *msg)

 {

+    struct sbus_talloc_msg *talloc_msg;

+    void *data;

+

+    if (mem_ctx == NULL) {

+        DEBUG(SSSDBG_CRIT_FAILURE, "Warning: bounding to NULL context!\n");

+        return EINVAL;

+    }

+

     if (msg == NULL) {

         DEBUG(SSSDBG_CRIT_FAILURE, "Message can not be NULL!\n");

         return EINVAL;

     }

-    dbus_message_ref(msg);

-    return sbus_message_bound(mem_ctx, msg);

+    if (global_data_slot < 0) {

+        DEBUG(SSSDBG_CRIT_FAILURE, "This message is not talloc-bound! "

+              "(data slot < 0)\n");

+        return ERR_INTERNAL;

+    }

+

+    data = dbus_message_get_data(msg, global_data_slot);

+    if (data == NULL) {

+        DEBUG(SSSDBG_CRIT_FAILURE, "This message is not talloc-bound! "

+              "(returned data is NULL)\n");

+        return ERR_INTERNAL;

+    }

+

+    talloc_msg = talloc_get_type(data, struct sbus_talloc_msg);

+    if (talloc_msg == NULL) {

+        DEBUG(SSSDBG_CRIT_FAILURE, "This message is not talloc-bound! "

+              "(invalid data)\n");

+        return ERR_INTERNAL;

+    }

+

+    talloc_steal(mem_ctx, talloc_msg);

+

+    return EOK;

 }

 DBusMessage *

diff --git a/src/sbus/request/sbus_request.c b/src/sbus/request/sbus_request.c

index 3d0e2f9e5b0283da7f1d778bf86262db997f12cd..1ccd01e7d571df3c8e196ce7923c8e04523a3b04 100644

--- a/src/sbus/request/sbus_request.c

+++ b/src/sbus/request/sbus_request.c

@@ -564,10 +564,9 @@ sbus_incoming_request_recv(TALLOC_CTX *mem_ctx,

         return EOK;

     }

-    /* Create new reference to the reply and bound it with caller mem_ctx. */

-    ret = sbus_message_bound_ref(mem_ctx, state->reply);

+    ret = sbus_message_bound_steal(mem_ctx, state->reply);

     if (ret != EOK) {

-        DEBUG(SSSDBG_CRIT_FAILURE, "Unable to bound message [%d]: %s\n",

+        DEBUG(SSSDBG_CRIT_FAILURE, "Unable to steal message [%d]: %s\n",

               ret, sss_strerror(ret));

         return ret;

     }

@@ -709,10 +708,9 @@ sbus_outgoing_request_recv(TALLOC_CTX *mem_ctx,

     TEVENT_REQ_RETURN_ON_ERROR(req);

-    /* Create new reference to the reply and bound it with caller mem_ctx. */

-    ret = sbus_message_bound_ref(mem_ctx, state->reply);

+    ret = sbus_message_bound_steal(mem_ctx, state->reply);

     if (ret != EOK) {

-        DEBUG(SSSDBG_CRIT_FAILURE, "Unable to bound message [%d]: %s\n",

+        DEBUG(SSSDBG_CRIT_FAILURE, "Unable to steal message [%d]: %s\n",

               ret, sss_strerror(ret));

         return ret;

     }

diff --git a/src/sbus/request/sbus_request_call.c b/src/sbus/request/sbus_request_call.c

index 1cf58bdd0aecc5814c24c8f0b87864d91bafd094..cf2a6e5bfb7d403a413b6fc06225b0e7e4b663f3 100644

--- a/src/sbus/request/sbus_request_call.c

+++ b/src/sbus/request/sbus_request_call.c

@@ -126,10 +126,9 @@ sbus_call_method_recv(TALLOC_CTX *mem_ctx,

     TEVENT_REQ_RETURN_ON_ERROR(req);

-    /* Create new reference to the reply and bound it with caller mem_ctx. */

-    ret = sbus_message_bound_ref(mem_ctx, state->reply);

+    ret = sbus_message_bound_steal(mem_ctx, state->reply);

     if (ret != EOK) {

-        DEBUG(SSSDBG_CRIT_FAILURE, "Unable to bound message [%d]: %s\n",

+        DEBUG(SSSDBG_CRIT_FAILURE, "Unable to steal message [%d]: %s\n",

               ret, sss_strerror(ret));

         return ret;

     }

diff --git a/src/sbus/sbus_message.h b/src/sbus/sbus_message.h

index 92d5cea83b3c19ac19701849972a82ce67b09849..e7b8fe5942d993fb31740465c6cdbf2797ab0db4 100644

--- a/src/sbus/sbus_message.h

+++ b/src/sbus/sbus_message.h

@@ -45,11 +45,7 @@ errno_t

 sbus_message_bound(TALLOC_CTX *mem_ctx, DBusMessage *msg);

 /**

- * Reference the message and bound it with talloc context.

- *

- * DO NOT USE dbus_message_unref() on such message anymore since it would not

- * release internal data about the bound. The message will be automatically

- * unreferenced when the talloc context is freed.

+ * Steal previously bound D-Bus message to a new talloc parent.

  *

  * @param mem_ctx Memory context to bound the message with. It can not be NULL.

  * @param msg     Message to be bound with memory context.

@@ -57,7 +53,7 @@ sbus_message_bound(TALLOC_CTX *mem_ctx, DBusMessage *msg);

  * @return EOK on success, other errno code on error.

  */

 errno_t

-sbus_message_bound_ref(TALLOC_CTX *mem_ctx, DBusMessage *msg);

+sbus_message_bound_steal(TALLOC_CTX *mem_ctx, DBusMessage *msg);

 /**

  * Create an empty D-Bus method call.

diff --git a/src/sbus/sync/sbus_sync_call.c b/src/sbus/sync/sbus_sync_call.c

index 8549e5831d4320ffc7831ce8a67f382682d891bb..a4f8a5cc40f4b517fba902ff0dc90d4449d5b3ef 100644

--- a/src/sbus/sync/sbus_sync_call.c

+++ b/src/sbus/sync/sbus_sync_call.c

@@ -63,10 +63,9 @@ sbus_sync_call_method(TALLOC_CTX *mem_ctx,

         goto done;

     }

-    /* Create new reference to the reply and bound it with caller mem_ctx. */

-    ret = sbus_message_bound_ref(mem_ctx, reply);

+    ret = sbus_message_bound_steal(mem_ctx, reply);

     if (ret != EOK) {

-        DEBUG(SSSDBG_CRIT_FAILURE, "Unable to bound message [%d]: %s\n",

+        DEBUG(SSSDBG_CRIT_FAILURE, "Unable to steal message [%d]: %s\n",

               ret, sss_strerror(ret));

         goto done;

     }

-- 

2.14.4