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

[Christian Schoenebeck <qemu_oss@crudebyte.com>]

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