[pulseaudio-discuss] [PATCH v0 14/16] bluetooth: Fix double free with reference counting

Mikel Astiz mikel.astiz.oss at gmail.com
Fri Sep 28 08:45:38 PDT 2012


From: Mikel Astiz <mikel.astiz at bmw-carit.de>

Instead of repeatedly asking the discovery API to find a transport given
our transport path, let's use the reference counting mechanism to hold
a pointer to the transport. This makes the code easier to follow and
slightly more efficient.

Additionally, the change fixes a significant problem: without this
reference counting, the transport could be destroyed while the hook
slots (i.e. nrec_changed_slot) were still set. This led to a double
free of these objects in stop_thread().
---
 src/modules/bluetooth/module-bluetooth-device.c | 60 +++++++++++--------------
 1 file changed, 25 insertions(+), 35 deletions(-)

diff --git a/src/modules/bluetooth/module-bluetooth-device.c b/src/modules/bluetooth/module-bluetooth-device.c
index 68838e7..1328c5f 100644
--- a/src/modules/bluetooth/module-bluetooth-device.c
+++ b/src/modules/bluetooth/module-bluetooth-device.c
@@ -144,7 +144,7 @@ struct userdata {
 
     char *address;
     char *path;
-    char *transport;
+    pa_bluetooth_transport *transport;
     char *accesstype;
 
     pa_bluetooth_discovery *discovery;
@@ -346,18 +346,15 @@ static void teardown_stream(struct userdata *u) {
 }
 
 static void bt_transport_release(struct userdata *u) {
-    const char *accesstype = "rw";
-    const pa_bluetooth_transport *t;
+    pa_assert(u->transport);
 
     /* Ignore if already released */
     if (!bt_transport_is_acquired(u))
         return;
 
-    pa_log_debug("Releasing transport %s", u->transport);
+    pa_log_debug("Releasing transport %s", u->transport->path);
 
-    t = pa_bluetooth_discovery_get_transport(u->discovery, u->transport);
-    if (t)
-        pa_bluetooth_transport_release(t, accesstype);
+    pa_bluetooth_transport_release(u->transport, u->accesstype);
 
     pa_xfree(u->accesstype);
     u->accesstype = NULL;
@@ -385,12 +382,8 @@ static pa_bt_audio_state_t get_profile_audio_state(const struct userdata *u, con
 static int bt_transport_acquire(struct userdata *u, pa_bool_t start) {
     const char *accesstype = "rw";
     const pa_bluetooth_device *d;
-    const pa_bluetooth_transport *t;
 
-    if (u->transport == NULL) {
-        pa_log("Transport no longer available.");
-        return -1;
-    }
+    pa_assert(u->transport);
 
     if (bt_transport_is_acquired(u)) {
         if (start)
@@ -398,7 +391,7 @@ static int bt_transport_acquire(struct userdata *u, pa_bool_t start) {
         return 0;
     }
 
-    pa_log_debug("Acquiring transport %s", u->transport);
+    pa_log_debug("Acquiring transport %s", u->transport->path);
 
     d = pa_bluetooth_discovery_get_by_path(u->discovery, u->path);
     if (!d) {
@@ -406,14 +399,6 @@ static int bt_transport_acquire(struct userdata *u, pa_bool_t start) {
         return -1;
     }
 
-    t = pa_bluetooth_discovery_get_transport(u->discovery, u->transport);
-    if (!t) {
-        pa_log("Transport %s no longer available", u->transport);
-        pa_xfree(u->transport);
-        u->transport = NULL;
-        return -1;
-    }
-
     if (!start) {
         /* FIXME: we are trying to acquire the transport only if the stream is
            playing, without actually initiating the stream request from our side
@@ -423,29 +408,29 @@ static int bt_transport_acquire(struct userdata *u, pa_bool_t start) {
            stream will not be requested until BlueZ's API supports this
            atomically. */
         if (get_profile_audio_state(u, d) < PA_BT_AUDIO_STATE_PLAYING) {
-            pa_log_info("Failed optional acquire of transport %s", u->transport);
+            pa_log_info("Failed optional acquire of transport %s", u->transport->path);
             return -1;
         }
     }
 
-    u->stream_fd = pa_bluetooth_transport_acquire(t, accesstype, &u->read_link_mtu, &u->write_link_mtu);
+    u->stream_fd = pa_bluetooth_transport_acquire(u->transport, accesstype, &u->read_link_mtu, &u->write_link_mtu);
     if (u->stream_fd < 0) {
         if (start)
-            pa_log("Failed to acquire transport %s", u->transport);
+            pa_log("Failed to acquire transport %s", u->transport->path);
         else
-            pa_log_info("Failed optional acquire of transport %s", u->transport);
+            pa_log_info("Failed optional acquire of transport %s", u->transport->path);
 
         return -1;
     }
 
     u->accesstype = pa_xstrdup(accesstype);
-    pa_log_info("Transport %s acquired: fd %d", u->transport, u->stream_fd);
+    pa_log_info("Transport %s acquired: fd %d", u->transport->path, u->stream_fd);
 
     if (!start)
         return 0;
 
 done:
-    pa_log_info("Transport %s resuming", u->transport);
+    pa_log_info("Transport %s resuming", u->transport->path);
     setup_stream(u);
 
     return 0;
@@ -1321,7 +1306,7 @@ static DBusHandlerResult filter_cb(DBusConnection *bus, DBusMessage *m, void *us
                  dbus_message_get_path(m),
                  dbus_message_get_member(m));
 
-    if (!dbus_message_has_path(m, u->path) && !dbus_message_has_path(m, u->transport))
+    if (!dbus_message_has_path(m, u->path) && (!u->transport || !dbus_message_has_path(m, u->transport->path)))
         goto fail;
 
     if (dbus_message_is_signal(m, "org.bluez.Headset", "SpeakerGainChanged") ||
@@ -1701,6 +1686,8 @@ static int source_set_port_cb(pa_source *s, pa_device_port *p) {
 static int add_sink(struct userdata *u) {
     char *k;
 
+    pa_assert(u->transport);
+
     if (USE_SCO_OVER_PCM(u)) {
         pa_proplist *p;
 
@@ -1778,6 +1765,8 @@ static int add_sink(struct userdata *u) {
 static int add_source(struct userdata *u) {
     char *k;
 
+    pa_assert(u->transport);
+
     if (USE_SCO_OVER_PCM(u)) {
         u->source = u->hsp.sco_source;
         pa_proplist_sets(u->source->proplist, "bluetooth.protocol", profile_to_string(u->profile));
@@ -1837,9 +1826,7 @@ static int add_source(struct userdata *u) {
     }
 
     if ((u->profile == PROFILE_HSP) || (u->profile == PROFILE_HFGW)) {
-        pa_bluetooth_transport *t;
-        t = pa_bluetooth_discovery_get_transport(u->discovery, u->transport);
-        pa_assert(t);
+        pa_bluetooth_transport *t = u->transport;
         pa_proplist_sets(u->source->proplist, "bluetooth.nrec", t->nrec ? "1" : "0");
 
         if (!u->hsp.nrec_changed_slot)
@@ -1863,7 +1850,7 @@ static void bt_transport_config_a2dp(struct userdata *u) {
     struct a2dp_info *a2dp = &u->a2dp;
     a2dp_sbc_t *config;
 
-    t = pa_bluetooth_discovery_get_transport(u->discovery, u->transport);
+    t = u->transport;
     pa_assert(t);
 
     config = (a2dp_sbc_t *) t->config;
@@ -1981,9 +1968,10 @@ static void bt_transport_config(struct userdata *u) {
 /* Run from main thread */
 static int setup_transport(struct userdata *u) {
     const pa_bluetooth_device *d;
-    const pa_bluetooth_transport *t;
+    pa_bluetooth_transport *t;
 
     pa_assert(u);
+    pa_assert(!u->transport);
 
     if (!(d = pa_bluetooth_discovery_get_by_path(u->discovery, u->path))) {
         pa_log_error("Failed to get device object.");
@@ -1997,7 +1985,7 @@ static int setup_transport(struct userdata *u) {
         return -1;
     }
 
-    u->transport = pa_xstrdup(t->path);
+    u->transport = pa_bluetooth_transport_ref(t);
 
     bt_transport_acquire(u, FALSE);
 
@@ -2015,6 +2003,8 @@ static int init_profile(struct userdata *u) {
     if (setup_transport(u) < 0)
         return -1;
 
+    pa_assert(u->transport);
+
     if (u->profile == PROFILE_A2DP ||
         u->profile == PROFILE_HSP ||
         u->profile == PROFILE_HFGW)
@@ -2070,7 +2060,7 @@ static void stop_thread(struct userdata *u) {
 
     if (u->transport) {
         bt_transport_release(u);
-        pa_xfree(u->transport);
+        pa_bluetooth_transport_unref(u->transport);
         u->transport = NULL;
     }
 
-- 
1.7.11.4



More information about the pulseaudio-discuss mailing list