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

[Marton Balint]

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