On 2015-03-10 14:25, Tanu Kaskinen wrote: > 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. > Thanks, pushed all three after removing the "if" and the comment. The third one's indentation was fixed up as well. // David
