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 <ge...@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
