* [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.