[FFmpeg-devel] [PATCH 1/8] avfilter/vf_overlay: fix memory leaks

Ganesh Ajjanagadde gajjanag at mit.edu
Thu Dec 10 19:54:26 CET 2015


On Thu, Dec 10, 2015 at 7:54 AM, Ganesh Ajjanagadde <gajjanag at mit.edu> wrote:
> On Thu, Dec 10, 2015 at 3:47 AM, Paul B Mahol <onemda at gmail.com> wrote:
>> On 12/10/15, Ganesh Ajjanagadde <gajjanag at mit.edu> wrote:
>>> On Wed, Dec 9, 2015 at 6:09 PM, Paul B Mahol <onemda at gmail.com> wrote:
>>>> On 12/9/15, Ganesh Ajjanagadde <gajjanagadde at gmail.com> wrote:
>>>>> On Fri, Dec 4, 2015 at 9:39 AM, Ganesh Ajjanagadde
>>>>> <gajjanagadde at gmail.com> wrote:
>>>>>> Recent commits 6aaac24d72a7da631173209841a3944fcb4a3309 and
>>>>>> 3835554bf8ed78539a3492c239f979c0ab03a15f made progress towards cleaning
>>>>>> up usage of the formats API, and in particular fixed possible NULL
>>>>>> pointer
>>>>>> dereferences.
>>>>>>
>>>>>> This commit addresses the issue of possible resource leaks when some
>>>>>> intermediate
>>>>>> call fails.
>>>>>>
>>>>>> Tested with valgrind --leak-check=full --show-leak-kinds=all, and manual
>>>>>> simulation
>>>>>> of malloc/realloc failures.
>>>>>>
>>>>>> Fixes: CID 1338327.
>>>>>>
>>>>>> Signed-off-by: Ganesh Ajjanagadde <gajjanagadde at gmail.com>
>>>>>> ---
>>>>>>  libavfilter/vf_overlay.c | 32 +++++++++++++++++++++++---------
>>>>>>  1 file changed, 23 insertions(+), 9 deletions(-)
>>>>>>
>>>>>> diff --git a/libavfilter/vf_overlay.c b/libavfilter/vf_overlay.c
>>>>>> index 3c61731..68cfb1b 100644
>>>>>> --- a/libavfilter/vf_overlay.c
>>>>>> +++ b/libavfilter/vf_overlay.c
>>>>>> @@ -252,23 +252,31 @@ static int query_formats(AVFilterContext *ctx)
>>>>>>      switch (s->format) {
>>>>>>      case OVERLAY_FORMAT_YUV420:
>>>>>>          if (!(main_formats    =
>>>>>> ff_make_format_list(main_pix_fmts_yuv420)) ||
>>>>>> -            !(overlay_formats =
>>>>>> ff_make_format_list(overlay_pix_fmts_yuv420)))
>>>>>> -            return AVERROR(ENOMEM);
>>>>>> +            !(overlay_formats =
>>>>>> ff_make_format_list(overlay_pix_fmts_yuv420))) {
>>>>>> +                ret = AVERROR(ENOMEM);
>>>>>> +                goto fail;
>>>>>> +            }
>>>>>>          break;
>>>>>>      case OVERLAY_FORMAT_YUV422:
>>>>>>          if (!(main_formats    =
>>>>>> ff_make_format_list(main_pix_fmts_yuv422)) ||
>>>>>> -            !(overlay_formats =
>>>>>> ff_make_format_list(overlay_pix_fmts_yuv422)))
>>>>>> -            return AVERROR(ENOMEM);
>>>>>> +            !(overlay_formats =
>>>>>> ff_make_format_list(overlay_pix_fmts_yuv422))) {
>>>>>> +                ret = AVERROR(ENOMEM);
>>>>>> +                goto fail;
>>>>>> +            }
>>>>>>          break;
>>>>>>      case OVERLAY_FORMAT_YUV444:
>>>>>>          if (!(main_formats    =
>>>>>> ff_make_format_list(main_pix_fmts_yuv444)) ||
>>>>>> -            !(overlay_formats =
>>>>>> ff_make_format_list(overlay_pix_fmts_yuv444)))
>>>>>> -            return AVERROR(ENOMEM);
>>>>>> +            !(overlay_formats =
>>>>>> ff_make_format_list(overlay_pix_fmts_yuv444))) {
>>>>>> +                ret = AVERROR(ENOMEM);
>>>>>> +                goto fail;
>>>>>> +            }
>>>>>>          break;
>>>>>>      case OVERLAY_FORMAT_RGB:
>>>>>>          if (!(main_formats    = ff_make_format_list(main_pix_fmts_rgb))
>>>>>> ||
>>>>>> -            !(overlay_formats =
>>>>>> ff_make_format_list(overlay_pix_fmts_rgb)))
>>>>>> -            return AVERROR(ENOMEM);
>>>>>> +            !(overlay_formats =
>>>>>> ff_make_format_list(overlay_pix_fmts_rgb))) {
>>>>>> +                ret = AVERROR(ENOMEM);
>>>>>> +                goto fail;
>>>>>> +            }
>>>>>>          break;
>>>>>>      default:
>>>>>>          av_assert0(0);
>>>>>> @@ -277,9 +285,15 @@ static int query_formats(AVFilterContext *ctx)
>>>>>>      if ((ret = ff_formats_ref(main_formats   ,
>>>>>> &ctx->inputs[MAIN]->out_formats   )) < 0 ||
>>>>>>          (ret = ff_formats_ref(overlay_formats,
>>>>>> &ctx->inputs[OVERLAY]->out_formats)) < 0 ||
>>>>>>          (ret = ff_formats_ref(main_formats   ,
>>>>>> &ctx->outputs[MAIN]->in_formats   )) < 0)
>>>>>> -        return ret;
>>>>>> +            goto fail;
>>>>>>
>>>>>>      return 0;
>>>>>> +fail:
>>>>>> +    av_freep(&main_formats->formats);
>>>>>> +    av_freep(&main_formats);
>>>>>> +    av_freep(&overlay_formats->formats);
>>>>>> +    av_freep(&overlay_formats);
>>>>>> +    return ret;
>>>>>>  }
>>>>>>
>>>>>>  static const enum AVPixelFormat alpha_pix_fmts[] = {
>>>>>> --
>>>>>> 2.6.3
>>>>>>
>>>>>
>>>>> pushed, with the necessary modification described by Clement
>>>>
>>>> This tries to dereference uninitialized value.
>>>
>>> See right above; I did the desired modification and believe there is no
>>> issue.
>>> I might be wrong; but if so, could you please refer to the relevant
>>> cvslog message?
>>> Thanks.
>>
>> libavfilter/vf_overlay.c:275:13: warning: variable 'overlay_formats'
>> is used uninitialized whenever '||' condition is true
>> [-Wsometimes-uninitialized]
>>     if (!(main_formats    = ff_make_format_list(main_pix_fmts_rgb)) ||
>>         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> libavfilter/vf_overlay.c:295:9: note: uninitialized use occurs here
>>     if (overlay_formats)
>>         ^~~~~~~~~~~~~~~
>> libavfilter/vf_overlay.c:275:13: note: remove the '||' if its
>> condition is always false
>>     if (!(main_formats    = ff_make_format_list(main_pix_fmts_rgb)) ||
>>         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> libavfilter/vf_overlay.c:268:13: warning: variable 'overlay_formats'
>> is used uninitialized whenever '||' condition is true
>> [-Wsometimes-uninitialized]
>>     if (!(main_formats    = ff_make_format_list(main_pix_fmts_yuv444)) ||
>>         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> libavfilter/vf_overlay.c:295:9: note: uninitialized use occurs here
>>     if (overlay_formats)
>>         ^~~~~~~~~~~~~~~
>> libavfilter/vf_overlay.c:268:13: note: remove the '||' if its
>> condition is always false
>>     if (!(main_formats    = ff_make_format_list(main_pix_fmts_yuv444)) ||
>>         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> libavfilter/vf_overlay.c:261:13: warning: variable 'overlay_formats'
>> is used uninitialized whenever '||' condition is true
>> [-Wsometimes-uninitialized]
>>     if (!(main_formats    = ff_make_format_list(main_pix_fmts_yuv422)) ||
>>         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> libavfilter/vf_overlay.c:295:9: note: uninitialized use occurs here
>>     if (overlay_formats)
>>         ^~~~~~~~~~~~~~~
>> libavfilter/vf_overlay.c:261:13: note: remove the '||' if its
>> condition is always false
>>     if (!(main_formats    = ff_make_format_list(main_pix_fmts_yuv422)) ||
>>         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> libavfilter/vf_overlay.c:254:13: warning: variable 'overlay_formats'
>> is used uninitialized whenever '||' condition is true
>> [-Wsometimes-uninitialized]
>>     if (!(main_formats    = ff_make_format_list(main_pix_fmts_yuv420)) ||
>>         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> libavfilter/vf_overlay.c:295:9: note: uninitialized use occurs here
>>     if (overlay_formats)
>>         ^~~~~~~~~~~~~~~
>> libavfilter/vf_overlay.c:254:13: note: remove the '||' if its
>> condition is always false
>>     if (!(main_formats    = ff_make_format_list(main_pix_fmts_yuv420)) ||
>>         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> libavfilter/vf_overlay.c:249:37: note: initialize the variable
>> 'overlay_formats' to silence this warning
>>     AVFilterFormats *overlay_formats;
>>                                     ^
>>                                      = NULL
>> 4 warnings generated.
>
> Wish my GCC had shown this, thanks. Fixed and pushed.

Based on a comment by Clement earlier, I suspect that a lot of filters
in libavfilter have this issue. There are a number of possibilities
regarding this (in decreasing order of my preference):
1. Take an active stance, but with an incremental approach, one filter
at a time.
2. Take an active stance, but fix them en-masse with a large avfilter patch.
3. Ignore them as they do not occur unless ENOMEM happens, which
generally means something terribly wrong happened anyway. If Coverity
flags them, we address them to keep the Coverity count low.
4. Ignore altogether.

This is also complicated by the fact that very few are interested in
reviewing these things. So I could either send out patches, or fix
directly and push. Since I think all the caveats have been addressed
in general over the ML, and the work is fairly mechanical, I think it
may be ok if these go in without further review. Of course, exactly
what is done depends on the above.

>
>>>
>>>> _______________________________________________
>>>> ffmpeg-devel mailing list
>>>> ffmpeg-devel at ffmpeg.org
>>>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>> _______________________________________________
>>> ffmpeg-devel mailing list
>>> ffmpeg-devel at ffmpeg.org
>>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>>
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel at ffmpeg.org
>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel


More information about the ffmpeg-devel mailing list