[pulseaudio-discuss] [PATCH] bluez5-device: Fix memory leak in sco_process_render()

[Raman Shishniou]

Hello,

On 04/10/2018 01:38 PM, Georg Chini wrote:
> On 10.04.2018 10:21, Raman Shishniou wrote:
>> Hello,
>>
>> On 04/09/2018 07:15 PM, Georg Chini wrote:
>>> sco_process_render does not unref the memblock when it encounters an error.
>>>
>>> This patch fixes the issue. It also changes the return value to 1 in the case
>>> of EAGAIN. Because the data was already rendered and cannot be re-sent, we
>>> have to discard the block.
>>> ---
>>>   src/modules/bluetooth/module-bluez5-device.c | 12 +++++++++---
>>>   1 file changed, 9 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/src/modules/bluetooth/module-bluez5-device.c b/src/modules/bluetooth/module-bluez5-device.c
>>> index 95d288ef..b81c233c 100644
>>> --- a/src/modules/bluetooth/module-bluez5-device.c
>>> +++ b/src/modules/bluetooth/module-bluez5-device.c
>>> @@ -282,9 +282,13 @@ static int sco_process_render(struct userdata *u) {
>>>           if (errno == EINTR)
>>>               /* Retry right away if we got interrupted */
>>>               continue;
>>> -        else if (errno == EAGAIN)
>>> -            /* Hmm, apparently the socket was not writable, give up for now */
>>> -            return 0;
>>> +
>>> +        pa_memblock_unref(memchunk.memblock);
>>> +
>>> +        if (errno == EAGAIN)
>>> +            /* Hmm, apparently the socket was not writable, give up for now.
>>> +             * Because the data was already rendered, let's discard the block. */
>>> +            return 1;
>>>   
>> 1. errno value can be changed during pa_memblock_unref()
> 
> I don't think so. The only possible system calls used during unref should be
> calls to free() and because we always use pa_xfree(), the errno value will
> be preserved.
> 

I seen callback processing during memblock_free()

>> 2. I think the same changes are required for a2dp_process_render() too.
> 
> No, a2dp_process_render() works slightly different. It keeps the memchunk
> in userdata and tries to re-send the same block again.
> 
>>
>>
>>>           pa_log_error("Failed to write data to SCO socket: %s", pa_cstrerror(errno));
>>>           return -1;
>>> @@ -296,6 +300,8 @@ static int sco_process_render(struct userdata *u) {
>>>           pa_log_error("Wrote memory block to socket only partially! %llu written, wanted to write %llu.",
>>>                       (unsigned long long) l,
>>>                       (unsigned long long) memchunk.length);
>>> +
>>> +        pa_memblock_unref(memchunk.memblock);
>>>           return -1;
>>>       }
>>>  
>> -- 
>> Raman
> 
>