[pulseaudio-discuss] [PATCH 1/3] pstream: Don't split (non-SHM) memblocks

[Tanu Kaskinen]

On Fri, 2015-03-06 at 21:20 +0100, Peter Meerwald wrote:
> On Thu, 5 Mar 2015, David Henningsson wrote:
> 
> > In case SHM is full or disabled, audio data is sent through the
> > io/srbchannel. When this channel in turn gets full, memblocks
> > could previously be split up. This could lead to crashes in case
> > the split was on non-frame boundaries (in combination with full
> > memblock queues).
> > 
> > BugLink: https://bugs.freedesktop.org/show_bug.cgi?id=88452
> > Signed-off-by: David Henningsson <david.henningsson at canonical.com>
> > ---
> >  src/pulsecore/pstream.c | 67 +++++++++++++++++++++++--------------------------
> >  1 file changed, 31 insertions(+), 36 deletions(-)
> > 
> > diff --git a/src/pulsecore/pstream.c b/src/pulsecore/pstream.c
> > index b0ed5a7..8c05a87 100644
> > --- a/src/pulsecore/pstream.c
> > +++ b/src/pulsecore/pstream.c
> > @@ -682,6 +682,36 @@ fail:
> >      return -1;
> >  }
> >  
> > +static void memblock_complete(pa_pstream *p, struct pstream_read *re) {
> > +    size_t l;
> > +    pa_memchunk chunk;
> > +    int64_t offset;
> > +
> > +    if (!p->receive_memblock_callback)
> > +        return;
> > +
> > +    /* Is this memblock data? Then pass it to the user */
> > +    l = re->index - PA_PSTREAM_DESCRIPTOR_SIZE;
> > +    if (l <= 0)
> 
> l is an unsigned data type; the difference should not become negative and 
> the check should just be for (l == 0)

There are many places where this kind of redundant negativity checks are
done (I suppose Lennart likes that style), so for consistency reasons, I
wouldn't say that the check *should* be just for (l == 0). I certainly
wouldn't be against using (l == 0) either, though (I don't personally
like the style of using redundant negativity checks).

But that's maybe irrelevant anyway, because I think the whole check is
redundant. There is a check elsewhere that ensures that empty memblocks
are rejected, so l can never be zero.

Even if David prefers to keep the check "just in case", the comment
certainly needs editing. "Is this memblock data?" is a nonsensical
question here - of course it's memblock data, that should be obvious
from the function name.

Otherwise this patch looks good to me.

-- 
Tanu