[FFmpeg-devel] [PATCH 4/6] avfilter/formats: Leave lists' ownership unchanged upon merge failure
Nicolas George
george at nsup.org
Tue Aug 11 13:09:59 EEST 2020
Andreas Rheinhardt (12020-08-08):
> ff_merge_formats(), ff_merge_samplerates() and ff_merge_channel_layouts()
> share common semantics: If merging succeeds, a non-NULL pointer is
> returned and both input lists (of type AVFilterFormats resp.
> AVFilterChannelLayouts) are to be treated as if they had been freed;
> the owners of the input parameters (if any) become owners of the
> returned list. If merging does not succeed, NULL is returned and both
> input lists are supposed to be unchanged.
>
> The problem is that the functions did not abide by these semantics:
> In case of reallocation failure, it is possible for these functions
> to return NULL after having already freed one of the two input list.
> This happens because sometimes the refs-array of the destined output
> gets reallocated twice to its final size and if the second of these
> reallocations fails, the first of the two inputs has already been freed
> and its refs updated to point to the destined output which in this case
> will be freed immediately so that all of the already updated pointers
> are now dangling. This leads to use-after-frees and memory corruptions
> lateron (when these owners get cleaned up, the lists they own get
> unreferenced). Should the input lists don't have owners at all, the
> caller (namely can_merge_formats() in avfiltergraph.c) thinks that both
> the input lists are unchanged and need to be freed, leading to a double
> free.
>
> The solution to this is simple: Don't reallocate twice; do it just once.
> This also saves a reallocation.
>
> This commit fixes the issue behind Coverity issue #1452636. It might
> also make Coverity realize that the issue has been fixed.
>
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt at gmail.com>
> ---
Sorry for the delay, there was a detail that bugged me, I had to look at
it carefully.
> This is not the only thing wrong with the formats API. Will soon send more;
> those who can't wait can already take a look at
> https://github.com/mkver/FFmpeg/commits/avfilter
>
> The most important of these patches is
> https://github.com/mkver/FFmpeg/commit/a151b88f395387c4bb85fbf14c042e2cd3f677ed
>
> libavfilter/formats.c | 41 +++++++++++++++++++++++++++++------------
> 1 file changed, 29 insertions(+), 12 deletions(-)
>
> diff --git a/libavfilter/formats.c b/libavfilter/formats.c
> index 48b27d0c53..ae44006dfe 100644
> --- a/libavfilter/formats.c
> +++ b/libavfilter/formats.c
> @@ -31,19 +31,14 @@
>
> #define KNOWN(l) (!FF_LAYOUT2COUNT(l)) /* for readability */
>
> +
> /**
> * Add all refs from a to ret and destroy a.
> + * ret->refs must have enough spare room left for this.
> */
> -#define MERGE_REF(ret, a, fmts, type, fail) \
> +#define MERGE_REF_NO_ALLOC(ret, a, fmts) \
> do { \
> - type ***tmp; \
> int i; \
> - \
> - if (!(tmp = av_realloc_array(ret->refs, ret->refcount + a->refcount, \
> - sizeof(*tmp)))) \
> - goto fail; \
> - ret->refs = tmp; \
> - \
> for (i = 0; i < a->refcount; i ++) { \
> ret->refs[ret->refcount] = a->refs[i]; \
> *ret->refs[ret->refcount++] = ret; \
> @@ -54,6 +49,17 @@ do { \
> av_freep(&a); \
> } while (0)
>
> +#define MERGE_REF(ret, a, fmts, type, fail) \
> +do { \
> + type ***tmp; \
> + \
> + if (!(tmp = av_realloc_array(ret->refs, ret->refcount + a->refcount, \
> + sizeof(*tmp)))) \
> + goto fail; \
> + ret->refs = tmp; \
> + MERGE_REF_NO_ALLOC(ret, a, fmts); \
> +} while (0)
> +
> /**
> * Add all formats common for a and b to ret, copy the refs and destroy
> * a and b.
> @@ -61,6 +67,7 @@ do { \
> #define MERGE_FORMATS(ret, a, b, fmts, nb, type, fail) \
> do { \
> int i, j, k = 0, count = FFMIN(a->nb, b->nb); \
> + type ***tmp; \
> \
> if (!(ret = av_mallocz(sizeof(*ret)))) \
> goto fail; \
> @@ -85,8 +92,13 @@ do {
> if (!ret->nb) \
> goto fail; \
> \
> - MERGE_REF(ret, a, fmts, type, fail); \
> - MERGE_REF(ret, b, fmts, type, fail); \
> + tmp = av_realloc_array(NULL, a->refcount + b->refcount, sizeof(*tmp)); \
I was worried about the NULL here, because it means allocating a new
array, even if the memory we already have fits. But it is no matter,
because the first call to MERGE_REF is done with ret->refs being NULL.
So patch ok, good catch.
Possibly: add a comment:
/* TODO see if we can reallocate a->refs, or the larger of a->refs and
b->refs instead of allocating a brand new array.
*/
> + if (!tmp) \
> + goto fail; \
> + ret->refs = tmp; \
> + \
> + MERGE_REF_NO_ALLOC(ret, a, fmts); \
> + MERGE_REF_NO_ALLOC(ret, b, fmts); \
> } while (0)
>
> AVFilterFormats *ff_merge_formats(AVFilterFormats *a, AVFilterFormats *b,
> @@ -238,8 +250,13 @@ AVFilterChannelLayouts *ff_merge_channel_layouts(AVFilterChannelLayouts *a,
> ret->nb_channel_layouts = ret_nb;
> if (!ret->nb_channel_layouts)
> goto fail;
> - MERGE_REF(ret, a, channel_layouts, AVFilterChannelLayouts, fail);
> - MERGE_REF(ret, b, channel_layouts, AVFilterChannelLayouts, fail);
> +
> + ret->refs = av_realloc_array(NULL, a->refcount + b->refcount,
> + sizeof(*ret->refs));
> + if (!ret->refs)
> + goto fail;
> + MERGE_REF_NO_ALLOC(ret, a, channel_layouts);
> + MERGE_REF_NO_ALLOC(ret, b, channel_layouts);
> return ret;
>
> fail:
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/20200811/af46146b/attachment.sig>
More information about the ffmpeg-devel
mailing list