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