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

[Christian Schoenebeck <qemu_oss@crudebyte.com>]

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