[FFmpeg-devel] [PATCH 17/21] avfilter/formats: Fix double frees and memleaks on error

[Andreas Rheinhardt]

Andreas Rheinhardt:
> 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).
> 

Actually, no has no option but to remove said code:
"The value of a pointer becomes indeterminate when the object it points
to reaches the end of its lifetime." (C99, 6.2.4.2)

If *f doesn't have any owners, it has already been freed in unref_fn()
and oldf becomes a dangling pointer, so that using it in a check is
undefined behaviour. (Storing the information whether this is a freshly
allocated list in a different way (e.g. an int) would of course work,
but there is no point in doing so.)

- Andreas