[FFmpeg-devel] [PATCH 2/2] avfilter: add sidechaingate filter

[Ganesh Ajjanagadde]

On Wed, Dec 2, 2015 at 1:42 AM, Paul B Mahol <onemda at gmail.com> wrote:
> On 12/2/15, Paul B Mahol <onemda at gmail.com> wrote:
>> On 12/2/15, Ganesh Ajjanagadde <gajjanag at mit.edu> wrote:
>>> On Tue, Dec 1, 2015 at 2:14 PM, Paul B Mahol <onemda at gmail.com> wrote:
>>>> On 12/1/15, Nicolas George <george at nsup.org> wrote:
>>>>> Le primidi 11 frimaire, an CCXXIV, Paul B Mahol a ecrit :
>>>>>> Similar how its freed when no longer used.
>>>>>
>>>>> Please elaborate. I know the API, I do not see what you suggest.
>>>>>
>>>>> (Thanks for trimming.)
>>>>>
>>>>> Regards,
>>>>
>>>> After carefully looking at this functon, I see no leaking at all.
>>>
>>> Care to elaborate on this? My question is: what happens when e.g
>>> layouts get set and allocated/ref'ed correctly, but while trying to
>>> allocate formats, ENOMEM occurs? Or in other words, where does the
>>> formats get deallocated in such a case? And why does Coverity flag
>>> these things?
>>
>> Maybe it is for test program: libavfilter/filtfmts.c
>>
>
> Also I changed return value of query_formats to always be -1 and run
> program under 'valgrind --leak-check=full --show-leak-kinds=all'
> I see unrelated leaks from af_aformat and others, and no leaks from
> query_formats.

You are being ambiguous here, and your testing was likely not thorough enough.

Please try the following:
diff --git a/libavfilter/af_agate.c b/libavfilter/af_agate.c
index 291e803..416b671 100644
--- a/libavfilter/af_agate.c
+++ b/libavfilter/af_agate.c
@@ -188,6 +188,7 @@ static int query_formats(AVFilterContext *ctx)
     layouts = ff_all_channel_counts();
     if (!layouts)
         return AVERROR(ENOMEM);
+    return AVERROR(ENOMEM);
     ret = ff_set_common_channel_layouts(ctx, layouts);
     if (ret < 0)
         return ret;

Valgrind output (trimmed for brevity; easily reproduced via similar
things to the above):
==13895== 32 bytes in 1 blocks are definitely lost in loss record 7 of 15
==13895==    at 0x4C2AD45: memalign (in
/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==13895==    by 0x4C2AE0D: posix_memalign (in
/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==13895==    by 0x77948C6: av_malloc (mem.c:97)
==13895==    by 0x7794A9D: av_mallocz (mem.c:254)
==13895==    by 0x50DD13D: ff_all_channel_counts (formats.c:401)
==13895==    by 0x50A3AA4: query_formats (af_agate.c:188)
==13895==    by 0x50D1225: filter_query_formats (avfiltergraph.c:307)
==13895==    by 0x50D1A1B: query_formats (avfiltergraph.c:435)
==13895==    by 0x50D26F2: graph_config_formats (avfiltergraph.c:1139)
==13895==    by 0x50D26F2: avfilter_graph_config (avfiltergraph.c:1250)
==13895==    by 0x4193D4: configure_filtergraph (ffmpeg_filter.c:1042)
==13895==    by 0x422E62: transcode_init (ffmpeg.c:3035)
==13895==    by 0x424685: transcode (ffmpeg.c:4073)

This is admittedly a somewhat unfair test, since this does not exactly
represent the failure pattern of ff_set_common_channel_layouts.

Nevertheless, there is a better illustration, but this needs an
important patch fixing a null pointer dereference (that I will submit
soon since I think it is needed for a corner case):
diff --git a/libavfilter/formats.c b/libavfilter/formats.c
index 2b13cbf..35a934a 100644
--- a/libavfilter/formats.c
+++ b/libavfilter/formats.c
@@ -411,11 +411,11 @@ AVFilterChannelLayouts *ff_all_channel_counts(void)
     if (!f || !ref)
          \
         return AVERROR(ENOMEM);
          \

          \
-    tmp = av_realloc_array(f->refs, sizeof(*f->refs), f->refcount +
1);         \
-    if (!tmp) {
          \
+    /*tmp = av_realloc_array(f->refs, sizeof(*f->refs), f->refcount +
1);*/         \
+    /*if (!tmp) {*/
              \
         unref_fn(&f);
          \
         return AVERROR(ENOMEM);
          \
-    }
          \
+    /*}*/
              \
     f->refs = tmp;
          \
     f->refs[f->refcount++] = ref;
          \
     *ref = f;
          \
@@ -445,7 +445,7 @@ do {                                        \
 do {                                                               \
     int idx = -1;                                                  \
                                                                    \
-    if (!*ref)                                                     \
+    if (!*ref || !(*ref)->refs)                                    \
         return;                                                    \
                                                                    \
     FIND_REF_INDEX(ref, idx);                                      \

(the second hunk is the patch that is needed)
yielding similar memory leaks.

>
> PS: I better wasted this time on writing equirectangular2cubic filter.

It is as important to fix issues as it is to write new filters, that
continue to suffer from a recognized problem. Fixing it now makes sure
that new filters are not affected by it.

To reiterate, I had no problem with the leak per se; it is somewhat
subtle and not easily caught and does not "practically" occur due to
rarity of malloc failures. There are likely far worse issues in FFmpeg
or even avfilter than this. But when it is caught at the time of
review, and the reviewer asks for verification, it needs to be
addressed. Even if you told me "I don't have time for this now, it is
mostly theoretical", I would have been all right with it and pinged
you after a few weeks to make sure it got addressed at some point.

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