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

[Geoffrey McRae]

On 2020-08-20 01:21, Christian Schoenebeck wrote:
> On Mittwoch, 19. August 2020 08:29:39 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>
> > ---
>
> Looks much better now, but ...
>
> >  audio/jackaudio.c | 29 ++++++++++++++++++++---------
> >  1 file changed, 20 insertions(+), 9 deletions(-)
> >
> > diff --git a/audio/jackaudio.c b/audio/jackaudio.c
> > index 72ed7c4929..b0da5cd00b 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;
> > @@ -306,21 +308,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;
> > +    c->state       = QJACK_STATE_SHUTDOWN;
>
> White space changes are not much embraced on high traffic projects BTW.

This change is unintentional and was missed in the rollback from the 
prior patch version.

> > +    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) {
> > @@ -417,6 +425,10 @@ static int qjack_client_init(QJackClient *c)
> >          options |= JackServerName;
> >      }
> >
> > +    if (!c->shutdown_bh) {
> > +        c->shutdown_bh = qemu_bh_new(qjack_shutdown_bh, c);
> > +    }
> > +
>
> Where is qemu_bh_delete() ?

Whoops... I will correct this :)

> >      c->client = jack_client_open(client_name, options, &status,
> >        c->opt->server_name);
> >
> > @@ -489,8 +501,6 @@ 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;
> > @@ -525,8 +535,6 @@ 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;
> > @@ -557,6 +565,8 @@ static int qjack_init_in(HWVoiceIn *hw, struct
> > audsettings *as,
> >
> >  static void qjack_client_fini(QJackClient *c)
> >  {
> > +    qemu_bh_cancel(c->shutdown_bh);
> > +
>
> Looks like a potential race. Quote from the API doc of 
> qemu_bh_cancel():
>
> 	"While cancellation itself is also wait-free and thread-safe, it can 
> of
>         course race with the loop that executes bottom halves unless you are
> 	holding the iothread mutex.  This makes it mostly useless if you are 
> not
>         holding the mutex."

Noted. How does one go about holding the iothread mutex?

> >      switch (c->state) {
> >      case QJACK_STATE_RUNNING:
> >          jack_deactivate(c->client);
> > @@ -564,6 +574,7 @@ static void qjack_client_fini(QJackClient *c)
> >
> >      case QJACK_STATE_SHUTDOWN:
> >          jack_client_close(c->client);
> > +        c->client = NULL;
> >          /* fallthrough */
> >
> >      case QJACK_STATE_DISCONNECTED:
>
> Best regards,
> Christian Schoenebeck