Re: [PATCH] audio: Add sndio backend

[Volker Rümelin]

Hi Brad,

I tested the sndio backend on my Linux system and I found a bug in the 
sndio backend. The problem is that the function audio_run() can call the 
function sndio_enable_out() to disable audio playback.

In the sndio_poll_event() function, audio_run() is called, which removes 
the poll handlers when the playback stream is stopped by calling 
sndio_enable_out(). Next, sndio_poll_wait() is called in 
sndio_poll_event(), which can reinstall the poll handlers of the stopped 
stream again. After a subsequent call to sndio_fini_out(), the pindex 
pointer of the still installed poll handlers points to a memory area 
that has already been freed. This can lead to a segmentation fault or a 
QEMU lockup because of a blocking read.

I suggest to use a flag to prevent that sndio_poll_event() reinstalls 
the poll handlers.

> +typedef struct SndioVoice {
> +    union {
> +        HWVoiceOut out;
> +        HWVoiceIn in;
> +    } hw;
> +    struct sio_par par;
> +    struct sio_hdl *hdl;
> +    struct pollfd *pfds;
> +    struct pollindex {
> +        struct SndioVoice *self;
> +        int index;
> +    } *pindexes;
> +    unsigned char *buf;
> +    size_t buf_size;
> +    size_t sndio_pos;
> +    size_t qemu_pos;
> +    unsigned int mode;
> +    unsigned int nfds;

+    bool enabled;

> +} SndioVoice;

> +/*
> + * call-back called when one of the descriptors
> + * became readable or writable
> + */
> +static void sndio_poll_event(SndioVoice *self, int index, int event)
> +{
> +    int revents;
> +
> +    /*
> +     * ensure we're not called twice this cycle
> +     */
> +    sndio_poll_clear(self);
> +
> +    /*
> +     * make self->pfds[] look as we're returning from poll syscal,
> +     * this is how sio_revents expects events to be.
> +     */
> +    self->pfds[index].revents = event;
> +
> +    /*
> +     * tell sndio to handle events and return whether we can read or
> +     * write without blocking.
> +     */
> +    revents = sio_revents(self->hdl, self->pfds);
> +    if (self->mode == SIO_PLAY) {
> +        if (revents & POLLOUT) {
> +            sndio_write(self);
> +        }
> +
> +        if (self->qemu_pos < self->buf_size) {
> +            audio_run(self->hw.out.s, "sndio_out");
> +        }
> +    } else {
> +        if (revents & POLLIN) {
> +            sndio_read(self);
> +        }
> +
> +        if (self->qemu_pos < self->sndio_pos) {
> +            audio_run(self->hw.in.s, "sndio_in");
> +        }
> +    }
> +
> +    sndio_poll_wait(self);

-    sndio_poll_wait(self);
+    if (self->enabled) {
+        sndio_poll_wait(self);
+    }

> +}

> +/*
> + * return a buffer where data to play can be stored
> + */
> +static size_t sndio_put_buffer_out(HWVoiceOut *hw, void *buf, size_t size)
> +{
> +    SndioVoice *self = (SndioVoice *) hw;
> +
> +    self->qemu_pos += size;
> +    sndio_poll_wait(self);
> +    return size;
> +}

> +/*
> + * discard the given amount of recorded data
> + */
> +static void sndio_put_buffer_in(HWVoiceIn *hw, void *buf, size_t size)
> +{
> +    SndioVoice *self = (SndioVoice *) hw;
> +
> +    self->qemu_pos += size;
> +    if (self->qemu_pos == self->buf_size) {
> +        self->qemu_pos = 0;
> +        self->sndio_pos = 0;
> +    }
> +    sndio_poll_wait(self);
> +}

It's not necessary to guard sndio_poll_wait() in sndio_put_buffer_out() 
and sndio_put_buffer_in() because audio_run() will never call those 
functions for a disabled stream.

> +static void sndio_enable(SndioVoice *self, bool enable)
> +{
> +    if (enable) {
> +        sio_start(self->hdl);

+        self->enabled = true;

> +        sndio_poll_wait(self);
> +    } else {

+        self->enabled = false;

> +        sndio_poll_clear(self);
> +        sio_stop(self->hdl);
> +    }
> +}

With best regards,
Volker