[FFmpeg-devel] [PATCH] avcodec/cbs: Allocate more CodedBitstreamUnit at once in cbs_insert_unit()

[Andreas Rheinhardt]

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].

> 
> 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
> 
This array is clearly not intended to be directly used with any SIMD stuff.

- Andreas

[1]: http://ffmpeg.org/pipermail/ffmpeg-devel/2020-January/255182.html