[FFmpeg-devel] [PATCH] avcodec/cbs: Allocate more CodedBitstreamUnit at once in cbs_insert_unit()
Michael Niedermayer
michael at niedermayer.cc
Sat Apr 11 16:04:50 EEST 2020
On Sat, Apr 11, 2020 at 12:26:42PM +0200, Andreas Rheinhardt wrote:
> Michael Niedermayer:
> > 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
> >
> The one is in bytes, the other is in number of units.
>
> > 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 ?
>
> Your guess is correct.
>
> I once proposed an av_fast_realloc_array() for this purpose [1].
a av_fast_realloc_array() would be useful
thx
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
If the United States is serious about tackling the national security threats
related to an insecure 5G network, it needs to rethink the extent to which it
values corporate profits and government espionage over security.-Bruce Schneier
-------------- 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/b98997f1/attachment.sig>
More information about the ffmpeg-devel
mailing list