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 <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. > > 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 _______________________________________________ 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".
