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

[Christian Schoenebeck <qemu_oss@crudebyte.com>]

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