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
