[FFmpeg-devel] [PATCH 4/6] avfilter/formats: Leave lists' ownership unchanged upon merge failure
Andreas Rheinhardt
andreas.rheinhardt at gmail.com
Tue Aug 11 15:34:06 EEST 2020
Nicolas George:
> Andreas Rheinhardt (12020-08-08):
>> 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>
>> ---
>
> Sorry for the delay, there was a detail that bugged me, I had to look at
> it carefully.
>
No problem, I never expected the reviewing process to be instantaneous.
>> 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)); \
>
> I was worried about the NULL here, because it means allocating a new
> array, even if the memory we already have fits. But it is no matter,
> because the first call to MERGE_REF is done with ret->refs being NULL.
>
> So patch ok, good catch.
>
> Possibly: add a comment:
>
> /* TODO see if we can reallocate a->refs, or the larger of a->refs and
> b->refs instead of allocating a brand new array.
>
> */
Do you remember
https://ffmpeg.org/pipermail/ffmpeg-devel/2020-August/267549.html? If I
were allowed to reorder a and b so that a->nb <= b->nb, one would not
have to allocate a new list to return at all: One could reuse a and
would only have to reallocate a->refs to also include b->refs and update
the refs coming from b (the ones from a are already fine!).
I have also pondered a second possible way of improvement: There are two
types of callers (all in avfiltergraph.c) of the ff_merge_ functions:
can_merge_formats() actually only wants to know whether two formats are
mergeable and to do so it currently creates short-lived throwaway copies
of the lists. Very wasteful. can_merge_formats() could be removed
altogether if one would implement check-only functions (which should
return an int, not an AVFilterFormats *).
The other callers also don't really inspect the current return value at
all except from checking whether it is NULL or not (that's because in
this case the returned AVFilterFormats has owners, so one doesn't need
to care about leaks). These could be satisfied by a function returning
an int which also allows to actually distinguish between inability to
merge and (re)allocation failures (< 0 would be errors, 0 inability to
merge and 1 successfull merge).
- Andreas
More information about the ffmpeg-devel
mailing list