[FFmpeg-devel] [PATCH 3/4] avcodec/mpegutils: consolidate single byte av_log()

[Andreas Rheinhardt]

Michael Niedermayer:
> On Fri, Sep 03, 2021 at 09:00:22PM +0200, Andreas Rheinhardt wrote:
>> Michael Niedermayer:
>>> Fixes: Timeout (56sec -> 15sec)
>>> Fixes: 37141/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_MPEG4_fuzzer-6192122867875840
>>>
>>> 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/mpegutils.c | 56 ++++++++++++++++++++++++------------------
>>>  1 file changed, 32 insertions(+), 24 deletions(-)
>>>
>>> diff --git a/libavcodec/mpegutils.c b/libavcodec/mpegutils.c
>>> index e5105ecc58..e91c554781 100644
>>> --- a/libavcodec/mpegutils.c
>>> +++ b/libavcodec/mpegutils.c
>>> @@ -187,7 +187,6 @@ void ff_print_debug_info2(AVCodecContext *avctx, AVFrame *pict, uint8_t *mbskip_
>>>  
>>>          av_freep(&mvs);
>>>      }
>>> -
>>>      /* TODO: export all the following to make them accessible for users (and filters) */
>>>      if (avctx->hwaccel || !mbtype_table)
>>>          return;
>>> @@ -195,71 +194,80 @@ void ff_print_debug_info2(AVCodecContext *avctx, AVFrame *pict, uint8_t *mbskip_
>>>  
>>>      if (avctx->debug & (FF_DEBUG_SKIP | FF_DEBUG_QP | FF_DEBUG_MB_TYPE)) {
>>>          int x,y;
>>> +#define MB_STRING_SIZE 6
>>> +        char *mbstring = av_malloc(MB_STRING_SIZE * mb_width + 1);
>>
>> Is it guaranteed that mb_width can't be huge? (I wouldn't be surprised
>> if there were a compile-time bound for it; this could be used for a
>> stack allocation.)
> 
> i had no major problem creating a file with a mb_width above 40k, the failure
> was from allocating the image if the size was increased not anything inherent
> to mb_width
> not every codec and container allows this but some do, i picked 
> msmpeg4v3 in nut but that is not a unique choice at all
> 

Forget what I said above in parentheses: I thought that the size of a
single macroblock is bounded for all codecs using this function. But
mb_width is apparently the width of the picture in macroblocks.

> 
>>
>>> +        if (!mbstring)
>>> +            return;
>>>  
>>>          av_log(avctx, AV_LOG_DEBUG, "New frame, type: %c\n",
>>>                 av_get_picture_type_char(pict->pict_type));
>>>          for (y = 0; y < mb_height; y++) {
>>> +            char *mbs = mbstring;
>>>              for (x = 0; x < mb_width; x++) {
>>> +                av_assert0(mbs - mbstring <= MB_STRING_SIZE * x);
>>>                  if (avctx->debug & FF_DEBUG_SKIP) {
>>>                      int count = mbskip_table ? mbskip_table[x + y * mb_stride] : 0;
>>>                      if (count > 9)
>>>                          count = 9;
>>> -                    av_log(avctx, AV_LOG_DEBUG, "%1d", count);
>>> +                    *mbs++ = '0' + count;
>>>                  }
>>>                  if (avctx->debug & FF_DEBUG_QP) {
>>> -                    av_log(avctx, AV_LOG_DEBUG, "%2d",
>>> -                           qscale_table[x + y * mb_stride]);
>>> +                    int q = qscale_table[x + y * mb_stride];
>>> +                    *mbs++ = '0' + q/10;
>>> +                    *mbs++ = '0' + q%10;
>>
>> This is only equivalent to the old code if the value is in the range
>> 0-99. Is this guaranteed?
> 
> the code prior to this assumed so, if it exceeded it, the output would
> become difficult to read. So if this assumtation is wrong we would need to
> already fix that, Do you know of a case where this is wrong ?
> 
No.

- Andreas