[FFmpeg-devel] [PATCH 17/21] avfilter/formats: Fix double frees and memleaks on error
Nicolas George
george at nsup.org
Sun Aug 23 17:31:03 EEST 2020
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()?
> 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20200823/981d2e43/attachment.sig>
More information about the ffmpeg-devel
mailing list