aboutsummaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorAndreas Rheinhardt <andreas.rheinhardt@gmail.com>2020-08-07 03:41:18 +0200
committerAndreas Rheinhardt <andreas.rheinhardt@gmail.com>2020-08-12 21:22:53 +0200
commit195a25a7aba89d4fc040de63cbc6389abece393d (patch)
tree325a8c7073a822ad343174a90733a74a21d66248
parentae5026c90502fad71338b81badd227db00b462cf (diff)
downloadffmpeg-195a25a7aba89d4fc040de63cbc6389abece393d.tar.gz
avfilter/formats: Leave lists' ownership unchanged upon merge failure
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. Reviewed-by: Nicolas George <george@nsup.org> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
-rw-r--r--libavfilter/formats.c40
1 files changed, 28 insertions, 12 deletions
diff --git a/libavfilter/formats.c b/libavfilter/formats.c
index 48b27d0c53..7099949d5c 100644
--- a/libavfilter/formats.c
+++ b/libavfilter/formats.c
@@ -33,17 +33,11 @@
/**
* 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 +48,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 +66,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 +91,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 +249,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: