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 <[email protected]> > --- > 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,
--
Nicolas George
signature.asc
Description: PGP signature
_______________________________________________ ffmpeg-devel mailing list [email protected] https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email [email protected] with subject "unsubscribe".
