[pulseaudio-discuss] [PATCH] fix dbus message leaks

[Arun Raghavan]

On Mon, 3 Jul 2017, at 08:05 PM, Tanu Kaskinen wrote:
> I reviewed all places that call
> dbus_connection_send_with_reply_and_block(), and found several places
> where dbus messages aren't properly unreffed.
> ---
>  src/modules/bluetooth/backend-ofono.c |  6 ++++++
>  src/modules/bluetooth/bluez4-util.c   | 12 ++++++++++--
>  src/modules/bluetooth/bluez5-util.c   | 12 ++++++++++--
>  src/modules/reserve.c                 |  6 ++++++
>  4 files changed, 32 insertions(+), 4 deletions(-)
> 
> diff --git a/src/modules/bluetooth/backend-ofono.c
> b/src/modules/bluetooth/backend-ofono.c
> index 6e9a3664e..2c51497f3 100644
> --- a/src/modules/bluetooth/backend-ofono.c
> +++ b/src/modules/bluetooth/backend-ofono.c
> @@ -168,9 +168,15 @@ static int
> hf_audio_agent_transport_acquire(pa_bluetooth_transport *t, bool opti
>          dbus_error_init(&derr);
>          pa_assert_se(m = dbus_message_new_method_call(t->owner, t->path,
>          "org.ofono.HandsfreeAudioCard", "Connect"));
>          r =
>          dbus_connection_send_with_reply_and_block(pa_dbus_connection_get(card->backend->connection),
>          m, -1, &derr);
> +        dbus_message_unref(m);
> +        m = NULL;
> +
>          if (!r)
>              return -1;
>  
> +        dbus_message_unref(r);
> +        r = NULL;
> +
>          if (card->connecting)
>              return -EAGAIN;
>      }
> diff --git a/src/modules/bluetooth/bluez4-util.c
> b/src/modules/bluetooth/bluez4-util.c
> index 82654508f..ca606193d 100644
> --- a/src/modules/bluetooth/bluez4-util.c
> +++ b/src/modules/bluetooth/bluez4-util.c
> @@ -1116,6 +1116,8 @@ int pa_bluez4_transport_acquire(pa_bluez4_transport
> *t, bool optional, size_t *i
>      pa_assert_se(m = dbus_message_new_method_call(t->owner, t->path,
>      "org.bluez.MediaTransport", "Acquire"));
>      pa_assert_se(dbus_message_append_args(m, DBUS_TYPE_STRING,
>      &accesstype, DBUS_TYPE_INVALID));
>      r =
>      dbus_connection_send_with_reply_and_block(pa_dbus_connection_get(t->device->discovery->connection),
>      m, -1, &err);
> +    dbus_message_unref(m);
> +    m = NULL;
>  
>      if (!r) {
>          dbus_error_free(&err);
> @@ -1143,7 +1145,7 @@ fail:
>  
>  void pa_bluez4_transport_release(pa_bluez4_transport *t) {
>      const char *accesstype = "rw";
> -    DBusMessage *m;
> +    DBusMessage *m, *r;
>      DBusError err;
>  
>      pa_assert(t);
> @@ -1154,7 +1156,13 @@ void
> pa_bluez4_transport_release(pa_bluez4_transport *t) {
>  
>      pa_assert_se(m = dbus_message_new_method_call(t->owner, t->path,
>      "org.bluez.MediaTransport", "Release"));
>      pa_assert_se(dbus_message_append_args(m, DBUS_TYPE_STRING,
>      &accesstype, DBUS_TYPE_INVALID));
> -   
> dbus_connection_send_with_reply_and_block(pa_dbus_connection_get(t->device->discovery->connection),
> m, -1, &err);
> +    r =
> dbus_connection_send_with_reply_and_block(pa_dbus_connection_get(t->device->discovery->connection),
> m, -1, &err);
> +    dbus_message_unref(m);
> +    m = NULL;
> +    if (r) {
> +        dbus_message_unref(r);
> +        r = NULL;
> +    }
>  
>      if (dbus_error_is_set(&err)) {
>          pa_log("Failed to release transport %s: %s", t->path,
>          err.message);
> diff --git a/src/modules/bluetooth/bluez5-util.c
> b/src/modules/bluetooth/bluez5-util.c
> index 8956fb13a..c92832329 100644
> --- a/src/modules/bluetooth/bluez5-util.c
> +++ b/src/modules/bluetooth/bluez5-util.c
> @@ -363,6 +363,8 @@ static int
> bluez5_transport_acquire_cb(pa_bluetooth_transport *t, bool optional,
>      dbus_error_init(&err);
>  
>      r =
>      dbus_connection_send_with_reply_and_block(pa_dbus_connection_get(t->device->discovery->connection),
>      m, -1, &err);
> +    dbus_message_unref(m);
> +    m = NULL;
>      if (!r) {
>          if (optional && pa_streq(err.name,
>          "org.bluez.Error.NotAvailable"))
>              pa_log_info("Failed optional acquire of unavailable
>              transport %s", t->path);
> @@ -393,7 +395,7 @@ finish:
>  }
>  
>  static void bluez5_transport_release_cb(pa_bluetooth_transport *t) {
> -    DBusMessage *m;
> +    DBusMessage *m, *r;
>      DBusError err;
>  
>      pa_assert(t);
> @@ -408,7 +410,13 @@ static void
> bluez5_transport_release_cb(pa_bluetooth_transport *t) {
>      }
>  
>      pa_assert_se(m = dbus_message_new_method_call(t->owner, t->path,
>      BLUEZ_MEDIA_TRANSPORT_INTERFACE, "Release"));
> -   
> dbus_connection_send_with_reply_and_block(pa_dbus_connection_get(t->device->discovery->connection),
> m, -1, &err);
> +    r =
> dbus_connection_send_with_reply_and_block(pa_dbus_connection_get(t->device->discovery->connection),
> m, -1, &err);
> +    dbus_message_unref(m);
> +    m = NULL;
> +    if (r) {
> +        dbus_message_unref(r);
> +        r = NULL;
> +    }
>  
>      if (dbus_error_is_set(&err)) {
>          pa_log_error("Failed to release transport %s: %s", t->path,
>          err.message);
> diff --git a/src/modules/reserve.c b/src/modules/reserve.c
> index f78805ed7..b0038e662 100644
> --- a/src/modules/reserve.c
> +++ b/src/modules/reserve.c
> @@ -474,6 +474,9 @@ int rd_acquire(
>  		goto fail;
>  	}
>  
> +        dbus_message_unref(m);
> +        m = NULL;
> +

Why do you reset all these to NULL here when m/r are never referenced
again? No harm of course, but it seems verbose, and isn't a pattern we
use anywhere else.

-- Arun