[FFmpeg-devel] [PATCH 4/6] avfilter/formats: Leave lists' ownership unchanged upon merge failure

[Andreas Rheinhardt]

ff_merge_formats(), ff_merge_samplerates() and ff_merge_channel_layouts()
share common semantics: If merging succeeds, a non-NULL pointer is
returned and both input lists (of type AVFilterFormats resp.
AVFilterChannelLayouts) are to be treated as if they had been freed;
the owners of the input parameters (if any) become owners of the
returned list. If merging does not succeed, NULL is returned and both
input lists are supposed to be unchanged.

The problem is that the functions did not abide by these semantics:
In case of reallocation failure, it is possible for these functions
to return NULL after having already freed one of the two input list.
This happens because sometimes the refs-array of the destined output
gets reallocated twice to its final size and if the second of these
reallocations fails, the first of the two inputs has already been freed
and its refs updated to point to the destined output which in this case
will be freed immediately so that all of the already updated pointers
are now dangling. This leads to use-after-frees and memory corruptions
lateron (when these owners get cleaned up, the lists they own get
unreferenced). Should the input lists don't have owners at all, the
caller (namely can_merge_formats() in avfiltergraph.c) thinks that both
the input lists are unchanged and need to be freed, leading to a double
free.

The solution to this is simple: Don't reallocate twice; do it just once.
This also saves a reallocation.

This commit fixes the issue behind Coverity issue #1452636. It might
also make Coverity realize that the issue has been fixed.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt at gmail.com>
---
This is not the only thing wrong with the formats API. Will soon send more;
those who can't wait can already take a look at
https://github.com/mkver/FFmpeg/commits/avfilter

The most important of these patches is
https://github.com/mkver/FFmpeg/commit/a151b88f395387c4bb85fbf14c042e2cd3f677ed

 libavfilter/formats.c | 41 +++++++++++++++++++++++++++++------------
 1 file changed, 29 insertions(+), 12 deletions(-)

diff --git a/libavfilter/formats.c b/libavfilter/formats.c
index 48b27d0c53..ae44006dfe 100644
--- a/libavfilter/formats.c
+++ b/libavfilter/formats.c
@@ -31,19 +31,14 @@
 
 #define KNOWN(l) (!FF_LAYOUT2COUNT(l)) /* for readability */
 
+
 /**
  * Add all refs from a to ret and destroy a.
+ * ret->refs must have enough spare room left for this.
  */
-#define MERGE_REF(ret, a, fmts, type, fail)                                \
+#define MERGE_REF_NO_ALLOC(ret, a, fmts)                                   \
 do {                                                                       \
-    type ***tmp;                                                           \
     int i;                                                                 \
-                                                                           \
-    if (!(tmp = av_realloc_array(ret->refs, ret->refcount + a->refcount,   \
-                                 sizeof(*tmp))))                           \
-        goto fail;                                                         \
-    ret->refs = tmp;                                                       \
-                                                                           \
     for (i = 0; i < a->refcount; i ++) {                                   \
         ret->refs[ret->refcount] = a->refs[i];                             \
         *ret->refs[ret->refcount++] = ret;                                 \
@@ -54,6 +49,17 @@ do {                                                                       \
     av_freep(&a);                                                          \
 } while (0)
 
+#define MERGE_REF(ret, a, fmts, type, fail)                                \
+do {                                                                       \
+    type ***tmp;                                                           \
+                                                                           \
+    if (!(tmp = av_realloc_array(ret->refs, ret->refcount + a->refcount,   \
+                                 sizeof(*tmp))))                           \
+        goto fail;                                                         \
+    ret->refs = tmp;                                                       \
+    MERGE_REF_NO_ALLOC(ret, a, fmts);                                      \
+} while (0)
+
 /**
  * Add all formats common for a and b to ret, copy the refs and destroy
  * a and b.
@@ -61,6 +67,7 @@ do {                                                                       \
 #define MERGE_FORMATS(ret, a, b, fmts, nb, type, fail)                          \
 do {                                                                            \
     int i, j, k = 0, count = FFMIN(a->nb, b->nb);                               \
+    type ***tmp;                                                                \
                                                                                 \
     if (!(ret = av_mallocz(sizeof(*ret))))                                      \
         goto fail;                                                              \
@@ -85,8 +92,13 @@ do {
     if (!ret->nb)                                                               \
         goto fail;                                                              \
                                                                                 \
-    MERGE_REF(ret, a, fmts, type, fail);                                        \
-    MERGE_REF(ret, b, fmts, type, fail);                                        \
+    tmp = av_realloc_array(NULL, a->refcount + b->refcount, sizeof(*tmp));      \
+    if (!tmp)                                                                   \
+        goto fail;                                                              \
+    ret->refs = tmp;                                                            \
+                                                                                \
+    MERGE_REF_NO_ALLOC(ret, a, fmts);                                           \
+    MERGE_REF_NO_ALLOC(ret, b, fmts);                                           \
 } while (0)
 
 AVFilterFormats *ff_merge_formats(AVFilterFormats *a, AVFilterFormats *b,
@@ -238,8 +250,13 @@ AVFilterChannelLayouts *ff_merge_channel_layouts(AVFilterChannelLayouts *a,
     ret->nb_channel_layouts = ret_nb;
     if (!ret->nb_channel_layouts)
         goto fail;
-    MERGE_REF(ret, a, channel_layouts, AVFilterChannelLayouts, fail);
-    MERGE_REF(ret, b, channel_layouts, AVFilterChannelLayouts, fail);
+
+    ret->refs = av_realloc_array(NULL, a->refcount + b->refcount,
+                                 sizeof(*ret->refs));
+    if (!ret->refs)
+        goto fail;
+    MERGE_REF_NO_ALLOC(ret, a, channel_layouts);
+    MERGE_REF_NO_ALLOC(ret, b, channel_layouts);
     return ret;
 
 fail:
-- 
2.20.1