* [PATCH] audio/jack: fix use after free segfault
@ 2020-08-18 12:40 Geoffrey McRae
2020-08-18 13:41 ` no-reply
2020-08-18 18:11 ` Christian Schoenebeck
0 siblings, 2 replies; 12+ messages in thread
From: Geoffrey McRae @ 2020-08-18 12:40 UTC (permalink / raw)
To: qemu-devel; +Cc: kraxel
Due to a ridiculous commit in the Jack library, the client may have been
freed already by a secondary audio device recovering its session.
https://github.com/jackaudio/jack2/issues/627
Until there is a proper fix for this we can not risk using the pointer
at all if we have been notified of a shutdown as it may have been freed
by the jack library, as such the close call is commented out to prevent
a use after free segfault.
At this time, this will not cause a memory leak as the recovery routine
will trigger the "cleanup" code in the jack library, however, if this is
ever corrected in the jack library this will need to be revisited.
Signed-off-by: Geoffrey McRae <geoff@hostfission.com>
---
audio/jackaudio.c | 17 ++++++++++++++++-
1 file changed, 16 insertions(+), 1 deletion(-)
diff --git a/audio/jackaudio.c b/audio/jackaudio.c
index 72ed7c4929..e8faf1bb89 100644
--- a/audio/jackaudio.c
+++ b/audio/jackaudio.c
@@ -563,7 +563,22 @@ static void qjack_client_fini(QJackClient *c)
/* fallthrough */
case QJACK_STATE_SHUTDOWN:
- jack_client_close(c->client);
+ /*
+ * Due to a rediculous commit in the Jack library, the client may have
+ * been freed already.
+ *
+ * Until there is a proper fix for this we can not risk using the
+ * pointer at all if we have been notified of a shutdown, as such the
+ * below line is commented out to prevent a use after free segfault.
+ * This will not cause a memory leak as the recovery routine will trigger
+ * the "cleanup" code in the jack library.
+ *
+ * https://github.com/jackaudio/jack2/commit/171a3c4a0ddd18d2afae56f3af6291c8e96ee3ac
+ */
+
+ //jack_client_close(c->client);
+ c->client = NULL;
+
/* fallthrough */
case QJACK_STATE_DISCONNECTED:
--
2.20.1
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] audio/jack: fix use after free segfault
2020-08-18 12:40 [PATCH] audio/jack: fix use after free segfault Geoffrey McRae
@ 2020-08-18 13:41 ` no-reply
2020-08-18 18:11 ` Christian Schoenebeck
1 sibling, 0 replies; 12+ messages in thread
From: no-reply @ 2020-08-18 13:41 UTC (permalink / raw)
To: geoff; +Cc: qemu-devel, kraxel
Patchew URL: https://patchew.org/QEMU/20200818131206.BB75F3A0B9F@moya.office.hostfission.com/
Hi,
This series seems to have some coding style problems. See output below for
more information:
Type: series
Message-id: 20200818131206.BB75F3A0B9F@moya.office.hostfission.com
Subject: [PATCH] audio/jack: fix use after free segfault
=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===
Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
8ec400e audio/jack: fix use after free segfault
=== OUTPUT BEGIN ===
WARNING: line over 80 characters
#40: FILE: audio/jackaudio.c:573:
+ * This will not cause a memory leak as the recovery routine will trigger
ERROR: do not use C99 // comments
#46: FILE: audio/jackaudio.c:579:
+ //jack_client_close(c->client);
total: 1 errors, 1 warnings, 23 lines checked
Commit 8ec400ec086a (audio/jack: fix use after free segfault) has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===
Test command exited with code: 1
The full log is available at
http://patchew.org/logs/20200818131206.BB75F3A0B9F@moya.office.hostfission.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] audio/jack: fix use after free segfault
2020-08-18 12:40 [PATCH] audio/jack: fix use after free segfault Geoffrey McRae
2020-08-18 13:41 ` no-reply
@ 2020-08-18 18:11 ` Christian Schoenebeck
2020-08-18 22:20 ` Geoffrey McRae
1 sibling, 1 reply; 12+ messages in thread
From: Christian Schoenebeck @ 2020-08-18 18:11 UTC (permalink / raw)
To: qemu-devel; +Cc: Geoffrey McRae, kraxel
On Dienstag, 18. August 2020 14:40:36 CEST Geoffrey McRae wrote:
> Due to a ridiculous commit in the Jack library, the client may have been
> freed already by a secondary audio device recovering its session.
>
> https://github.com/jackaudio/jack2/issues/627
>
> Until there is a proper fix for this we can not risk using the pointer
> at all if we have been notified of a shutdown as it may have been freed
> by the jack library, as such the close call is commented out to prevent
> a use after free segfault.
>
> At this time, this will not cause a memory leak as the recovery routine
> will trigger the "cleanup" code in the jack library, however, if this is
> ever corrected in the jack library this will need to be revisited.
>
> Signed-off-by: Geoffrey McRae <geoff@hostfission.com>
> ---
> audio/jackaudio.c | 17 ++++++++++++++++-
> 1 file changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/audio/jackaudio.c b/audio/jackaudio.c
> index 72ed7c4929..e8faf1bb89 100644
> --- a/audio/jackaudio.c
> +++ b/audio/jackaudio.c
> @@ -563,7 +563,22 @@ static void qjack_client_fini(QJackClient *c)
> /* fallthrough */
>
> case QJACK_STATE_SHUTDOWN:
> - jack_client_close(c->client);
> + /*
> + * Due to a rediculous commit in the Jack library, the client may
> have + * been freed already.
No need to be offending, and especially no need to insult Stéphane in QEMU
code.
Could you please describe in more detail how you ran into this situation with
your 2nd audio device?
> + *
> + * Until there is a proper fix for this we can not risk using the
> + * pointer at all if we have been notified of a shutdown, as such
> the + * below line is commented out to prevent a use after free
> segfault. + * This will not cause a memory leak as the recovery
> routine will trigger + * the "cleanup" code in the jack library.
> + *
> + *
> https://github.com/jackaudio/jack2/commit/171a3c4a0ddd18d2afae56f3af6291c8e
> 96ee3ac + */
> +
> + //jack_client_close(c->client);
> + c->client = NULL;
> +
> /* fallthrough */
Are you aware that there are two distinct variants of JACK? They are commonly
referred to as JACK1 vs. JACK2 and despite their names, they are in fact
completely separate implementations and there are people who prefer one over
the other. Your change would affect JACK1 as well.
Best regards,
Christian Schoenebeck
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] audio/jack: fix use after free segfault
2020-08-18 18:11 ` Christian Schoenebeck
@ 2020-08-18 22:20 ` Geoffrey McRae
2020-08-19 11:30 ` Christian Schoenebeck
0 siblings, 1 reply; 12+ messages in thread
From: Geoffrey McRae @ 2020-08-18 22:20 UTC (permalink / raw)
To: Christian Schoenebeck; +Cc: qemu-devel, kraxel
On 2020-08-19 04:11, Christian Schoenebeck wrote:
> On Dienstag, 18. August 2020 14:40:36 CEST Geoffrey McRae wrote:
>> Due to a ridiculous commit in the Jack library, the client may have
>> been
>> freed already by a secondary audio device recovering its session.
>>
>> https://github.com/jackaudio/jack2/issues/627
>>
>> Until there is a proper fix for this we can not risk using the pointer
>> at all if we have been notified of a shutdown as it may have been
>> freed
>> by the jack library, as such the close call is commented out to
>> prevent
>> a use after free segfault.
>>
>> At this time, this will not cause a memory leak as the recovery
>> routine
>> will trigger the "cleanup" code in the jack library, however, if this
>> is
>> ever corrected in the jack library this will need to be revisited.
>>
>> Signed-off-by: Geoffrey McRae <geoff@hostfission.com>
>> ---
>> audio/jackaudio.c | 17 ++++++++++++++++-
>> 1 file changed, 16 insertions(+), 1 deletion(-)
>>
>> diff --git a/audio/jackaudio.c b/audio/jackaudio.c
>> index 72ed7c4929..e8faf1bb89 100644
>> --- a/audio/jackaudio.c
>> +++ b/audio/jackaudio.c
>> @@ -563,7 +563,22 @@ static void qjack_client_fini(QJackClient *c)
>> /* fallthrough */
>>
>> case QJACK_STATE_SHUTDOWN:
>> - jack_client_close(c->client);
>> + /*
>> + * Due to a rediculous commit in the Jack library, the client
>> may
>> have + * been freed already.
>
> No need to be offending, and especially no need to insult Stéphane in
> QEMU
> code.
Fair enough, I was not intending to offend Stéphane, and I apologize for
this.
I will revise this patch.
>
> Could you please describe in more detail how you ran into this
> situation with
> your 2nd audio device?
Sure. Run a Windows guest with two audio devices, let it boot up, then
restart
the jack service to trigger the recovery routine, then attempt to use
the 2nd
(non-primary) audio device. Ie, go to windows audio settings to test the
microphone of the second audio device.
When windows try to use the 2nd audio device it goes through the
recovery
routine triggering this fault.
>
>> + *
>> + * Until there is a proper fix for this we can not risk using
>> the
>> + * pointer at all if we have been notified of a shutdown, as
>> such
>> the + * below line is commented out to prevent a use after
>> free
>> segfault. + * This will not cause a memory leak as the
>> recovery
>> routine will trigger + * the "cleanup" code in the jack
>> library.
>> + *
>> + *
>> https://github.com/jackaudio/jack2/commit/171a3c4a0ddd18d2afae56f3af6291c8e
>> 96ee3ac + */
>> +
>> + //jack_client_close(c->client);
>> + c->client = NULL;
>> +
>> /* fallthrough */
>
> Are you aware that there are two distinct variants of JACK? They are
> commonly
> referred to as JACK1 vs. JACK2 and despite their names, they are in
> fact
> completely separate implementations and there are people who prefer one
> over
> the other. Your change would affect JACK1 as well.
I am aware and since these libraries are interchangeable I had assumed
that
JACK1 will have the same fault. If not I suppose we need to detect which
is in
use and change this code appropriately.
>
> Best regards,
> Christian Schoenebeck
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] audio/jack: fix use after free segfault
2020-08-18 22:20 ` Geoffrey McRae
@ 2020-08-19 11:30 ` Christian Schoenebeck
2020-08-19 11:45 ` Geoffrey McRae
0 siblings, 1 reply; 12+ messages in thread
From: Christian Schoenebeck @ 2020-08-19 11:30 UTC (permalink / raw)
To: qemu-devel; +Cc: Geoffrey McRae, kraxel
On Mittwoch, 19. August 2020 00:20:07 CEST Geoffrey McRae wrote:
> > Could you please describe in more detail how you ran into this
> > situation with
> > your 2nd audio device?
>
> Sure. Run a Windows guest with two audio devices, let it boot up, then
> restart
> the jack service to trigger the recovery routine, then attempt to use
> the 2nd
> (non-primary) audio device. Ie, go to windows audio settings to test the
> microphone of the second audio device.
>
> When windows try to use the 2nd audio device it goes through the
> recovery
> routine triggering this fault.
I still don't quite get how this correlates. So you are forcing a restart of
jackd on host side in between, for what purpose? To simulate the Windows
client being kicked by jackd?
What latencies do you achieve BTW with Windows guests?
> I am aware and since these libraries are interchangeable I had assumed
> that
> JACK1 will have the same fault. If not I suppose we need to detect which
> is in
> use and change this code appropriately.
I haven't checked this in the JACK1 code base yet, but I assume JACK1 does not
behave like JACK2 here, because the JACK API is very clear that it is the
client's responsibility to free itself.
So it looks like a JACK2-only-bug to me.
Very weird that there is no jack_client_version() in the shared weak API (i.e.
missing on JACK1 side).
Best regards,
Christian Schoenebeck
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] audio/jack: fix use after free segfault
2020-08-19 11:30 ` Christian Schoenebeck
@ 2020-08-19 11:45 ` Geoffrey McRae
2020-08-19 12:41 ` Christian Schoenebeck
2020-08-19 13:30 ` Gerd Hoffmann
0 siblings, 2 replies; 12+ messages in thread
From: Geoffrey McRae @ 2020-08-19 11:45 UTC (permalink / raw)
To: Christian Schoenebeck; +Cc: qemu-devel, kraxel
On 2020-08-19 21:30, Christian Schoenebeck wrote:
> On Mittwoch, 19. August 2020 00:20:07 CEST Geoffrey McRae wrote:
>> > Could you please describe in more detail how you ran into this
>> > situation with
>> > your 2nd audio device?
>>
>> Sure. Run a Windows guest with two audio devices, let it boot up, then
>> restart
>> the jack service to trigger the recovery routine, then attempt to use
>> the 2nd
>> (non-primary) audio device. Ie, go to windows audio settings to test
>> the
>> microphone of the second audio device.
>>
>> When windows try to use the 2nd audio device it goes through the
>> recovery
>> routine triggering this fault.
>
> I still don't quite get how this correlates. So you are forcing a
> restart of
> jackd on host side in between, for what purpose? To simulate the
> Windows
> client being kicked by jackd?
For many reasons jack may need to be stopped and started again, such as
hardware changes when switching to a USB audio device, or tuning the
period size, etc. QEMU should be able to recover if the jack server goes
away, it's that simple.
The following sequence is what triggers this fault.
client1 = jack_client_open();
client2 = jack_client_open();
client1 gets a shutdown signal
jack_client_close(client1);
client1 = jack_client_open();
client2 gets a shutdown signal
jack_client_close(client2);
client2 = jack_client_open();
One would expect this sequence to work fine as it conforms to the JACK
documentation and common design practice, however, the call to
`jack_client_open` notices that there is the 2nd session and frees it
out from under the application.
This has been resolved in the v5 patch as suggested by Gerd by
scheduling a QEMUBH to perform the closures so they occur in order
before an attempt to open again. Even still this is clearly a design
flaw in the Jack2 library.
>
> What latencies do you achieve BTW with Windows guests?
>
Never tested, it's not the reason why I use jack. Suffice to say it's
far better than PulseAudio, I get no stuttering issues like is commonly
reported for ALSA and PA, and allows for a high degree of
reconfigurability. The guest VM overall performs far better also as
windows is never waiting on the audio device due to the decoupling
provided by the ring buffer in my implementation.
>> I am aware and since these libraries are interchangeable I had assumed
>> that
>> JACK1 will have the same fault. If not I suppose we need to detect
>> which
>> is in
>> use and change this code appropriately.
>
> I haven't checked this in the JACK1 code base yet, but I assume JACK1
> does not
> behave like JACK2 here, because the JACK API is very clear that it is
> the
> client's responsibility to free itself.
>
> So it looks like a JACK2-only-bug to me.
Confirmed, this was investigated today.
>
> Very weird that there is no jack_client_version() in the shared weak
> API (i.e.
> missing on JACK1 side).
I raised this as an issue today:
https://github.com/jackaudio/jack2/issues/628
The developer there seems to feel that allowing the application to know
the jack client version is a bad thing.
>
> Best regards,
> Christian Schoenebeck
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] audio/jack: fix use after free segfault
2020-08-19 11:45 ` Geoffrey McRae
@ 2020-08-19 12:41 ` Christian Schoenebeck
2020-08-19 12:51 ` Geoffrey McRae
2020-08-19 13:30 ` Gerd Hoffmann
1 sibling, 1 reply; 12+ messages in thread
From: Christian Schoenebeck @ 2020-08-19 12:41 UTC (permalink / raw)
To: qemu-devel; +Cc: Geoffrey McRae, kraxel
On Mittwoch, 19. August 2020 13:45:33 CEST Geoffrey McRae wrote:
> > I still don't quite get how this correlates. So you are forcing a
> > restart of
> > jackd on host side in between, for what purpose? To simulate the
> > Windows
> > client being kicked by jackd?
>
> For many reasons jack may need to be stopped and started again, such as
> hardware changes when switching to a USB audio device, or tuning the
> period size, etc. QEMU should be able to recover if the jack server goes
> away, it's that simple.
Most of that could be done without jackd restart, but yeah I got your point.
Thanks!
> The following sequence is what triggers this fault.
>
> client1 = jack_client_open();
> client2 = jack_client_open();
>
> client1 gets a shutdown signal
>
> jack_client_close(client1);
> client1 = jack_client_open();
>
> client2 gets a shutdown signal
>
> jack_client_close(client2);
> client2 = jack_client_open();
>
> One would expect this sequence to work fine as it conforms to the JACK
> documentation and common design practice, however, the call to
> `jack_client_open` notices that there is the 2nd session and frees it
> out from under the application.
>
> This has been resolved in the v5 patch as suggested by Gerd by
> scheduling a QEMUBH to perform the closures so they occur in order
> before an attempt to open again. Even still this is clearly a design
> flaw in the Jack2 library.
Agreed.
I look at your v5 a bit later.
> > What latencies do you achieve BTW with Windows guests?
>
> Never tested, it's not the reason why I use jack.
Surpring that you never checked the min. latency there, as you nailed quite an
ambitous jack driver into QEMU which I just realize now. Must have been
splipped my awareness due to traffic.
> Suffice to say it's far better than PulseAudio,
Of course! :) I was shaking my head when distros started to pick PulseAudio as
standard sound server. I never got that decision as JACK was always way
superior than PA.
Probably the only issue for consumer use cases is JACK's habit to kick
unreliable clients out of the graph. Even though this could be handled of
course.
> I get no stuttering issues like is commonly
> reported for ALSA and PA, and allows for a high degree of
> reconfigurability. The guest VM overall performs far better also as
> windows is never waiting on the audio device due to the decoupling
> provided by the ring buffer in my implementation.
Yeah, looks good indeed!
> >> I am aware and since these libraries are interchangeable I had assumed
> >> that
> >> JACK1 will have the same fault. If not I suppose we need to detect
> >> which
> >> is in
> >> use and change this code appropriately.
> >
> > I haven't checked this in the JACK1 code base yet, but I assume JACK1
> > does not
> > behave like JACK2 here, because the JACK API is very clear that it is
> > the
> > client's responsibility to free itself.
> >
> > So it looks like a JACK2-only-bug to me.
>
> Confirmed, this was investigated today.
>
> > Very weird that there is no jack_client_version() in the shared weak
> > API (i.e.
> > missing on JACK1 side).
>
> I raised this as an issue today:
> https://github.com/jackaudio/jack2/issues/628
> The developer there seems to feel that allowing the application to know
> the jack client version is a bad thing.
Yeah, I raised my voice there as well now.
Best regards,
Christian Schoenebeck
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] audio/jack: fix use after free segfault
2020-08-19 12:41 ` Christian Schoenebeck
@ 2020-08-19 12:51 ` Geoffrey McRae
2020-08-19 15:51 ` Christian Schoenebeck
0 siblings, 1 reply; 12+ messages in thread
From: Geoffrey McRae @ 2020-08-19 12:51 UTC (permalink / raw)
To: Christian Schoenebeck; +Cc: qemu-devel, kraxel
On 2020-08-19 22:41, Christian Schoenebeck wrote:
> On Mittwoch, 19. August 2020 13:45:33 CEST Geoffrey McRae wrote:
>> > I still don't quite get how this correlates. So you are forcing a
>> > restart of
>> > jackd on host side in between, for what purpose? To simulate the
>> > Windows
>> > client being kicked by jackd?
>>
>> For many reasons jack may need to be stopped and started again, such
>> as
>> hardware changes when switching to a USB audio device, or tuning the
>> period size, etc. QEMU should be able to recover if the jack server
>> goes
>> away, it's that simple.
>
> Most of that could be done without jackd restart, but yeah I got your
> point.
> Thanks!
>
>> The following sequence is what triggers this fault.
>>
>> client1 = jack_client_open();
>> client2 = jack_client_open();
>>
>> client1 gets a shutdown signal
>>
>> jack_client_close(client1);
>> client1 = jack_client_open();
>>
>> client2 gets a shutdown signal
>>
>> jack_client_close(client2);
>> client2 = jack_client_open();
>>
>> One would expect this sequence to work fine as it conforms to the JACK
>> documentation and common design practice, however, the call to
>> `jack_client_open` notices that there is the 2nd session and frees it
>> out from under the application.
>>
>> This has been resolved in the v5 patch as suggested by Gerd by
>> scheduling a QEMUBH to perform the closures so they occur in order
>> before an attempt to open again. Even still this is clearly a design
>> flaw in the Jack2 library.
>
> Agreed.
>
> I look at your v5 a bit later.
>
>> > What latencies do you achieve BTW with Windows guests?
>>
>> Never tested, it's not the reason why I use jack.
>
> Surpring that you never checked the min. latency there, as you nailed
> quite an
> ambitous jack driver into QEMU which I just realize now. Must have been
> splipped my awareness due to traffic.
Sorry, I should have been clearer. I have tested windows and the latency
is excellent, but I have never performed any empirical measurements.
>
>> Suffice to say it's far better than PulseAudio,
>
> Of course! :) I was shaking my head when distros started to pick
> PulseAudio as
> standard sound server. I never got that decision as JACK was always way
> superior than PA.
>
> Probably the only issue for consumer use cases is JACK's habit to kick
> unreliable clients out of the graph. Even though this could be handled
> of
> course.
>
>> I get no stuttering issues like is commonly
>> reported for ALSA and PA, and allows for a high degree of
>> reconfigurability. The guest VM overall performs far better also as
>> windows is never waiting on the audio device due to the decoupling
>> provided by the ring buffer in my implementation.
>
> Yeah, looks good indeed!
>
>> >> I am aware and since these libraries are interchangeable I had assumed
>> >> that
>> >> JACK1 will have the same fault. If not I suppose we need to detect
>> >> which
>> >> is in
>> >> use and change this code appropriately.
>> >
>> > I haven't checked this in the JACK1 code base yet, but I assume JACK1
>> > does not
>> > behave like JACK2 here, because the JACK API is very clear that it is
>> > the
>> > client's responsibility to free itself.
>> >
>> > So it looks like a JACK2-only-bug to me.
>>
>> Confirmed, this was investigated today.
>>
>> > Very weird that there is no jack_client_version() in the shared weak
>> > API (i.e.
>> > missing on JACK1 side).
>>
>> I raised this as an issue today:
>> https://github.com/jackaudio/jack2/issues/628
>> The developer there seems to feel that allowing the application to
>> know
>> the jack client version is a bad thing.
>
> Yeah, I raised my voice there as well now.
I noticed, thanks! :D
>
> Best regards,
> Christian Schoenebeck
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] audio/jack: fix use after free segfault
2020-08-19 11:45 ` Geoffrey McRae
2020-08-19 12:41 ` Christian Schoenebeck
@ 2020-08-19 13:30 ` Gerd Hoffmann
1 sibling, 0 replies; 12+ messages in thread
From: Gerd Hoffmann @ 2020-08-19 13:30 UTC (permalink / raw)
To: Geoffrey McRae; +Cc: Christian Schoenebeck, qemu-devel
Hi,
> Even still this is clearly a design flaw in the Jack2 library.
Agreeing here. Bugs in apps should be fixed in apps.
It isn't the libraries business to workaround them.
take care,
Gerd
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] audio/jack: fix use after free segfault
2020-08-19 12:51 ` Geoffrey McRae
@ 2020-08-19 15:51 ` Christian Schoenebeck
2020-08-19 15:57 ` Geoffrey McRae
0 siblings, 1 reply; 12+ messages in thread
From: Christian Schoenebeck @ 2020-08-19 15:51 UTC (permalink / raw)
To: qemu-devel; +Cc: Geoffrey McRae, kraxel
On Mittwoch, 19. August 2020 14:51:52 CEST Geoffrey McRae wrote:
> >> > What latencies do you achieve BTW with Windows guests?
> >>
> >> Never tested, it's not the reason why I use jack.
> >
> > Surpring that you never checked the min. latency there, as you nailed
> > quite an
> > ambitous jack driver into QEMU which I just realize now. Must have been
> > splipped my awareness due to traffic.
>
> Sorry, I should have been clearer. I have tested windows and the latency
> is excellent, but I have never performed any empirical measurements.
/*
* ensure the buffersize is no smaller then 512 samples, some (all?) qemu
* virtual devices do not work correctly otherwise
*/
if (c->buffersize < 512) {
c->buffersize = 512;
}
So min. latency is 12ms @44.1 kHz.
> >> I get no stuttering issues like is commonly
> >> reported for ALSA and PA, and allows for a high degree of
> >> reconfigurability. The guest VM overall performs far better also as
> >> windows is never waiting on the audio device due to the decoupling
> >> provided by the ring buffer in my implementation.
> >
> > Yeah, looks good indeed!
The ringbuffer implementation looks a bit wild:
/* read PCM interleaved */
static int qjack_buffer_read(QJackBuffer *buffer, float *dest, int size)
{
assert(buffer->data);
const int samples = size / sizeof(float);
int frames = samples / buffer->channels;
const int avail = atomic_load_acquire(&buffer->used);
if (frames > avail) {
frames = avail;
}
int copy = frames;
int rptr = buffer->rptr;
while (copy) {
for (int c = 0; c < buffer->channels; ++c) {
*dest++ = buffer->data[c][rptr];
}
if (++rptr == buffer->frames) {
rptr = 0;
}
--copy;
}
buffer->rptr = rptr;
atomic_sub(&buffer->used, frames);
return frames * buffer->channels * sizeof(float);
}
On both sides there is no check whether one side is over/underrunning the
other side (rptr vs. wptr). I would really recommend using an existing
ringbuffer implementation instead of writing one by yourself.
And question:
static size_t qjack_write(HWVoiceOut *hw, void *buf, size_t len)
{
QJackOut *jo = (QJackOut *)hw;
++jo->c.packets;
if (jo->c.state != QJACK_STATE_RUNNING) {
qjack_client_recover(&jo->c);
return len;
}
qjack_client_connect_ports(&jo->c);
return qjack_buffer_write(&jo->c.fifo, buf, len);
}
So you are ensuring to reconnect the JACK ports in every cycle. Isn't that a
bit often?
Best regards,
Christian Schoenebeck
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] audio/jack: fix use after free segfault
2020-08-19 15:51 ` Christian Schoenebeck
@ 2020-08-19 15:57 ` Geoffrey McRae
2020-08-20 13:14 ` Christian Schoenebeck
0 siblings, 1 reply; 12+ messages in thread
From: Geoffrey McRae @ 2020-08-19 15:57 UTC (permalink / raw)
To: Christian Schoenebeck; +Cc: qemu-devel, kraxel
On 2020-08-20 01:51, Christian Schoenebeck wrote:
> On Mittwoch, 19. August 2020 14:51:52 CEST Geoffrey McRae wrote:
>> >> > What latencies do you achieve BTW with Windows guests?
>> >>
>> >> Never tested, it's not the reason why I use jack.
>> >
>> > Surpring that you never checked the min. latency there, as you nailed
>> > quite an
>> > ambitous jack driver into QEMU which I just realize now. Must have been
>> > splipped my awareness due to traffic.
>>
>> Sorry, I should have been clearer. I have tested windows and the
>> latency
>> is excellent, but I have never performed any empirical measurements.
>
> /*
> * ensure the buffersize is no smaller then 512 samples, some
> (all?) qemu
> * virtual devices do not work correctly otherwise
> */
> if (c->buffersize < 512) {
> c->buffersize = 512;
> }
>
> So min. latency is 12ms @44.1 kHz.
>
>> >> I get no stuttering issues like is commonly
>> >> reported for ALSA and PA, and allows for a high degree of
>> >> reconfigurability. The guest VM overall performs far better also as
>> >> windows is never waiting on the audio device due to the decoupling
>> >> provided by the ring buffer in my implementation.
>> >
>> > Yeah, looks good indeed!
>
> The ringbuffer implementation looks a bit wild:
>
> /* read PCM interleaved */
> static int qjack_buffer_read(QJackBuffer *buffer, float *dest, int
> size)
> {
> assert(buffer->data);
> const int samples = size / sizeof(float);
> int frames = samples / buffer->channels;
> const int avail = atomic_load_acquire(&buffer->used);
>
> if (frames > avail) {
> frames = avail;
> }
>
> int copy = frames;
> int rptr = buffer->rptr;
>
> while (copy) {
>
> for (int c = 0; c < buffer->channels; ++c) {
> *dest++ = buffer->data[c][rptr];
> }
>
> if (++rptr == buffer->frames) {
> rptr = 0;
> }
>
> --copy;
> }
>
> buffer->rptr = rptr;
>
> atomic_sub(&buffer->used, frames);
> return frames * buffer->channels * sizeof(float);
> }
>
> On both sides there is no check whether one side is over/underrunning
> the
> other side (rptr vs. wptr). I would really recommend using an existing
> ringbuffer implementation instead of writing one by yourself.
`buffer->used` ensures there is no overwrite unless I have missed
something?
>
> And question:
>
> static size_t qjack_write(HWVoiceOut *hw, void *buf, size_t len)
> {
> QJackOut *jo = (QJackOut *)hw;
> ++jo->c.packets;
>
> if (jo->c.state != QJACK_STATE_RUNNING) {
> qjack_client_recover(&jo->c);
> return len;
> }
>
> qjack_client_connect_ports(&jo->c);
> return qjack_buffer_write(&jo->c.fifo, buf, len);
> }
>
> So you are ensuring to reconnect the JACK ports in every cycle. Isn't
> that a
> bit often?
No, please see the implementation of qjack_client_connect_ports.
>
> Best regards,
> Christian Schoenebeck
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] audio/jack: fix use after free segfault
2020-08-19 15:57 ` Geoffrey McRae
@ 2020-08-20 13:14 ` Christian Schoenebeck
0 siblings, 0 replies; 12+ messages in thread
From: Christian Schoenebeck @ 2020-08-20 13:14 UTC (permalink / raw)
To: qemu-devel; +Cc: Geoffrey McRae, kraxel
On Mittwoch, 19. August 2020 17:57:35 CEST Geoffrey McRae wrote:
> > The ringbuffer implementation looks a bit wild:
> >
> > /* read PCM interleaved */
> > static int qjack_buffer_read(QJackBuffer *buffer, float *dest, int
> > size)
> > {
> >
> > assert(buffer->data);
> > const int samples = size / sizeof(float);
> > int frames = samples / buffer->channels;
> > const int avail = atomic_load_acquire(&buffer->used);
> >
> > if (frames > avail) {
> >
> > frames = avail;
> >
> > }
> >
> > int copy = frames;
> > int rptr = buffer->rptr;
> >
> > while (copy) {
> >
> > for (int c = 0; c < buffer->channels; ++c) {
> >
> > *dest++ = buffer->data[c][rptr];
> >
> > }
> >
> > if (++rptr == buffer->frames) {
> >
> > rptr = 0;
> >
> > }
> >
> > --copy;
> >
> > }
> >
> > buffer->rptr = rptr;
> >
> > atomic_sub(&buffer->used, frames);
> > return frames * buffer->channels * sizeof(float);
> >
> > }
> >
> > On both sides there is no check whether one side is over/underrunning
> > the
> > other side (rptr vs. wptr). I would really recommend using an existing
> > ringbuffer implementation instead of writing one by yourself.
>
> `buffer->used` ensures there is no overwrite unless I have missed
> something?
Right, I missed that you are using that separate variable for that. OK then!
Typical ringbuffer implementations only have a rptr and wptr. That's why I
missed it.
>
> > And question:
> >
> > static size_t qjack_write(HWVoiceOut *hw, void *buf, size_t len)
> > {
> >
> > QJackOut *jo = (QJackOut *)hw;
> > ++jo->c.packets;
> >
> > if (jo->c.state != QJACK_STATE_RUNNING) {
> >
> > qjack_client_recover(&jo->c);
> > return len;
> >
> > }
> >
> > qjack_client_connect_ports(&jo->c);
> > return qjack_buffer_write(&jo->c.fifo, buf, len);
> >
> > }
> >
> > So you are ensuring to reconnect the JACK ports in every cycle. Isn't
> > that a
> > bit often?
>
> No, please see the implementation of qjack_client_connect_ports.
Ah, you mean this entry check:
static void qjack_client_connect_ports(QJackClient *c)
{
if (!c->connect_ports || !c->opt->connect_ports) {
return;
}
...
It's okay. However on the long-term I would consider moving that away from the
audio thread as most JACK API functions are not deterministic, i.e. they could
lead to audio dropouts if executed on audio thread.
Best regards,
Christian Schoenebeck
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2020-08-20 13:16 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-08-18 12:40 [PATCH] audio/jack: fix use after free segfault Geoffrey McRae
2020-08-18 13:41 ` no-reply
2020-08-18 18:11 ` Christian Schoenebeck
2020-08-18 22:20 ` Geoffrey McRae
2020-08-19 11:30 ` Christian Schoenebeck
2020-08-19 11:45 ` Geoffrey McRae
2020-08-19 12:41 ` Christian Schoenebeck
2020-08-19 12:51 ` Geoffrey McRae
2020-08-19 15:51 ` Christian Schoenebeck
2020-08-19 15:57 ` Geoffrey McRae
2020-08-20 13:14 ` Christian Schoenebeck
2020-08-19 13:30 ` Gerd Hoffmann
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.