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

[David Henningsson]

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