[PATCH v8 0/1] audio/jack: fix use after free segfault

[PATCH v8 0/1] audio/jack: fix use after free segfault

[Geoffrey McRae]

v8:
  * use a local mutex instead of the BQL

Geoffrey McRae (1):
  audio/jack: fix use after free segfault

 audio/jackaudio.c | 55 ++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 42 insertions(+), 13 deletions(-)

-- 
2.20.1

[PATCH v8 1/1] audio/jack: fix use after free segfault

[Geoffrey McRae]

This change registers a bottom handler to close the JACK client
connection when a server shutdown signal is recieved. Without this
libjack2 attempts to "clean up" old clients and causes a use after free
segfault.

Signed-off-by: Geoffrey McRae <geoff@hostfission.com>
---
 audio/jackaudio.c | 55 ++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 42 insertions(+), 13 deletions(-)

diff --git a/audio/jackaudio.c b/audio/jackaudio.c
index 72ed7c4929..572ebea208 100644
--- a/audio/jackaudio.c
+++ b/audio/jackaudio.c
@@ -25,6 +25,7 @@
 #include "qemu/osdep.h"
 #include "qemu/module.h"
 #include "qemu/atomic.h"
+#include "qemu/main-loop.h"
 #include "qemu-common.h"
 #include "audio.h"
 
@@ -63,6 +64,8 @@ typedef struct QJackClient {
     QJackState      state;
     jack_client_t  *client;
     jack_nframes_t  freq;
+    QemuMutex       shutdown_lock;
+    QEMUBH         *shutdown_bh;
 
     struct QJack   *j;
     int             nchannels;
@@ -306,21 +309,27 @@ static int qjack_xrun(void *arg)
     return 0;
 }
 
+static void qjack_shutdown_bh(void *opaque)
+{
+    QJackClient *c = (QJackClient *)opaque;
+    qjack_client_fini(c);
+}
+
 static void qjack_shutdown(void *arg)
 {
     QJackClient *c = (QJackClient *)arg;
     c->state = QJACK_STATE_SHUTDOWN;
+    qemu_bh_schedule(c->shutdown_bh);
 }
 
 static void qjack_client_recover(QJackClient *c)
 {
-    if (c->state == QJACK_STATE_SHUTDOWN) {
-        qjack_client_fini(c);
+    if (c->state != QJACK_STATE_DISCONNECTED) {
+        return;
     }
 
     /* packets is used simply to throttle this */
-    if (c->state == QJACK_STATE_DISCONNECTED &&
-        c->packets % 100 == 0) {
+    if (c->packets % 100 == 0) {
 
         /* if enabled then attempt to recover */
         if (c->enabled) {
@@ -489,15 +498,18 @@ static int qjack_init_out(HWVoiceOut *hw, struct audsettings *as,
     QJackOut *jo  = (QJackOut *)hw;
     Audiodev *dev = (Audiodev *)drv_opaque;
 
-    qjack_client_fini(&jo->c);
-
     jo->c.out       = true;
     jo->c.enabled   = false;
     jo->c.nchannels = as->nchannels;
     jo->c.opt       = dev->u.jack.out;
 
+    jo->c.shutdown_bh = qemu_bh_new(qjack_shutdown_bh, &jo->c);
+    qemu_mutex_init(&jo->c.shutdown_lock);
+
     int ret = qjack_client_init(&jo->c);
     if (ret != 0) {
+        qemu_bh_delete(jo->c.shutdown_bh);
+        qemu_mutex_destroy(&jo->c.shutdown_lock);
         return ret;
     }
 
@@ -525,15 +537,18 @@ static int qjack_init_in(HWVoiceIn *hw, struct audsettings *as,
     QJackIn  *ji  = (QJackIn *)hw;
     Audiodev *dev = (Audiodev *)drv_opaque;
 
-    qjack_client_fini(&ji->c);
-
     ji->c.out       = false;
     ji->c.enabled   = false;
     ji->c.nchannels = as->nchannels;
     ji->c.opt       = dev->u.jack.in;
 
+    ji->c.shutdown_bh = qemu_bh_new(qjack_shutdown_bh, &ji->c);
+    qemu_mutex_init(&ji->c.shutdown_lock);
+
     int ret = qjack_client_init(&ji->c);
     if (ret != 0) {
+        qemu_bh_delete(ji->c.shutdown_bh);
+        qemu_mutex_destroy(&ji->c.shutdown_lock);
         return ret;
     }
 
@@ -555,7 +570,7 @@ static int qjack_init_in(HWVoiceIn *hw, struct audsettings *as,
     return 0;
 }
 
-static void qjack_client_fini(QJackClient *c)
+static void qjack_client_fini_locked(QJackClient *c)
 {
     switch (c->state) {
     case QJACK_STATE_RUNNING:
@@ -564,28 +579,42 @@ static void qjack_client_fini(QJackClient *c)
 
     case QJACK_STATE_SHUTDOWN:
         jack_client_close(c->client);
+        c->client = NULL;
+
+        qjack_buffer_free(&c->fifo);
+        g_free(c->port);
+
+        c->state = QJACK_STATE_DISCONNECTED;
         /* fallthrough */
 
     case QJACK_STATE_DISCONNECTED:
         break;
     }
+}
 
-    qjack_buffer_free(&c->fifo);
-    g_free(c->port);
-
-    c->state = QJACK_STATE_DISCONNECTED;
+static void qjack_client_fini(QJackClient *c)
+{
+    qemu_mutex_lock(&c->shutdown_lock);
+    qjack_client_fini_locked(c);
+    qemu_mutex_unlock(&c->shutdown_lock);
 }
 
 static void qjack_fini_out(HWVoiceOut *hw)
 {
     QJackOut *jo = (QJackOut *)hw;
     qjack_client_fini(&jo->c);
+
+    qemu_bh_delete(jo->c.shutdown_bh);
+    qemu_mutex_destroy(&jo->c.shutdown_lock);
 }
 
 static void qjack_fini_in(HWVoiceIn *hw)
 {
     QJackIn *ji = (QJackIn *)hw;
     qjack_client_fini(&ji->c);
+
+    qemu_bh_delete(ji->c.shutdown_bh);
+    qemu_mutex_destroy(&ji->c.shutdown_lock);
 }
 
 static void qjack_enable_out(HWVoiceOut *hw, bool enable)
-- 
2.20.1

Re: [PATCH v8 1/1] audio/jack: fix use after free segfault

[Christian Schoenebeck]

On Freitag, 21. August 2020 15:45:54 CEST Geoffrey McRae wrote:
> This change registers a bottom handler to close the JACK client
> connection when a server shutdown signal is recieved. Without this
> libjack2 attempts to "clean up" old clients and causes a use after free
> segfault.
> 
> Signed-off-by: Geoffrey McRae <geoff@hostfission.com>
> ---
>  audio/jackaudio.c | 55 ++++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 42 insertions(+), 13 deletions(-)
> 
> diff --git a/audio/jackaudio.c b/audio/jackaudio.c
> index 72ed7c4929..572ebea208 100644
> --- a/audio/jackaudio.c
> +++ b/audio/jackaudio.c
> @@ -25,6 +25,7 @@
>  #include "qemu/osdep.h"
>  #include "qemu/module.h"
>  #include "qemu/atomic.h"
> +#include "qemu/main-loop.h"
>  #include "qemu-common.h"
>  #include "audio.h"
> 
> @@ -63,6 +64,8 @@ typedef struct QJackClient {
>      QJackState      state;
>      jack_client_t  *client;
>      jack_nframes_t  freq;
> +    QemuMutex       shutdown_lock;
> +    QEMUBH         *shutdown_bh;
> 
>      struct QJack   *j;
>      int             nchannels;
> @@ -306,21 +309,27 @@ static int qjack_xrun(void *arg)
>      return 0;
>  }
> 
> +static void qjack_shutdown_bh(void *opaque)
> +{
> +    QJackClient *c = (QJackClient *)opaque;
> +    qjack_client_fini(c);
> +}
> +
>  static void qjack_shutdown(void *arg)
>  {
>      QJackClient *c = (QJackClient *)arg;
>      c->state = QJACK_STATE_SHUTDOWN;
> +    qemu_bh_schedule(c->shutdown_bh);
>  }
> 
>  static void qjack_client_recover(QJackClient *c)
>  {
> -    if (c->state == QJACK_STATE_SHUTDOWN) {
> -        qjack_client_fini(c);
> +    if (c->state != QJACK_STATE_DISCONNECTED) {
> +        return;
>      }
> 
>      /* packets is used simply to throttle this */
> -    if (c->state == QJACK_STATE_DISCONNECTED &&
> -        c->packets % 100 == 0) {
> +    if (c->packets % 100 == 0) {
> 
>          /* if enabled then attempt to recover */
>          if (c->enabled) {
> @@ -489,15 +498,18 @@ static int qjack_init_out(HWVoiceOut *hw, struct
> audsettings *as, QJackOut *jo  = (QJackOut *)hw;
>      Audiodev *dev = (Audiodev *)drv_opaque;
> 
> -    qjack_client_fini(&jo->c);
> -
>      jo->c.out       = true;
>      jo->c.enabled   = false;
>      jo->c.nchannels = as->nchannels;
>      jo->c.opt       = dev->u.jack.out;
> 
> +    jo->c.shutdown_bh = qemu_bh_new(qjack_shutdown_bh, &jo->c);
> +    qemu_mutex_init(&jo->c.shutdown_lock);
> +
>      int ret = qjack_client_init(&jo->c);
>      if (ret != 0) {
> +        qemu_bh_delete(jo->c.shutdown_bh);
> +        qemu_mutex_destroy(&jo->c.shutdown_lock);
>          return ret;
>      }
> 
> @@ -525,15 +537,18 @@ static int qjack_init_in(HWVoiceIn *hw, struct
> audsettings *as, QJackIn  *ji  = (QJackIn *)hw;
>      Audiodev *dev = (Audiodev *)drv_opaque;
> 
> -    qjack_client_fini(&ji->c);
> -
>      ji->c.out       = false;
>      ji->c.enabled   = false;
>      ji->c.nchannels = as->nchannels;
>      ji->c.opt       = dev->u.jack.in;
> 
> +    ji->c.shutdown_bh = qemu_bh_new(qjack_shutdown_bh, &ji->c);
> +    qemu_mutex_init(&ji->c.shutdown_lock);
> +
>      int ret = qjack_client_init(&ji->c);
>      if (ret != 0) {
> +        qemu_bh_delete(ji->c.shutdown_bh);
> +        qemu_mutex_destroy(&ji->c.shutdown_lock);
>          return ret;
>      }
> 
> @@ -555,7 +570,7 @@ static int qjack_init_in(HWVoiceIn *hw, struct
> audsettings *as, return 0;
>  }
> 
> -static void qjack_client_fini(QJackClient *c)
> +static void qjack_client_fini_locked(QJackClient *c)
>  {
>      switch (c->state) {
>      case QJACK_STATE_RUNNING:
> @@ -564,28 +579,42 @@ static void qjack_client_fini(QJackClient *c)
> 
>      case QJACK_STATE_SHUTDOWN:
>          jack_client_close(c->client);
> +        c->client = NULL;
> +
> +        qjack_buffer_free(&c->fifo);
> +        g_free(c->port);
> +
> +        c->state = QJACK_STATE_DISCONNECTED;
>          /* fallthrough */
> 
>      case QJACK_STATE_DISCONNECTED:
>          break;
>      }
> +}
> 
> -    qjack_buffer_free(&c->fifo);
> -    g_free(c->port);
> -
> -    c->state = QJACK_STATE_DISCONNECTED;
> +static void qjack_client_fini(QJackClient *c)
> +{
> +    qemu_mutex_lock(&c->shutdown_lock);
> +    qjack_client_fini_locked(c);
> +    qemu_mutex_unlock(&c->shutdown_lock);
>  }
> 
>  static void qjack_fini_out(HWVoiceOut *hw)
>  {
>      QJackOut *jo = (QJackOut *)hw;
>      qjack_client_fini(&jo->c);
> +
> +    qemu_bh_delete(jo->c.shutdown_bh);

Paolo wrapped that qemu_bh_delete() call inside the lock as well. So I guess 
it makes a difference for the BH API?

> +    qemu_mutex_destroy(&jo->c.shutdown_lock);
>  }

Hmmm, is this qemu_mutex_destroy() safe at this point?

> 
>  static void qjack_fini_in(HWVoiceIn *hw)
>  {
>      QJackIn *ji = (QJackIn *)hw;
>      qjack_client_fini(&ji->c);
> +
> +    qemu_bh_delete(ji->c.shutdown_bh);
> +    qemu_mutex_destroy(&ji->c.shutdown_lock);
>  }
> 
>  static void qjack_enable_out(HWVoiceOut *hw, bool enable)

Best regards,
Christian Schoenebeck

Re: [PATCH v8 1/1] audio/jack: fix use after free segfault

[Paolo Bonzini]

On 21/08/20 19:34, Christian Schoenebeck wrote:
>>
>>  static void qjack_fini_out(HWVoiceOut *hw)
>>  {
>>      QJackOut *jo = (QJackOut *)hw;
>>      qjack_client_fini(&jo->c);
>> +
>> +    qemu_bh_delete(jo->c.shutdown_bh);
> Paolo wrapped that qemu_bh_delete() call inside the lock as well. So I guess 
> it makes a difference for the BH API?

It is not a problem as long as qjack_client_fini is idempotent.

>> +    qemu_mutex_destroy(&jo->c.shutdown_lock);
>>  }
> 
> Hmmm, is this qemu_mutex_destroy() safe at this point?

Perhaps make the mutex global and not destroy it at all.

Paolo

Re: [PATCH v8 1/1] audio/jack: fix use after free segfault

[Geoffrey McRae]

On 2020-08-22 03:47, Paolo Bonzini wrote:
> On 21/08/20 19:34, Christian Schoenebeck wrote:
>>> 
>>>  static void qjack_fini_out(HWVoiceOut *hw)
>>>  {
>>>      QJackOut *jo = (QJackOut *)hw;
>>>      qjack_client_fini(&jo->c);
>>> +
>>> +    qemu_bh_delete(jo->c.shutdown_bh);
>> Paolo wrapped that qemu_bh_delete() call inside the lock as well. So I 
>> guess
>> it makes a difference for the BH API?
> 
> It is not a problem as long as qjack_client_fini is idempotent.

`qjack_client_fini` is indeed idempotent

> 
>>> +    qemu_mutex_destroy(&jo->c.shutdown_lock);
>>>  }
>> 
>> Hmmm, is this qemu_mutex_destroy() safe at this point?
> 
> Perhaps make the mutex global and not destroy it at all.

It's safe at this point as `qjack_fini_out` is only called at device 
destruction, and `qjack_client_fini` ensures that JACK is shut down 
which prevents jack from trying to call the shutdown event handler.

> 
> Paolo

Re: [PATCH v8 1/1] audio/jack: fix use after free segfault

[Christian Schoenebeck]

On Samstag, 22. August 2020 02:16:23 CEST Geoffrey McRae wrote:
> On 2020-08-22 03:47, Paolo Bonzini wrote:
> > On 21/08/20 19:34, Christian Schoenebeck wrote:
> >>>  static void qjack_fini_out(HWVoiceOut *hw)
> >>>  {
> >>>  
> >>>      QJackOut *jo = (QJackOut *)hw;
> >>>      qjack_client_fini(&jo->c);
> >>> 
> >>> +
> >>> +    qemu_bh_delete(jo->c.shutdown_bh);
> >> 
> >> Paolo wrapped that qemu_bh_delete() call inside the lock as well. So I
> >> guess
> >> it makes a difference for the BH API?
> > 
> > It is not a problem as long as qjack_client_fini is idempotent.
> 
> `qjack_client_fini` is indeed idempotent

Right.

> >>> +    qemu_mutex_destroy(&jo->c.shutdown_lock);
> >>> 
> >>>  }
> >> 
> >> Hmmm, is this qemu_mutex_destroy() safe at this point?
> > 
> > Perhaps make the mutex global and not destroy it at all.
> 
> It's safe at this point as `qjack_fini_out` is only called at device
> destruction, and `qjack_client_fini` ensures that JACK is shut down
> which prevents jack from trying to call the shutdown event handler.

You mean because jack_client_close() is synchronized. That prevents JACK from 
firing the callback after jack_client_close() returns, that's correct.

But as qemu_bh_delete() is async, you do not have a guarantee that a 
previously scheduled BH shutdown handler is no longer running. So it might 
still hold the lock when you attempt to destroy the mutex.

On doubt I would do like Paolo suggested by making the mutex global and not 
destroying it at all.

Best regards,
Christian Schoenebeck

[PATCH v8 0/1] audio/jack: fix use after free segfault

[Geoffrey McRae]

v9:
  * switch to using a global shutdown mutex

Geoffrey McRae (1):
  audio/jack: fix use after free segfault

 audio/jackaudio.c | 50 +++++++++++++++++++++++++++++++++++------------
 1 file changed, 37 insertions(+), 13 deletions(-)

-- 
2.20.1

[PATCH v9 1/1] audio/jack: fix use after free segfault

[Geoffrey McRae]

This change registers a bottom handler to close the JACK client
connection when a server shutdown signal is recieved. Without this
libjack2 attempts to "clean up" old clients and causes a use after free
segfault.

Signed-off-by: Geoffrey McRae <geoff@hostfission.com>
---
 audio/jackaudio.c | 50 +++++++++++++++++++++++++++++++++++------------
 1 file changed, 37 insertions(+), 13 deletions(-)

diff --git a/audio/jackaudio.c b/audio/jackaudio.c
index 1e714b30bc..e00e19061a 100644
--- a/audio/jackaudio.c
+++ b/audio/jackaudio.c
@@ -25,6 +25,7 @@
 #include "qemu/osdep.h"
 #include "qemu/module.h"
 #include "qemu/atomic.h"
+#include "qemu/main-loop.h"
 #include "qemu-common.h"
 #include "audio.h"
 
@@ -63,6 +64,7 @@ typedef struct QJackClient {
     QJackState      state;
     jack_client_t  *client;
     jack_nframes_t  freq;
+    QEMUBH         *shutdown_bh;
 
     struct QJack   *j;
     int             nchannels;
@@ -87,6 +89,7 @@ QJackIn;
 static int qjack_client_init(QJackClient *c);
 static void qjack_client_connect_ports(QJackClient *c);
 static void qjack_client_fini(QJackClient *c);
+QemuMutex qjack_shutdown_lock;
 
 static void qjack_buffer_create(QJackBuffer *buffer, int channels, int frames)
 {
@@ -306,21 +309,27 @@ static int qjack_xrun(void *arg)
     return 0;
 }
 
+static void qjack_shutdown_bh(void *opaque)
+{
+    QJackClient *c = (QJackClient *)opaque;
+    qjack_client_fini(c);
+}
+
 static void qjack_shutdown(void *arg)
 {
     QJackClient *c = (QJackClient *)arg;
     c->state = QJACK_STATE_SHUTDOWN;
+    qemu_bh_schedule(c->shutdown_bh);
 }
 
 static void qjack_client_recover(QJackClient *c)
 {
-    if (c->state == QJACK_STATE_SHUTDOWN) {
-        qjack_client_fini(c);
+    if (c->state != QJACK_STATE_DISCONNECTED) {
+        return;
     }
 
     /* packets is used simply to throttle this */
-    if (c->state == QJACK_STATE_DISCONNECTED &&
-        c->packets % 100 == 0) {
+    if (c->packets % 100 == 0) {
 
         /* if enabled then attempt to recover */
         if (c->enabled) {
@@ -489,15 +498,16 @@ static int qjack_init_out(HWVoiceOut *hw, struct audsettings *as,
     QJackOut *jo  = (QJackOut *)hw;
     Audiodev *dev = (Audiodev *)drv_opaque;
 
-    qjack_client_fini(&jo->c);
-
     jo->c.out       = true;
     jo->c.enabled   = false;
     jo->c.nchannels = as->nchannels;
     jo->c.opt       = dev->u.jack.out;
 
+    jo->c.shutdown_bh = qemu_bh_new(qjack_shutdown_bh, &jo->c);
+
     int ret = qjack_client_init(&jo->c);
     if (ret != 0) {
+        qemu_bh_delete(jo->c.shutdown_bh);
         return ret;
     }
 
@@ -525,15 +535,16 @@ static int qjack_init_in(HWVoiceIn *hw, struct audsettings *as,
     QJackIn  *ji  = (QJackIn *)hw;
     Audiodev *dev = (Audiodev *)drv_opaque;
 
-    qjack_client_fini(&ji->c);
-
     ji->c.out       = false;
     ji->c.enabled   = false;
     ji->c.nchannels = as->nchannels;
     ji->c.opt       = dev->u.jack.in;
 
+    ji->c.shutdown_bh = qemu_bh_new(qjack_shutdown_bh, &ji->c);
+
     int ret = qjack_client_init(&ji->c);
     if (ret != 0) {
+        qemu_bh_delete(ji->c.shutdown_bh);
         return ret;
     }
 
@@ -555,7 +566,7 @@ static int qjack_init_in(HWVoiceIn *hw, struct audsettings *as,
     return 0;
 }
 
-static void qjack_client_fini(QJackClient *c)
+static void qjack_client_fini_locked(QJackClient *c)
 {
     switch (c->state) {
     case QJACK_STATE_RUNNING:
@@ -564,28 +575,40 @@ static void qjack_client_fini(QJackClient *c)
 
     case QJACK_STATE_SHUTDOWN:
         jack_client_close(c->client);
+        c->client = NULL;
+
+        qjack_buffer_free(&c->fifo);
+        g_free(c->port);
+
+        c->state = QJACK_STATE_DISCONNECTED;
         /* fallthrough */
 
     case QJACK_STATE_DISCONNECTED:
         break;
     }
+}
 
-    qjack_buffer_free(&c->fifo);
-    g_free(c->port);
-
-    c->state = QJACK_STATE_DISCONNECTED;
+static void qjack_client_fini(QJackClient *c)
+{
+    qemu_mutex_lock(&qjack_shutdown_lock);
+    qjack_client_fini_locked(c);
+    qemu_mutex_unlock(&qjack_shutdown_lock);
 }
 
 static void qjack_fini_out(HWVoiceOut *hw)
 {
     QJackOut *jo = (QJackOut *)hw;
     qjack_client_fini(&jo->c);
+
+    qemu_bh_delete(jo->c.shutdown_bh);
 }
 
 static void qjack_fini_in(HWVoiceIn *hw)
 {
     QJackIn *ji = (QJackIn *)hw;
     qjack_client_fini(&ji->c);
+
+    qemu_bh_delete(ji->c.shutdown_bh);
 }
 
 static void qjack_enable_out(HWVoiceOut *hw, bool enable)
@@ -662,6 +685,7 @@ static void qjack_info(const char *msg)
 
 static void register_audio_jack(void)
 {
+    qemu_mutex_init(&qjack_shutdown_lock);
     audio_driver_register(&jack_driver);
     jack_set_thread_creator(qjack_thread_creator);
     jack_set_error_function(qjack_error);
-- 
2.20.1

Re: [PATCH v9 1/1] audio/jack: fix use after free segfault

[Christian Schoenebeck]

On Samstag, 7. November 2020 01:04:58 CET Geoffrey McRae wrote:
> This change registers a bottom handler to close the JACK client
> connection when a server shutdown signal is recieved. Without this
> libjack2 attempts to "clean up" old clients and causes a use after free
> segfault.
> 
> Signed-off-by: Geoffrey McRae <geoff@hostfission.com>
> ---
>  audio/jackaudio.c | 50 +++++++++++++++++++++++++++++++++++------------
>  1 file changed, 37 insertions(+), 13 deletions(-)
> 
> diff --git a/audio/jackaudio.c b/audio/jackaudio.c
> index 1e714b30bc..e00e19061a 100644
> --- a/audio/jackaudio.c
> +++ b/audio/jackaudio.c
> @@ -25,6 +25,7 @@
>  #include "qemu/osdep.h"
>  #include "qemu/module.h"
>  #include "qemu/atomic.h"
> +#include "qemu/main-loop.h"
>  #include "qemu-common.h"
>  #include "audio.h"
> 
> @@ -63,6 +64,7 @@ typedef struct QJackClient {
>      QJackState      state;
>      jack_client_t  *client;
>      jack_nframes_t  freq;
> +    QEMUBH         *shutdown_bh;
> 
>      struct QJack   *j;
>      int             nchannels;
> @@ -87,6 +89,7 @@ QJackIn;
>  static int qjack_client_init(QJackClient *c);
>  static void qjack_client_connect_ports(QJackClient *c);
>  static void qjack_client_fini(QJackClient *c);
> +QemuMutex qjack_shutdown_lock;
> 

I think this should be:

static QemuMutex qjack_shutdown_lock;

as this mutex is only accessed from within this unit. Except of that:

Reviewed-by: Christian Schoenebeck <qemu_oss@crudebyte.com>

BTW it is common practice to add local functions for initializing, destroying, 
locking and unlocking a specific mutex use case to avoid issues when code 
evolves. But so far the use of this mutex is trivial, so it is Ok for now from 
my PoV.

>  static void qjack_buffer_create(QJackBuffer *buffer, int channels, int
> frames) {
> @@ -306,21 +309,27 @@ static int qjack_xrun(void *arg)
>      return 0;
>  }
> 
> +static void qjack_shutdown_bh(void *opaque)
> +{
> +    QJackClient *c = (QJackClient *)opaque;
> +    qjack_client_fini(c);
> +}
> +
>  static void qjack_shutdown(void *arg)
>  {
>      QJackClient *c = (QJackClient *)arg;
>      c->state = QJACK_STATE_SHUTDOWN;
> +    qemu_bh_schedule(c->shutdown_bh);
>  }
> 
>  static void qjack_client_recover(QJackClient *c)
>  {
> -    if (c->state == QJACK_STATE_SHUTDOWN) {
> -        qjack_client_fini(c);
> +    if (c->state != QJACK_STATE_DISCONNECTED) {
> +        return;
>      }
> 
>      /* packets is used simply to throttle this */
> -    if (c->state == QJACK_STATE_DISCONNECTED &&
> -        c->packets % 100 == 0) {
> +    if (c->packets % 100 == 0) {
> 
>          /* if enabled then attempt to recover */
>          if (c->enabled) {
> @@ -489,15 +498,16 @@ static int qjack_init_out(HWVoiceOut *hw, struct
> audsettings *as, QJackOut *jo  = (QJackOut *)hw;
>      Audiodev *dev = (Audiodev *)drv_opaque;
> 
> -    qjack_client_fini(&jo->c);
> -
>      jo->c.out       = true;
>      jo->c.enabled   = false;
>      jo->c.nchannels = as->nchannels;
>      jo->c.opt       = dev->u.jack.out;
> 
> +    jo->c.shutdown_bh = qemu_bh_new(qjack_shutdown_bh, &jo->c);
> +
>      int ret = qjack_client_init(&jo->c);
>      if (ret != 0) {
> +        qemu_bh_delete(jo->c.shutdown_bh);
>          return ret;
>      }
> 
> @@ -525,15 +535,16 @@ static int qjack_init_in(HWVoiceIn *hw, struct
> audsettings *as, QJackIn  *ji  = (QJackIn *)hw;
>      Audiodev *dev = (Audiodev *)drv_opaque;
> 
> -    qjack_client_fini(&ji->c);
> -
>      ji->c.out       = false;
>      ji->c.enabled   = false;
>      ji->c.nchannels = as->nchannels;
>      ji->c.opt       = dev->u.jack.in;
> 
> +    ji->c.shutdown_bh = qemu_bh_new(qjack_shutdown_bh, &ji->c);
> +
>      int ret = qjack_client_init(&ji->c);
>      if (ret != 0) {
> +        qemu_bh_delete(ji->c.shutdown_bh);
>          return ret;
>      }
> 
> @@ -555,7 +566,7 @@ static int qjack_init_in(HWVoiceIn *hw, struct
> audsettings *as, return 0;
>  }
> 
> -static void qjack_client_fini(QJackClient *c)
> +static void qjack_client_fini_locked(QJackClient *c)
>  {
>      switch (c->state) {
>      case QJACK_STATE_RUNNING:
> @@ -564,28 +575,40 @@ static void qjack_client_fini(QJackClient *c)
> 
>      case QJACK_STATE_SHUTDOWN:
>          jack_client_close(c->client);
> +        c->client = NULL;
> +
> +        qjack_buffer_free(&c->fifo);
> +        g_free(c->port);
> +
> +        c->state = QJACK_STATE_DISCONNECTED;
>          /* fallthrough */
> 
>      case QJACK_STATE_DISCONNECTED:
>          break;
>      }
> +}
> 
> -    qjack_buffer_free(&c->fifo);
> -    g_free(c->port);
> -
> -    c->state = QJACK_STATE_DISCONNECTED;
> +static void qjack_client_fini(QJackClient *c)
> +{
> +    qemu_mutex_lock(&qjack_shutdown_lock);
> +    qjack_client_fini_locked(c);
> +    qemu_mutex_unlock(&qjack_shutdown_lock);
>  }
> 
>  static void qjack_fini_out(HWVoiceOut *hw)
>  {
>      QJackOut *jo = (QJackOut *)hw;
>      qjack_client_fini(&jo->c);
> +
> +    qemu_bh_delete(jo->c.shutdown_bh);
>  }
> 
>  static void qjack_fini_in(HWVoiceIn *hw)
>  {
>      QJackIn *ji = (QJackIn *)hw;
>      qjack_client_fini(&ji->c);
> +
> +    qemu_bh_delete(ji->c.shutdown_bh);
>  }
> 
>  static void qjack_enable_out(HWVoiceOut *hw, bool enable)
> @@ -662,6 +685,7 @@ static void qjack_info(const char *msg)
> 
>  static void register_audio_jack(void)
>  {
> +    qemu_mutex_init(&qjack_shutdown_lock);
>      audio_driver_register(&jack_driver);
>      jack_set_thread_creator(qjack_thread_creator);
>      jack_set_error_function(qjack_error);

Best regards,
Christian Schoenebeck

Re: [PATCH v9 1/1] audio/jack: fix use after free segfault

[Christian Schoenebeck]

On Samstag, 7. November 2020 10:58:10 CET Christian Schoenebeck wrote:
> On Samstag, 7. November 2020 01:04:58 CET Geoffrey McRae wrote:
> > This change registers a bottom handler to close the JACK client
> > connection when a server shutdown signal is recieved. Without this

One last minor thing, typo here: "received".

> > libjack2 attempts to "clean up" old clients and causes a use after free
> > segfault.
> > 
> > Signed-off-by: Geoffrey McRae <geoff@hostfission.com>
> > ---
> > 
> >  audio/jackaudio.c | 50 +++++++++++++++++++++++++++++++++++------------
> >  1 file changed, 37 insertions(+), 13 deletions(-)
> > 
> > diff --git a/audio/jackaudio.c b/audio/jackaudio.c
> > index 1e714b30bc..e00e19061a 100644
> > --- a/audio/jackaudio.c
> > +++ b/audio/jackaudio.c
> > @@ -25,6 +25,7 @@
> > 
> >  #include "qemu/osdep.h"
> >  #include "qemu/module.h"
> >  #include "qemu/atomic.h"
> > 
> > +#include "qemu/main-loop.h"
> > 
> >  #include "qemu-common.h"
> >  #include "audio.h"
> > 
> > @@ -63,6 +64,7 @@ typedef struct QJackClient {
> > 
> >      QJackState      state;
> >      jack_client_t  *client;
> >      jack_nframes_t  freq;
> > 
> > +    QEMUBH         *shutdown_bh;
> > 
> >      struct QJack   *j;
> >      int             nchannels;
> > 
> > @@ -87,6 +89,7 @@ QJackIn;
> > 
> >  static int qjack_client_init(QJackClient *c);
> >  static void qjack_client_connect_ports(QJackClient *c);
> >  static void qjack_client_fini(QJackClient *c);
> > 
> > +QemuMutex qjack_shutdown_lock;
> 
> I think this should be:
> 
> static QemuMutex qjack_shutdown_lock;
> 
> as this mutex is only accessed from within this unit. Except of that:
> 
> Reviewed-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> 
> BTW it is common practice to add local functions for initializing,
> destroying, locking and unlocking a specific mutex use case to avoid issues
> when code evolves. But so far the use of this mutex is trivial, so it is Ok
> for now from my PoV.
> 
> >  static void qjack_buffer_create(QJackBuffer *buffer, int channels, int
> > 
> > frames) {
> > @@ -306,21 +309,27 @@ static int qjack_xrun(void *arg)
> > 
> >      return 0;
> >  
> >  }
> > 
> > +static void qjack_shutdown_bh(void *opaque)
> > +{
> > +    QJackClient *c = (QJackClient *)opaque;
> > +    qjack_client_fini(c);
> > +}
> > +
> > 
> >  static void qjack_shutdown(void *arg)
> >  {
> >  
> >      QJackClient *c = (QJackClient *)arg;
> >      c->state = QJACK_STATE_SHUTDOWN;
> > 
> > +    qemu_bh_schedule(c->shutdown_bh);
> > 
> >  }
> >  
> >  static void qjack_client_recover(QJackClient *c)
> >  {
> > 
> > -    if (c->state == QJACK_STATE_SHUTDOWN) {
> > -        qjack_client_fini(c);
> > +    if (c->state != QJACK_STATE_DISCONNECTED) {
> > +        return;
> > 
> >      }
> >      
> >      /* packets is used simply to throttle this */
> > 
> > -    if (c->state == QJACK_STATE_DISCONNECTED &&
> > -        c->packets % 100 == 0) {
> > +    if (c->packets % 100 == 0) {
> > 
> >          /* if enabled then attempt to recover */
> >          if (c->enabled) {
> > 
> > @@ -489,15 +498,16 @@ static int qjack_init_out(HWVoiceOut *hw, struct
> > audsettings *as, QJackOut *jo  = (QJackOut *)hw;
> > 
> >      Audiodev *dev = (Audiodev *)drv_opaque;
> > 
> > -    qjack_client_fini(&jo->c);
> > -
> > 
> >      jo->c.out       = true;
> >      jo->c.enabled   = false;
> >      jo->c.nchannels = as->nchannels;
> >      jo->c.opt       = dev->u.jack.out;
> > 
> > +    jo->c.shutdown_bh = qemu_bh_new(qjack_shutdown_bh, &jo->c);
> > +
> > 
> >      int ret = qjack_client_init(&jo->c);
> >      if (ret != 0) {
> > 
> > +        qemu_bh_delete(jo->c.shutdown_bh);
> > 
> >          return ret;
> >      
> >      }
> > 
> > @@ -525,15 +535,16 @@ static int qjack_init_in(HWVoiceIn *hw, struct
> > audsettings *as, QJackIn  *ji  = (QJackIn *)hw;
> > 
> >      Audiodev *dev = (Audiodev *)drv_opaque;
> > 
> > -    qjack_client_fini(&ji->c);
> > -
> > 
> >      ji->c.out       = false;
> >      ji->c.enabled   = false;
> >      ji->c.nchannels = as->nchannels;
> >      ji->c.opt       = dev->u.jack.in;
> > 
> > +    ji->c.shutdown_bh = qemu_bh_new(qjack_shutdown_bh, &ji->c);
> > +
> > 
> >      int ret = qjack_client_init(&ji->c);
> >      if (ret != 0) {
> > 
> > +        qemu_bh_delete(ji->c.shutdown_bh);
> > 
> >          return ret;
> >      
> >      }
> > 
> > @@ -555,7 +566,7 @@ static int qjack_init_in(HWVoiceIn *hw, struct
> > audsettings *as, return 0;
> > 
> >  }
> > 
> > -static void qjack_client_fini(QJackClient *c)
> > +static void qjack_client_fini_locked(QJackClient *c)
> > 
> >  {
> >  
> >      switch (c->state) {
> > 
> >      case QJACK_STATE_RUNNING:
> > @@ -564,28 +575,40 @@ static void qjack_client_fini(QJackClient *c)
> > 
> >      case QJACK_STATE_SHUTDOWN:
> >          jack_client_close(c->client);
> > 
> > +        c->client = NULL;
> > +
> > +        qjack_buffer_free(&c->fifo);
> > +        g_free(c->port);
> > +
> > +        c->state = QJACK_STATE_DISCONNECTED;
> > 
> >          /* fallthrough */
> >      
> >      case QJACK_STATE_DISCONNECTED:
> >          break;
> >      
> >      }
> > 
> > +}
> > 
> > -    qjack_buffer_free(&c->fifo);
> > -    g_free(c->port);
> > -
> > -    c->state = QJACK_STATE_DISCONNECTED;
> > +static void qjack_client_fini(QJackClient *c)
> > +{
> > +    qemu_mutex_lock(&qjack_shutdown_lock);
> > +    qjack_client_fini_locked(c);
> > +    qemu_mutex_unlock(&qjack_shutdown_lock);
> > 
> >  }
> >  
> >  static void qjack_fini_out(HWVoiceOut *hw)
> >  {
> >  
> >      QJackOut *jo = (QJackOut *)hw;
> >      qjack_client_fini(&jo->c);
> > 
> > +
> > +    qemu_bh_delete(jo->c.shutdown_bh);
> > 
> >  }
> >  
> >  static void qjack_fini_in(HWVoiceIn *hw)
> >  {
> >  
> >      QJackIn *ji = (QJackIn *)hw;
> >      qjack_client_fini(&ji->c);
> > 
> > +
> > +    qemu_bh_delete(ji->c.shutdown_bh);
> > 
> >  }
> >  
> >  static void qjack_enable_out(HWVoiceOut *hw, bool enable)
> > 
> > @@ -662,6 +685,7 @@ static void qjack_info(const char *msg)
> > 
> >  static void register_audio_jack(void)
> >  {
> > 
> > +    qemu_mutex_init(&qjack_shutdown_lock);
> > 
> >      audio_driver_register(&jack_driver);
> >      jack_set_thread_creator(qjack_thread_creator);
> >      jack_set_error_function(qjack_error);
> 
> Best regards,
> Christian Schoenebeck

[PATCH v10 0/1] audio/jack: fix use after free segfault

[Geoffrey McRae]

v10:
  * fixed typo in commit message
  * qjack_shutdown_lock is now static

Geoffrey McRae (1):
  audio/jack: fix use after free segfault

 audio/jackaudio.c | 50 +++++++++++++++++++++++++++++++++++------------
 1 file changed, 37 insertions(+), 13 deletions(-)

-- 
2.20.1

[PATCH v10 1/1] audio/jack: fix use after free segfault

[Geoffrey McRae]

This change registers a bottom handler to close the JACK client
connection when a server shutdown signal is received. Without this
libjack2 attempts to "clean up" old clients and causes a use after free
segfault.

Signed-off-by: Geoffrey McRae <geoff@hostfission.com>
---
 audio/jackaudio.c | 50 +++++++++++++++++++++++++++++++++++------------
 1 file changed, 37 insertions(+), 13 deletions(-)

diff --git a/audio/jackaudio.c b/audio/jackaudio.c
index 1e714b30bc..3b7c18443d 100644
--- a/audio/jackaudio.c
+++ b/audio/jackaudio.c
@@ -25,6 +25,7 @@
 #include "qemu/osdep.h"
 #include "qemu/module.h"
 #include "qemu/atomic.h"
+#include "qemu/main-loop.h"
 #include "qemu-common.h"
 #include "audio.h"
 
@@ -63,6 +64,7 @@ typedef struct QJackClient {
     QJackState      state;
     jack_client_t  *client;
     jack_nframes_t  freq;
+    QEMUBH         *shutdown_bh;
 
     struct QJack   *j;
     int             nchannels;
@@ -87,6 +89,7 @@ QJackIn;
 static int qjack_client_init(QJackClient *c);
 static void qjack_client_connect_ports(QJackClient *c);
 static void qjack_client_fini(QJackClient *c);
+static QemuMutex qjack_shutdown_lock;
 
 static void qjack_buffer_create(QJackBuffer *buffer, int channels, int frames)
 {
@@ -306,21 +309,27 @@ static int qjack_xrun(void *arg)
     return 0;
 }
 
+static void qjack_shutdown_bh(void *opaque)
+{
+    QJackClient *c = (QJackClient *)opaque;
+    qjack_client_fini(c);
+}
+
 static void qjack_shutdown(void *arg)
 {
     QJackClient *c = (QJackClient *)arg;
     c->state = QJACK_STATE_SHUTDOWN;
+    qemu_bh_schedule(c->shutdown_bh);
 }
 
 static void qjack_client_recover(QJackClient *c)
 {
-    if (c->state == QJACK_STATE_SHUTDOWN) {
-        qjack_client_fini(c);
+    if (c->state != QJACK_STATE_DISCONNECTED) {
+        return;
     }
 
     /* packets is used simply to throttle this */
-    if (c->state == QJACK_STATE_DISCONNECTED &&
-        c->packets % 100 == 0) {
+    if (c->packets % 100 == 0) {
 
         /* if enabled then attempt to recover */
         if (c->enabled) {
@@ -489,15 +498,16 @@ static int qjack_init_out(HWVoiceOut *hw, struct audsettings *as,
     QJackOut *jo  = (QJackOut *)hw;
     Audiodev *dev = (Audiodev *)drv_opaque;
 
-    qjack_client_fini(&jo->c);
-
     jo->c.out       = true;
     jo->c.enabled   = false;
     jo->c.nchannels = as->nchannels;
     jo->c.opt       = dev->u.jack.out;
 
+    jo->c.shutdown_bh = qemu_bh_new(qjack_shutdown_bh, &jo->c);
+
     int ret = qjack_client_init(&jo->c);
     if (ret != 0) {
+        qemu_bh_delete(jo->c.shutdown_bh);
         return ret;
     }
 
@@ -525,15 +535,16 @@ static int qjack_init_in(HWVoiceIn *hw, struct audsettings *as,
     QJackIn  *ji  = (QJackIn *)hw;
     Audiodev *dev = (Audiodev *)drv_opaque;
 
-    qjack_client_fini(&ji->c);
-
     ji->c.out       = false;
     ji->c.enabled   = false;
     ji->c.nchannels = as->nchannels;
     ji->c.opt       = dev->u.jack.in;
 
+    ji->c.shutdown_bh = qemu_bh_new(qjack_shutdown_bh, &ji->c);
+
     int ret = qjack_client_init(&ji->c);
     if (ret != 0) {
+        qemu_bh_delete(ji->c.shutdown_bh);
         return ret;
     }
 
@@ -555,7 +566,7 @@ static int qjack_init_in(HWVoiceIn *hw, struct audsettings *as,
     return 0;
 }
 
-static void qjack_client_fini(QJackClient *c)
+static void qjack_client_fini_locked(QJackClient *c)
 {
     switch (c->state) {
     case QJACK_STATE_RUNNING:
@@ -564,28 +575,40 @@ static void qjack_client_fini(QJackClient *c)
 
     case QJACK_STATE_SHUTDOWN:
         jack_client_close(c->client);
+        c->client = NULL;
+
+        qjack_buffer_free(&c->fifo);
+        g_free(c->port);
+
+        c->state = QJACK_STATE_DISCONNECTED;
         /* fallthrough */
 
     case QJACK_STATE_DISCONNECTED:
         break;
     }
+}
 
-    qjack_buffer_free(&c->fifo);
-    g_free(c->port);
-
-    c->state = QJACK_STATE_DISCONNECTED;
+static void qjack_client_fini(QJackClient *c)
+{
+    qemu_mutex_lock(&qjack_shutdown_lock);
+    qjack_client_fini_locked(c);
+    qemu_mutex_unlock(&qjack_shutdown_lock);
 }
 
 static void qjack_fini_out(HWVoiceOut *hw)
 {
     QJackOut *jo = (QJackOut *)hw;
     qjack_client_fini(&jo->c);
+
+    qemu_bh_delete(jo->c.shutdown_bh);
 }
 
 static void qjack_fini_in(HWVoiceIn *hw)
 {
     QJackIn *ji = (QJackIn *)hw;
     qjack_client_fini(&ji->c);
+
+    qemu_bh_delete(ji->c.shutdown_bh);
 }
 
 static void qjack_enable_out(HWVoiceOut *hw, bool enable)
@@ -662,6 +685,7 @@ static void qjack_info(const char *msg)
 
 static void register_audio_jack(void)
 {
+    qemu_mutex_init(&qjack_shutdown_lock);
     audio_driver_register(&jack_driver);
     jack_set_thread_creator(qjack_thread_creator);
     jack_set_error_function(qjack_error);
-- 
2.20.1

Re: [PATCH v10 1/1] audio/jack: fix use after free segfault

[Christian Schoenebeck]

On Sonntag, 8. November 2020 07:33:50 CET Geoffrey McRae wrote:
> This change registers a bottom handler to close the JACK client
> connection when a server shutdown signal is received. Without this
> libjack2 attempts to "clean up" old clients and causes a use after free
> segfault.
> 
> Signed-off-by: Geoffrey McRae <geoff@hostfission.com>

Reviewed-by: Christian Schoenebeck <qemu_oss@crudebyte.com>

Gerd, I think it would be Ok to queue this as fix for 5.2.

> ---
>  audio/jackaudio.c | 50 +++++++++++++++++++++++++++++++++++------------
>  1 file changed, 37 insertions(+), 13 deletions(-)
> 
> diff --git a/audio/jackaudio.c b/audio/jackaudio.c
> index 1e714b30bc..3b7c18443d 100644
> --- a/audio/jackaudio.c
> +++ b/audio/jackaudio.c
> @@ -25,6 +25,7 @@
>  #include "qemu/osdep.h"
>  #include "qemu/module.h"
>  #include "qemu/atomic.h"
> +#include "qemu/main-loop.h"
>  #include "qemu-common.h"
>  #include "audio.h"
> 
> @@ -63,6 +64,7 @@ typedef struct QJackClient {
>      QJackState      state;
>      jack_client_t  *client;
>      jack_nframes_t  freq;
> +    QEMUBH         *shutdown_bh;
> 
>      struct QJack   *j;
>      int             nchannels;
> @@ -87,6 +89,7 @@ QJackIn;
>  static int qjack_client_init(QJackClient *c);
>  static void qjack_client_connect_ports(QJackClient *c);
>  static void qjack_client_fini(QJackClient *c);
> +static QemuMutex qjack_shutdown_lock;
> 
>  static void qjack_buffer_create(QJackBuffer *buffer, int channels, int
> frames) {
> @@ -306,21 +309,27 @@ static int qjack_xrun(void *arg)
>      return 0;
>  }
> 
> +static void qjack_shutdown_bh(void *opaque)
> +{
> +    QJackClient *c = (QJackClient *)opaque;
> +    qjack_client_fini(c);
> +}
> +
>  static void qjack_shutdown(void *arg)
>  {
>      QJackClient *c = (QJackClient *)arg;
>      c->state = QJACK_STATE_SHUTDOWN;
> +    qemu_bh_schedule(c->shutdown_bh);
>  }
> 
>  static void qjack_client_recover(QJackClient *c)
>  {
> -    if (c->state == QJACK_STATE_SHUTDOWN) {
> -        qjack_client_fini(c);
> +    if (c->state != QJACK_STATE_DISCONNECTED) {
> +        return;
>      }
> 
>      /* packets is used simply to throttle this */
> -    if (c->state == QJACK_STATE_DISCONNECTED &&
> -        c->packets % 100 == 0) {
> +    if (c->packets % 100 == 0) {
> 
>          /* if enabled then attempt to recover */
>          if (c->enabled) {
> @@ -489,15 +498,16 @@ static int qjack_init_out(HWVoiceOut *hw, struct
> audsettings *as, QJackOut *jo  = (QJackOut *)hw;
>      Audiodev *dev = (Audiodev *)drv_opaque;
> 
> -    qjack_client_fini(&jo->c);
> -
>      jo->c.out       = true;
>      jo->c.enabled   = false;
>      jo->c.nchannels = as->nchannels;
>      jo->c.opt       = dev->u.jack.out;
> 
> +    jo->c.shutdown_bh = qemu_bh_new(qjack_shutdown_bh, &jo->c);
> +
>      int ret = qjack_client_init(&jo->c);
>      if (ret != 0) {
> +        qemu_bh_delete(jo->c.shutdown_bh);
>          return ret;
>      }
> 
> @@ -525,15 +535,16 @@ static int qjack_init_in(HWVoiceIn *hw, struct
> audsettings *as, QJackIn  *ji  = (QJackIn *)hw;
>      Audiodev *dev = (Audiodev *)drv_opaque;
> 
> -    qjack_client_fini(&ji->c);
> -
>      ji->c.out       = false;
>      ji->c.enabled   = false;
>      ji->c.nchannels = as->nchannels;
>      ji->c.opt       = dev->u.jack.in;
> 
> +    ji->c.shutdown_bh = qemu_bh_new(qjack_shutdown_bh, &ji->c);
> +
>      int ret = qjack_client_init(&ji->c);
>      if (ret != 0) {
> +        qemu_bh_delete(ji->c.shutdown_bh);
>          return ret;
>      }
> 
> @@ -555,7 +566,7 @@ static int qjack_init_in(HWVoiceIn *hw, struct
> audsettings *as, return 0;
>  }
> 
> -static void qjack_client_fini(QJackClient *c)
> +static void qjack_client_fini_locked(QJackClient *c)
>  {
>      switch (c->state) {
>      case QJACK_STATE_RUNNING:
> @@ -564,28 +575,40 @@ static void qjack_client_fini(QJackClient *c)
> 
>      case QJACK_STATE_SHUTDOWN:
>          jack_client_close(c->client);
> +        c->client = NULL;
> +
> +        qjack_buffer_free(&c->fifo);
> +        g_free(c->port);
> +
> +        c->state = QJACK_STATE_DISCONNECTED;
>          /* fallthrough */
> 
>      case QJACK_STATE_DISCONNECTED:
>          break;
>      }
> +}
> 
> -    qjack_buffer_free(&c->fifo);
> -    g_free(c->port);
> -
> -    c->state = QJACK_STATE_DISCONNECTED;
> +static void qjack_client_fini(QJackClient *c)
> +{
> +    qemu_mutex_lock(&qjack_shutdown_lock);
> +    qjack_client_fini_locked(c);
> +    qemu_mutex_unlock(&qjack_shutdown_lock);
>  }
> 
>  static void qjack_fini_out(HWVoiceOut *hw)
>  {
>      QJackOut *jo = (QJackOut *)hw;
>      qjack_client_fini(&jo->c);
> +
> +    qemu_bh_delete(jo->c.shutdown_bh);
>  }
> 
>  static void qjack_fini_in(HWVoiceIn *hw)
>  {
>      QJackIn *ji = (QJackIn *)hw;
>      qjack_client_fini(&ji->c);
> +
> +    qemu_bh_delete(ji->c.shutdown_bh);
>  }
> 
>  static void qjack_enable_out(HWVoiceOut *hw, bool enable)
> @@ -662,6 +685,7 @@ static void qjack_info(const char *msg)
> 
>  static void register_audio_jack(void)
>  {
> +    qemu_mutex_init(&qjack_shutdown_lock);
>      audio_driver_register(&jack_driver);
>      jack_set_thread_creator(qjack_thread_creator);
>      jack_set_error_function(qjack_error);

Best regards,
Christian Schoenebeck