[FFmpeg-devel] [PATCH 17/21] avfilter/formats: Fix double frees and memleaks on error
Andreas Rheinhardt
andreas.rheinhardt at gmail.com
Sun Aug 23 19:47:40 EEST 2020
Nicolas George:
> Andreas Rheinhardt (12020-08-09):
>> The formats API deals with lists of channel layouts, sample rates,
>> pixel formats and sample formats. These lists are refcounted in a way in
>> which the list structure itself contains pointers to all of its owners.
>> Furthermore, it is possible for a list to be not owned by anyone yet;
>> this status is temporary until the list has been attached to an owner.
>> Adding an owner to a list involves reallocating the list's list of
>> owners and can therefore fail.
>>
>> In order to reduce the amount of checks and cleanup code for the users
>> of this API, the API is supposed to be lenient when faced with input
>> lists that are NULL and it is supposed to clean up if adding an owner
>> to a list fails, so that a simple use case like
>>
>> list = ff_make_format_list(foo_fmts);
>> if ((ret = ff_formats_ref(list, &ctx->inputs[0]->out_formats)) < 0)
>> return ret;
>>
>> needn't check whether list could be successfully allocated
>> (ff_formats_ref() return AVERROR(ENOMEM) if it couldn't) and it also
>> needn't free list if ff_formats_ref() couldn't add an owner for it.
>>
>> But the cleaning up after itself was broken. The root cause was that
>> the refcount was decremented during unreferencing whether or not the
>> element to be unreferenced was actually an owner of the list or not.
>> This means that if the above sample code is continued by
>>
>> if ((ret = ff_formats_ref(list, &ctx->inputs[1]->out_formats)) < 0)
>> return ret;
>>
>> and that if an error happens at the second ff_formats_ref() call, the
>> automatic cleaning of list will decrement the refcount from 1 (the sole
>> owner of list at this moment is ctx->input[0]->out_formats) to 0 and so
>> the list will be freed; yet ctx->input[0]->out_formats still points to
>> the list and this will lead to a double free/use-after-free when
>> ctx->input[0] is freed later.
>>
>> Presumably in order to work around such an issue, commit
>> 93afb338a405eac0f9e7b092bc26603378bfcca6 restricted unreferencing to
>> lists with owners. This does not solve the root cause (the above example
>> is not fixed by this) at all, but it solves some crashs.
>>
>> This commit fixes the API: The list's refcount is only decremented if
>> an owner is removed from the list of owners and not if the
>> unref-function is called with a pointer that is not among the owners of
>> the list. Furtermore, the requirement for the list to have owners is
>> dropped.
>>
>> This implies that if the first call to ff_formats_ref() in the above
>> example fails, the refcount which is initially zero during unreferencing
>> is not modified, so that the list will be freed automatically in said
>> call to ff_formats_ref() as every list whose refcount reaches zero is.
>>
>> If on the other hand, the second call to ff_formats_ref() is the first
>> to fail, the refcount would stay at one during the automatic
>> unreferencing in ff_formats_ref(). The list would later be freed when
>> its last (and in this case sole) owner (namely
>> ctx->inputs[0]->out_formats) gets unreferenced.
>>
>> The issues described here for ff_formats_ref() also affected the other
>> functions of this API. E.g. ff_add_format() failed to clean up after
>> itself if adding an entry to an already existing list failed (the case
>> of a freshly allocated list was handled specially and this commit also
>> removes said code). E.g. ff_all_formats() inherited the flaw.
>>
>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt at gmail.com>
>> ---
>> The count variable in SET_COMMON_FORMATS is now btw unnecessary; it
>> would be safe to always unref fmt in this macro (which does nothing
>> except when fmt has no owner in which case it frees fmt).
>>
>> libavfilter/formats.c | 32 ++++++++++----------------------
>> 1 file changed, 10 insertions(+), 22 deletions(-)
>>
>> diff --git a/libavfilter/formats.c b/libavfilter/formats.c
>> index 3118aa0925..2379be1518 100644
>> --- a/libavfilter/formats.c
>> +++ b/libavfilter/formats.c
>> @@ -327,7 +327,6 @@ AVFilterChannelLayouts *avfilter_make_format64_list(const int64_t *fmts)
>> #define ADD_FORMAT(f, fmt, unref_fn, type, list, nb) \
>> do { \
>> type *fmts; \
>> - void *oldf = *f; \
>> \
>> if (!(*f) && !(*f = av_mallocz(sizeof(**f)))) { \
>> return AVERROR(ENOMEM); \
>> @@ -337,8 +336,6 @@ do { \
>> sizeof(*(*f)->list)); \
>> if (!fmts) { \
>> unref_fn(f); \
>
>> - if (!oldf) \
>> - av_freep(f); \
>
> Should you not keep the av_freep()?
>
No. If *f is freshly allocated, it has no owner yet and unref_fn(f) will
free it and set *f to NULL; av_freep(f) is then a no-op, so I removed
it. Keeping it would also be against the philosphy of this API (that it
cleans up after itself in case of error).
>> return AVERROR(ENOMEM); \
>> } \
>> \
>> @@ -499,16 +496,17 @@ do { \
>> do { \
>> int idx = -1; \
>> \
>
>> - if (!ref || !*ref || !(*ref)->refs) \
>> + if (!ref || !*ref) \
>
> This is unrelated, but since this line is changing anyway: the !ref test
> is invalid, calling ff_formats_unref() or ff_channel_layouts_unref() in
> anything except &something is ridiculously invalid.
>
I know, but as you said: This would be unrelated.
>> return; \
>> \
>> FIND_REF_INDEX(ref, idx); \
>> \
>> - if (idx >= 0) \
>
>> + if (idx >= 0) { \
>> memmove((*ref)->refs + idx, (*ref)->refs + idx + 1, \
>> sizeof(*(*ref)->refs) * ((*ref)->refcount - idx - 1)); \
>> - \
>> - if(!--(*ref)->refcount) { \
>> + --(*ref)->refcount; \
>> + } \
>> + if (!(*ref)->refcount) { \
>> av_free((*ref)->list); \
>> av_free((*ref)->refs); \
>> av_free(*ref); \
>> @@ -550,7 +548,7 @@ void ff_formats_changeref(AVFilterFormats **oldref, AVFilterFormats **newref)
>> FORMATS_CHANGEREF(oldref, newref);
>> }
>>
>> -#define SET_COMMON_FORMATS(ctx, fmts, in_fmts, out_fmts, ref_fn, unref_fn, list) \
>> +#define SET_COMMON_FORMATS(ctx, fmts, in_fmts, out_fmts, ref_fn, unref_fn) \
>> int count = 0, i; \
>> \
>> if (!fmts) \
>> @@ -560,10 +558,6 @@ void ff_formats_changeref(AVFilterFormats **oldref, AVFilterFormats **newref)
>> if (ctx->inputs[i] && !ctx->inputs[i]->out_fmts) { \
>> int ret = ref_fn(fmts, &ctx->inputs[i]->out_fmts); \
>> if (ret < 0) { \
>> - unref_fn(&fmts); \
>> - if (fmts) \
>> - av_freep(&fmts->list); \
>> - av_freep(&fmts); \
>> return ret; \
>> } \
>> count++; \
>> @@ -573,10 +567,6 @@ void ff_formats_changeref(AVFilterFormats **oldref, AVFilterFormats **newref)
>> if (ctx->outputs[i] && !ctx->outputs[i]->in_fmts) { \
>> int ret = ref_fn(fmts, &ctx->outputs[i]->in_fmts); \
>> if (ret < 0) { \
>> - unref_fn(&fmts); \
>> - if (fmts) \
>> - av_freep(&fmts->list); \
>> - av_freep(&fmts); \
>> return ret; \
>> } \
>> count++; \
>> @@ -584,9 +574,7 @@ void ff_formats_changeref(AVFilterFormats **oldref, AVFilterFormats **newref)
>> } \
>> \
>> if (!count) { \
>> - av_freep(&fmts->list); \
>> - av_freep(&fmts->refs); \
>> - av_freep(&fmts); \
>> + unref_fn(&fmts); \
>> } \
>> \
>> return 0;
>> @@ -595,14 +583,14 @@ int ff_set_common_channel_layouts(AVFilterContext *ctx,
>> AVFilterChannelLayouts *layouts)
>> {
>> SET_COMMON_FORMATS(ctx, layouts, in_channel_layouts, out_channel_layouts,
>> - ff_channel_layouts_ref, ff_channel_layouts_unref, channel_layouts);
>> + ff_channel_layouts_ref, ff_channel_layouts_unref);
>> }
>>
>> int ff_set_common_samplerates(AVFilterContext *ctx,
>> AVFilterFormats *samplerates)
>> {
>> SET_COMMON_FORMATS(ctx, samplerates, in_samplerates, out_samplerates,
>> - ff_formats_ref, ff_formats_unref, formats);
>> + ff_formats_ref, ff_formats_unref);
>> }
>>
>> /**
>> @@ -613,7 +601,7 @@ int ff_set_common_samplerates(AVFilterContext *ctx,
>> int ff_set_common_formats(AVFilterContext *ctx, AVFilterFormats *formats)
>> {
>> SET_COMMON_FORMATS(ctx, formats, in_formats, out_formats,
>> - ff_formats_ref, ff_formats_unref, formats);
>> + ff_formats_ref, ff_formats_unref);
>> }
>>
>> static int default_query_formats_common(AVFilterContext *ctx,
>
> I think it is ok.
>
> Thanks for looking into all this.
>
> Regards,
>
I will push it as soon as you have approved the removal of av_freep() above.
- Andreas
More information about the ffmpeg-devel
mailing list