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 <mich...@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.
> + *
> + * Must always be >= nb_units_allocated; designed for internal use
> by cbs.
> + */
> + unsigned int unit_buffer_size;
> +
> /**
> * Pointer to an array of units of length nb_units_allocated.
> * Only the first nb_units are valid.
>
_______________________________________________
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".
