bluetooth: Free backends before devices and adapters.

Submitted by Juho Hämäläinen on June 28, 2018, 11:58 a.m.

Details

Message ID 1530187080-8769-1-git-send-email-jusa@hilvi.org
State New
Series "bluetooth: Free backends before devices and adapters."
Headers show

Commit Message

Juho Hämäläinen June 28, 2018, 11:58 a.m.
When bluetooth daemon disappears gracefully transports are freed before
the daemon disappears from DBus bus. However if bluetooth daemon
segfaults or is killed abruptly the daemon disappears from the bus
before PulseAudio is able to clean the transports. As the devices
and adapters are freed before ofono or native backends, PulseAudio
segfaults when dangling pointers are used when freeing the backends.
Fix by freeing the backends before devices and adapters when bluetooth
daemon disappears from DBus bus.

Signed-off-by: Juho Hämäläinen <jusa@hilvi.org>
---
 src/modules/bluetooth/bluez5-util.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/modules/bluetooth/bluez5-util.c b/src/modules/bluetooth/bluez5-util.c
index 2d83373..f71e458 100644
--- a/src/modules/bluetooth/bluez5-util.c
+++ b/src/modules/bluetooth/bluez5-util.c
@@ -1125,9 +1125,6 @@  static DBusHandlerResult filter_cb(DBusConnection *bus, DBusMessage *m, void *us
         if (pa_streq(name, BLUEZ_SERVICE)) {
             if (old_owner && *old_owner) {
                 pa_log_debug("Bluetooth daemon disappeared");
-                pa_hashmap_remove_all(y->devices);
-                pa_hashmap_remove_all(y->adapters);
-                y->objects_listed = false;
                 if (y->ofono_backend) {
                     pa_bluetooth_ofono_backend_free(y->ofono_backend);
                     y->ofono_backend = NULL;
@@ -1136,6 +1133,9 @@  static DBusHandlerResult filter_cb(DBusConnection *bus, DBusMessage *m, void *us
                     pa_bluetooth_native_backend_free(y->native_backend);
                     y->native_backend = NULL;
                 }
+                pa_hashmap_remove_all(y->devices);
+                pa_hashmap_remove_all(y->adapters);
+                y->objects_listed = false;
             }
 
             if (new_owner && *new_owner) {

Comments

Tanu Kaskinen June 29, 2018, 3:54 p.m.
On Thu, 2018-06-28 at 14:58 +0300, Juho Hämäläinen wrote:
> When bluetooth daemon disappears gracefully transports are freed before
> the daemon disappears from DBus bus. However if bluetooth daemon
> segfaults or is killed abruptly the daemon disappears from the bus
> before PulseAudio is able to clean the transports. As the devices
> and adapters are freed before ofono or native backends, PulseAudio
> segfaults when dangling pointers are used when freeing the backends.
> Fix by freeing the backends before devices and adapters when bluetooth
> daemon disappears from DBus bus.
> 
> Signed-off-by: Juho Hämäläinen <jusa@hilvi.org>
> ---
>  src/modules/bluetooth/bluez5-util.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)

I suppose it's the hf_audio_card.transport pointers that are dangling?

Rather than having unobvious ordering requirements, it would seem
better to me to notify the backends when the transports are freed so
that they can drop their references. There's already the destroy()
callback that could be used for this. What do you think?

(Sidenote: the transport ownership semantics are pretty messy. The
backends call transport_new(), but they don't necessarily call
transport_free(). I guess this is justifiable, because the transports
have to be freed when devices are freed anyway, so to fix this there
would have to be some kind of callback that device_free() would call,
and the callback would just call transport_free(), so maybe it's fine
that we currently cut the complexity by calling transport_free()
directly from device_free()...)
Juho Hämäläinen July 1, 2018, 6:47 p.m.
On [ pe 29.06.18 18:54 ], Tanu Kaskinen wrote:
> On Thu, 2018-06-28 at 14:58 +0300, Juho Hämäläinen wrote:
> > When bluetooth daemon disappears gracefully transports are freed before
> > the daemon disappears from DBus bus. However if bluetooth daemon
> > segfaults or is killed abruptly the daemon disappears from the bus
> > before PulseAudio is able to clean the transports. As the devices
> > and adapters are freed before ofono or native backends, PulseAudio
> > segfaults when dangling pointers are used when freeing the backends.
> > Fix by freeing the backends before devices and adapters when bluetooth
> > daemon disappears from DBus bus.
> > 
> > Signed-off-by: Juho Hämäläinen <jusa@hilvi.org>
> > ---
> >  src/modules/bluetooth/bluez5-util.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> I suppose it's the hf_audio_card.transport pointers that are dangling?
> 
> Rather than having unobvious ordering requirements, it would seem
> better to me to notify the backends when the transports are freed so
> that they can drop their references. There's already the destroy()
> callback that could be used for this. What do you think?

I was thinking the same, using destroy() callback with hf_audio_card,
but as it is more or less reasonable to free the transports before the
devices etc, I went with the simpler reordering instead. I can make new
patch with the destroy() callback as well.

- jusa
Tanu Kaskinen July 2, 2018, 2:40 p.m.
On Sun, 2018-07-01 at 21:47 +0300, Juho Hämäläinen wrote:
> On [ pe 29.06.18 18:54 ], Tanu Kaskinen wrote:
> > On Thu, 2018-06-28 at 14:58 +0300, Juho Hämäläinen wrote:
> > > When bluetooth daemon disappears gracefully transports are freed before
> > > the daemon disappears from DBus bus. However if bluetooth daemon
> > > segfaults or is killed abruptly the daemon disappears from the bus
> > > before PulseAudio is able to clean the transports. As the devices
> > > and adapters are freed before ofono or native backends, PulseAudio
> > > segfaults when dangling pointers are used when freeing the backends.
> > > Fix by freeing the backends before devices and adapters when bluetooth
> > > daemon disappears from DBus bus.
> > > 
> > > Signed-off-by: Juho Hämäläinen <jusa@hilvi.org>
> > > ---
> > >  src/modules/bluetooth/bluez5-util.c | 6 +++---
> > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > I suppose it's the hf_audio_card.transport pointers that are dangling?
> > 
> > Rather than having unobvious ordering requirements, it would seem
> > better to me to notify the backends when the transports are freed so
> > that they can drop their references. There's already the destroy()
> > callback that could be used for this. What do you think?
> 
> I was thinking the same, using destroy() callback with hf_audio_card,
> but as it is more or less reasonable to free the transports before the
> devices etc, I went with the simpler reordering instead. I can make new
> patch with the destroy() callback as well.

Thanks, I would prefer a new patch.