[FFmpeg-devel] [PATCH] avcodec/cbs: Allocate more CodedBitstreamUnit at once in cbs_insert_unit()
Michael Niedermayer
michael at niedermayer.cc
Sat Apr 11 13:18:11 EEST 2020
On Sat, Apr 11, 2020 at 12:29:37AM -0300, James Almer wrote:
> On 4/10/2020 11:49 PM, James Almer wrote:
> > On 4/10/2020 9:00 PM, James Almer wrote:
> >> On 4/10/2020 8:53 PM, Michael Niedermayer wrote:
> >>> On Fri, Apr 10, 2020 at 05:44:25PM -0300, James Almer wrote:
> >>>> On 4/10/2020 5:23 PM, Michael Niedermayer wrote:
> >>>>> Fixes: Timeout (85sec -> 0.5sec)
> >>>>> Fixes: 20791/clusterfuzz-testcase-minimized-ffmpeg_BSF_AV1_FRAME_SPLIT_fuzzer-5659537719951360
> >>>>> Fixes: 21214/clusterfuzz-testcase-minimized-ffmpeg_BSF_MPEG2_METADATA_fuzzer-5165560875974656
> >>>>> Fixes: 21247/clusterfuzz-testcase-minimized-ffmpeg_BSF_H264_METADATA_fuzzer-5715175257931776
> >>>>>
> >>>>> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> >>>>> Signed-off-by: Michael Niedermayer <michael at niedermayer.cc>
> >>>>> ---
> >>>>> libavcodec/cbs.c | 4 ++--
> >>>>> 1 file changed, 2 insertions(+), 2 deletions(-)
> >>>>>
> >>>>> diff --git a/libavcodec/cbs.c b/libavcodec/cbs.c
> >>>>> index 0bd5e1ac5d..42cb9711fa 100644
> >>>>> --- a/libavcodec/cbs.c
> >>>>> +++ b/libavcodec/cbs.c
> >>>>> @@ -693,11 +693,11 @@ static int cbs_insert_unit(CodedBitstreamContext *ctx,
> >>>>> memmove(units + position + 1, units + position,
> >>>>> (frag->nb_units - position) * sizeof(*units));
> >>>>> } else {
> >>>>> - units = av_malloc_array(frag->nb_units + 1, sizeof(*units));
> >>>>> + units = av_malloc_array(frag->nb_units*2 + 1, sizeof(*units));
> >>>>> if (!units)
> >>>>> return AVERROR(ENOMEM);
> >>>>>
> >>>>> - ++frag->nb_units_allocated;
> >>>>> + frag->nb_units_allocated = 2*frag->nb_units_allocated + 1;
> >>>>
> >>>> Use ff_fast_malloc(), please. This is quite ugly and the *2 undocumented
> >>>> and not obvious.
> >>>
> >>> not sure i understood your suggestion correctly but
> >>> ff_fast_malloc() deallocates its buffer and clears the size on error,
> >>> which cbs_insert_unit() does not do.
> >>> so using ff_fast_malloc() feels a bit like fitting a square peg in a round
> >>> hole. But it works too
> >>> is that what you had in mind ? (your comment sounded like something simpler ...)
> >>> so maybe iam missing something
> >>
> >> No, after thinking about it i realized it was not the best option and i
> >> sent a follow up reply about it, but i guess it was too late. If you
> >> have to change the implementation of ff_fast_malloc() then it's clearly
> >> not what should be used here. I didn't intend you to do this much work.
> >>
> >> av_fast_realloc() could work instead as i mentioned in the follow up
> >> reply, i think. Or porting this to AVTreeNode instead of a flat array,
> >> but that's also quite a bit of work and will still allocate one node per
> >> call, so your fuzzer cases may still timeout. So if av_fast_realloc() is
> >> also not an option then maybe increase the buffer by a
> >> small-but-bigger-than-1 amount of units instead of duplicating its size
> >> each call, which can get quite big pretty fast.
> >
> > Here's a version using av_fast_realloc(). FATE passes. Does it solve the
> > timeout?
> >
> > diff --git a/libavcodec/cbs.c b/libavcodec/cbs.c
> > index 0bd5e1ac5d..d6cb94589f 100644
> > --- a/libavcodec/cbs.c
> > +++ b/libavcodec/cbs.c
> > @@ -161,6 +161,7 @@ void ff_cbs_fragment_free(CodedBitstreamContext *ctx,
> >
> > av_freep(&frag->units);
> > frag->nb_units_allocated = 0;
> > + frag->unit_buffer_size = 0;
> > }
> >
> > static int cbs_read_fragment_content(CodedBitstreamContext *ctx,
> > @@ -684,35 +685,29 @@ static int cbs_insert_unit(CodedBitstreamContext *ctx,
> > CodedBitstreamFragment *frag,
> > int position)
> > {
> > - CodedBitstreamUnit *units;
> > + CodedBitstreamUnit *units = frag->units;
> >
> > - if (frag->nb_units < frag->nb_units_allocated) {
> > - units = frag->units;
> > + if (frag->nb_units >= frag->nb_units_allocated) {
> > + size_t new_size;
> > + void *tmp;
> >
> > - if (position < frag->nb_units)
> > - memmove(units + position + 1, units + position,
> > - (frag->nb_units - position) * sizeof(*units));
> > - } else {
> > - units = av_malloc_array(frag->nb_units + 1, sizeof(*units));
> > - if (!units)
> > + if (av_size_mult(frag->nb_units + 1, sizeof(*units), &new_size))
> > return AVERROR(ENOMEM);
> >
> > - ++frag->nb_units_allocated;
> > -
> > - if (position > 0)
> > - memcpy(units, frag->units, position * sizeof(*units));
> > + tmp = av_fast_realloc(units, &frag->unit_buffer_size,
> > + new_size * sizeof(*units));
>
> Should be new_size alone, sorry. av_size_mult() already stored the
> result of this calculation in there.
>
> Also, if you can't apply this diff because my mail client mangled it, i
> can re-send it as a proper patch.
>
> > + if (!tmp)
> > + return AVERROR(ENOMEM);
> >
> > - if (position < frag->nb_units)
> > - memcpy(units + position + 1, frag->units + position,
> > - (frag->nb_units - position) * sizeof(*units));
> > + frag->units = units = tmp;
> > + ++frag->nb_units_allocated;
> > }
> >
> > - memset(units + position, 0, sizeof(*units));
> > + if (position < frag->nb_units)
> > + memmove(units + position + 1, units + position,
> > + (frag->nb_units - position) * sizeof(*units));
> >
> > - if (units != frag->units) {
> > - av_free(frag->units);
> > - frag->units = units;
> > - }
> > + memset(units + position, 0, sizeof(*units));
> >
> > ++frag->nb_units;
> >
> > diff --git a/libavcodec/cbs.h b/libavcodec/cbs.h
> > index 9ca1fbd609..4833a8a3db 100644
> > --- a/libavcodec/cbs.h
> > +++ b/libavcodec/cbs.h
> > @@ -153,6 +153,13 @@ typedef struct CodedBitstreamFragment {
> > */
> > int nb_units_allocated;
> >
> > + /**
> > + * Size of allocated unit buffer.
Iam not sure if "Number of allocated units." and "Size of allocated unit buffer."
are clear descriptions to seperate them, also iam not sure why you need
2 variables
i guess unit_buffer_size is there to just be there for the av_fast_realloc API
but wouldnt it be more efficient to have just one ?
the patch fixes the timeout issue like all other variants proposed so far
one remaining thing that came to my mind is that *realloc has
less strict alignment so if future use of the array intends to place
SIMD data in it that may cause issues on some platforms
probably doesnt matter for this but doesnt hurt to mention it i guess
thx
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
Rewriting code that is poorly written but fully understood is good.
Rewriting code that one doesnt understand is a sign that one is less smart
then the original author, trying to rewrite it will not make it better.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20200411/f1fcd932/attachment.sig>
More information about the ffmpeg-devel
mailing list