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

[Jan Sebechlebsky]

On 03/24/2016 09:51 PM, Marton Balint wrote:
>
> On Thu, 24 Mar 2016, Jan Sebechlebsky wrote:
>
>> 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.
>>
>> Signed-off-by: Jan Sebechlebsky <sebechlebskyjan at gmail.com>
>> ---
>> libavformat/tee.c | 22 ++++++++++++++--------
>> 1 file changed, 14 insertions(+), 8 deletions(-)
>>
>> diff --git a/libavformat/tee.c b/libavformat/tee.c
>> index 09551b3..e43ef08 100644
>> --- a/libavformat/tee.c
>> +++ b/libavformat/tee.c
>> @@ -141,12 +141,14 @@ static void close_slave(TeeSlave* tee_slave)
>>     unsigned i;
>>
>>     avf = tee_slave->avf;
>> -    for (i=0; i < avf->nb_streams; ++i) {
>> -        AVBitStreamFilterContext *bsf_next, *bsf = tee_slave->bsfs[i];
>> -        while (bsf) {
>> -            bsf_next = bsf->next;
>> -            av_bitstream_filter_close(bsf);
>> -            bsf = bsf_next;
>> +    if (tee_slave->bsfs) {
>> +        for (i=0; i < avf->nb_streams; ++i) {
>> +            AVBitStreamFilterContext *bsf_next, *bsf = 
>> tee_slave->bsfs[i];
>> +            while (bsf) {
>> +                bsf_next = bsf->next;
>> +                av_bitstream_filter_close(bsf);
>> +                bsf = bsf_next;
>> +            }
>>         }
>>     }
>>     av_freep(&tee_slave->stream_map);
>> @@ -198,6 +200,7 @@ static int open_slave(AVFormatContext *avf, char 
>> *slave, TeeSlave *tee_slave)
>>     ret = avformat_alloc_output_context2(&avf2, NULL, format, filename);
>>     if (ret < 0)
>>         goto end;
>> +    tee_slave->avf = avf2;
>>     av_dict_copy(&avf2->metadata, avf->metadata, 0);
>>     avf2->opaque   = avf->opaque;
>>     avf2->io_open  = avf->io_open;
>> @@ -277,7 +280,6 @@ static int open_slave(AVFormatContext *avf, char 
>> *slave, TeeSlave *tee_slave)
>>         goto end;
>>     }
>>
>> -    tee_slave->avf = avf2;
>>     tee_slave->bsfs = av_calloc(avf2->nb_streams, sizeof(TeeSlave));
>>     if (!tee_slave->bsfs) {
>>         ret = AVERROR(ENOMEM);
>> @@ -292,7 +294,8 @@ static int open_slave(AVFormatContext *avf, char 
>> *slave, TeeSlave *tee_slave)
>>                 av_log(avf, AV_LOG_ERROR,
>>                        "Specifier separator in '%s' is '%c', but only 
>> characters '%s' "
>>                        "are allowed\n", entry->key, *spec, 
>> slave_bsfs_spec_sep);
>> -                return AVERROR(EINVAL);
>> +                ret = AVERROR(EINVAL);
>> +                goto end;
>>             }
>>             spec++; /* consume separator */
>>         }
>> @@ -337,6 +340,9 @@ static int open_slave(AVFormatContext *avf, char 
>> *slave, TeeSlave *tee_slave)
>>     }
>>
>> end:
>> +    if ( ret < 0 ){
>> +        close_slave(tee_slave);
>> +    }
>
> Do you really need to call close_slave here? As far as I see if 
> open_slave fails then the failure path of tee_write_header will call 
> close_slaves, so the streams will be closed, therefore no need to do 
> it here.
You're right, actually at this point I don't need to call close_slave 
here. But it will be needed in the next commit, since close_slaves 
function skips already dead slaves and then in case of failure of a 
slave which is allowed to fail, this one would not be freed.

I'll move this to the next commit to have consistent changes in each one.
>
> Regards,
> Marton
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Have a nice day
Jan