* [PATCH v8 0/1] audio/jack: fix use after free segfault @ 2020-08-21 13:45 Geoffrey McRae 2020-08-21 13:45 ` [PATCH v8 1/1] " Geoffrey McRae 0 siblings, 1 reply; 13+ messages in thread From: Geoffrey McRae @ 2020-08-21 13:45 UTC (permalink / raw) To: qemu-devel; +Cc: Geoffrey McRae, kraxel, pbonzini 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v8 1/1] audio/jack: fix use after free segfault 2020-08-21 13:45 [PATCH v8 0/1] audio/jack: fix use after free segfault Geoffrey McRae @ 2020-08-21 13:45 ` Geoffrey McRae 2020-08-21 17:34 ` Christian Schoenebeck 2020-11-07 0:04 ` [PATCH v8 0/1] " Geoffrey McRae 0 siblings, 2 replies; 13+ messages in thread From: Geoffrey McRae @ 2020-08-21 13:45 UTC (permalink / raw) To: qemu-devel; +Cc: Geoffrey McRae, kraxel, pbonzini 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v8 1/1] audio/jack: fix use after free segfault 2020-08-21 13:45 ` [PATCH v8 1/1] " Geoffrey McRae @ 2020-08-21 17:34 ` Christian Schoenebeck 2020-08-21 17:47 ` Paolo Bonzini 2020-11-07 0:04 ` [PATCH v8 0/1] " Geoffrey McRae 1 sibling, 1 reply; 13+ messages in thread From: Christian Schoenebeck @ 2020-08-21 17:34 UTC (permalink / raw) To: qemu-devel; +Cc: Geoffrey McRae, kraxel, pbonzini 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v8 1/1] audio/jack: fix use after free segfault 2020-08-21 17:34 ` Christian Schoenebeck @ 2020-08-21 17:47 ` Paolo Bonzini 2020-08-22 0:16 ` Geoffrey McRae 0 siblings, 1 reply; 13+ messages in thread From: Paolo Bonzini @ 2020-08-21 17:47 UTC (permalink / raw) To: Christian Schoenebeck, qemu-devel; +Cc: Geoffrey McRae, kraxel 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v8 1/1] audio/jack: fix use after free segfault 2020-08-21 17:47 ` Paolo Bonzini @ 2020-08-22 0:16 ` Geoffrey McRae 2020-08-22 6:58 ` Christian Schoenebeck 0 siblings, 1 reply; 13+ messages in thread From: Geoffrey McRae @ 2020-08-22 0:16 UTC (permalink / raw) To: Paolo Bonzini; +Cc: Christian Schoenebeck, qemu-devel, kraxel 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v8 1/1] audio/jack: fix use after free segfault 2020-08-22 0:16 ` Geoffrey McRae @ 2020-08-22 6:58 ` Christian Schoenebeck 0 siblings, 0 replies; 13+ messages in thread From: Christian Schoenebeck @ 2020-08-22 6:58 UTC (permalink / raw) To: qemu-devel; +Cc: Geoffrey McRae, Paolo Bonzini, kraxel 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v8 0/1] audio/jack: fix use after free segfault 2020-08-21 13:45 ` [PATCH v8 1/1] " Geoffrey McRae 2020-08-21 17:34 ` Christian Schoenebeck @ 2020-11-07 0:04 ` Geoffrey McRae 2020-11-07 0:04 ` [PATCH v9 1/1] " Geoffrey McRae 2020-11-08 6:33 ` [PATCH v10 0/1] " Geoffrey McRae 1 sibling, 2 replies; 13+ messages in thread From: Geoffrey McRae @ 2020-11-07 0:04 UTC (permalink / raw) To: qemu-devel; +Cc: Geoffrey McRae, kraxel, pbonzini 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v9 1/1] audio/jack: fix use after free segfault 2020-11-07 0:04 ` [PATCH v8 0/1] " Geoffrey McRae @ 2020-11-07 0:04 ` Geoffrey McRae 2020-11-07 9:58 ` Christian Schoenebeck 2020-11-08 6:33 ` [PATCH v10 0/1] " Geoffrey McRae 1 sibling, 1 reply; 13+ messages in thread From: Geoffrey McRae @ 2020-11-07 0:04 UTC (permalink / raw) To: qemu-devel; +Cc: Geoffrey McRae, kraxel, pbonzini 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v9 1/1] audio/jack: fix use after free segfault 2020-11-07 0:04 ` [PATCH v9 1/1] " Geoffrey McRae @ 2020-11-07 9:58 ` Christian Schoenebeck 2020-11-07 10:09 ` Christian Schoenebeck 0 siblings, 1 reply; 13+ messages in thread From: Christian Schoenebeck @ 2020-11-07 9:58 UTC (permalink / raw) To: qemu-devel; +Cc: Geoffrey McRae, kraxel, pbonzini 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v9 1/1] audio/jack: fix use after free segfault 2020-11-07 9:58 ` Christian Schoenebeck @ 2020-11-07 10:09 ` Christian Schoenebeck 0 siblings, 0 replies; 13+ messages in thread From: Christian Schoenebeck @ 2020-11-07 10:09 UTC (permalink / raw) To: qemu-devel; +Cc: Geoffrey McRae, kraxel, pbonzini 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v10 0/1] audio/jack: fix use after free segfault 2020-11-07 0:04 ` [PATCH v8 0/1] " Geoffrey McRae 2020-11-07 0:04 ` [PATCH v9 1/1] " Geoffrey McRae @ 2020-11-08 6:33 ` Geoffrey McRae 2020-11-08 6:33 ` [PATCH v10 1/1] " Geoffrey McRae 1 sibling, 1 reply; 13+ messages in thread From: Geoffrey McRae @ 2020-11-08 6:33 UTC (permalink / raw) To: qemu-devel; +Cc: Geoffrey McRae, kraxel, pbonzini, qemu_oss 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v10 1/1] audio/jack: fix use after free segfault 2020-11-08 6:33 ` [PATCH v10 0/1] " Geoffrey McRae @ 2020-11-08 6:33 ` Geoffrey McRae 2020-11-08 11:09 ` Christian Schoenebeck 0 siblings, 1 reply; 13+ messages in thread From: Geoffrey McRae @ 2020-11-08 6:33 UTC (permalink / raw) To: qemu-devel; +Cc: Geoffrey McRae, kraxel, pbonzini, qemu_oss 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v10 1/1] audio/jack: fix use after free segfault 2020-11-08 6:33 ` [PATCH v10 1/1] " Geoffrey McRae @ 2020-11-08 11:09 ` Christian Schoenebeck 0 siblings, 0 replies; 13+ messages in thread From: Christian Schoenebeck @ 2020-11-08 11:09 UTC (permalink / raw) To: qemu-devel; +Cc: Geoffrey McRae, kraxel, pbonzini 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2020-11-08 11:11 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-08-21 13:45 [PATCH v8 0/1] audio/jack: fix use after free segfault Geoffrey McRae 2020-08-21 13:45 ` [PATCH v8 1/1] " Geoffrey McRae 2020-08-21 17:34 ` Christian Schoenebeck 2020-08-21 17:47 ` Paolo Bonzini 2020-08-22 0:16 ` Geoffrey McRae 2020-08-22 6:58 ` Christian Schoenebeck 2020-11-07 0:04 ` [PATCH v8 0/1] " Geoffrey McRae 2020-11-07 0:04 ` [PATCH v9 1/1] " Geoffrey McRae 2020-11-07 9:58 ` Christian Schoenebeck 2020-11-07 10:09 ` Christian Schoenebeck 2020-11-08 6:33 ` [PATCH v10 0/1] " Geoffrey McRae 2020-11-08 6:33 ` [PATCH v10 1/1] " Geoffrey McRae 2020-11-08 11:09 ` Christian Schoenebeck
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.