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