[FFmpeg-devel] [PATCH v5 2/3] avformat/tee: Fix leaks in tee muxer when open_slave fails
Marton Balint
cus at passwd.hu
Sun Apr 17 19:54:04 CEST 2016
On Fri, 15 Apr 2016, Jan Sebechlebsky wrote:
>
> 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).
>
[...]
> @@ -423,8 +486,9 @@ static int tee_write_header(AVFormatContext *avf)
> return 0;
>
> fail:
> - for (i = 0; i < nb_slaves; i++)
> + for (i = 0; i < nb_slaves; i++) {
> av_freep(&slaves[i]);
> + }
This seems a no-op change.
[...]
> static int tee_write_trailer(AVFormatContext *avf)
> {
> TeeContext *tee = avf->priv_data;
> + AVFormatContext *avf2;
> int ret_all = 0, ret;
> unsigned i;
>
> for (i = 0; i < tee->nb_slaves; i++) {
> - if ((ret = close_slave(&tee->slaves[i])) < 0)
> - if (!ret_all)
> + if (!(avf2 = tee->slaves[i].avf))
> + continue;
> + if ((ret = close_slave(&tee->slaves[i])) < 0) {
> + ret = tee_process_slave_failure(avf2, i, ret);
> + if (!ret_all && ret < 0)
> ret_all = ret;
> + }
> }
> + close_slaves(avf);
I guess you don't need close_slaves here, because you already closed all
slaves.
Regards,
Marton
More information about the ffmpeg-devel
mailing list