[FFmpeg-devel] [PATCH v5 2/3] avformat/tee: Fix leaks in tee muxer when open_slave fails

[Jan Sebechlebsky]

On 04/15/2016 02:28 AM, Marton Balint wrote:
>
> On Thu, 14 Apr 2016, Marton Balint wrote:
>
>>
>> On Tue, 12 Apr 2016, sebechlebskyjan at gmail.com wrote:
>>
>>> From: Jan Sebechlebsky <sebechlebskyjan at gmail.com>
>>>
>>> Calling close_slave in case error is to be returned from open_slave
>>> will free allocated resources.
>>>
>>> Since failure can happen before bsfs array is initialized,
>>> close_slave must check that bsfs is not NULL before accessing
>>> tee_slave->bsfs[i] element.
>>>
>>> Slave muxer expects write_trailer to be called if it's
>>> write_header suceeded (so resources allocated in write_header
>>> are freed). Therefore if failure happens after successfull
>>> write_header call, we must ensure that write_trailer of
>>> that particular slave is called.
>>
>> Hmm, I guess you are right, I see no other way freeing resources 
>> allocated in write_header then calling write_trailer. It does make 
>> the code a bit more complex, but I don't really see a way to make it 
>> more simple.
>>
>> So this looks good to me. Nicolas, any ideas improving this?
>>
>
> Actually I have given this some additional thought, and by using a new 
> per-slave variable to keep the information if the header was written 
> or not, I think your patch can be simplifed, also close_slave can be 
> changed so it will write the trailer if necessary, in fact, the 
> write_trailer function can use it as well.
>
> I have modified your patch (see attached), could you please 
> test/review it and check if I missed anything? I hope you don't mind, 
> this kind of collaborative work is not that common in ffmpeg, but in 
> this case it seemed easier moving those few lines around than 
> describing what I wanted.
>
> Thanks,
> Marton

Hello Marton,
I'm ok with it, you're right it is more elegant this way. I've tested it 
and it seems allright. I've recreated the last patch on top of these 
changes and I'm sending it in attachment (and I am also cc-ing this mail 
to Nicolas, so he can review the patches).

Have a nice day!

Jan

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-avformat-tee-Handling-slave-failure-in-tee-muxer.patch
Type: text/x-patch
Size: 8493 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20160415/ba117a51/attachment.bin>