diff mbox series

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

Message ID 20200809155748.30092-11-andreas.rheinhardt@gmail.com
State Accepted
Headers show
Series [FFmpeg-devel,1/6] avfilter/formats: Remove ff_make_formatu64_list()
Related show

Checks

Context Check Description
andriy/default pending
andriy/make success Make finished
andriy/make_fate success Make fate finished

Commit Message

Andreas Rheinhardt Aug. 9, 2020, 3:57 p.m. UTC
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@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(-)

Comments

Nicolas George Aug. 23, 2020, 2:31 p.m. UTC | #1
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@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()?

>          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.

>          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,
Andreas Rheinhardt Aug. 23, 2020, 4:47 p.m. UTC | #2
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@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
Andreas Rheinhardt Aug. 23, 2020, 5:54 p.m. UTC | #3
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@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
Nicolas George Aug. 23, 2020, 5:59 p.m. UTC | #4
Andreas Rheinhardt (12020-08-23):
> > 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.)

Right, thanks for explaining.
diff mbox series

Patch

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);                                    \
         return AVERROR(ENOMEM);                             \
     }                                                       \
                                                             \
@@ -499,16 +496,17 @@  do {                                        \
 do {                                                               \
     int idx = -1;                                                  \
                                                                    \
-    if (!ref || !*ref || !(*ref)->refs)                            \
+    if (!ref || !*ref)                                             \
         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,