Re: [PATCH] audio/jack: fix use after free segfault

[Geoffrey McRae <geoff@hostfission.com>]

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