[FFmpeg-devel] [PATCH 1/3] avcodec/h2645_parse: Limit initial skipped_bytes_pos_size to nal size / 16

[James Almer]

On 10/4/2020 6:02 PM, James Almer wrote:
> On 10/4/2020 5:57 PM, Michael Niedermayer wrote:
>> On Sun, Oct 04, 2020 at 05:04:05PM -0300, James Almer wrote:
>>> On 10/4/2020 4:41 PM, Michael Niedermayer wrote:
>>>> Fixes: OOM
>>>> Fixes: 23817/clusterfuzz-testcase-minimized-ffmpeg_BSF_H264_METADATA_fuzzer-6300869057576960
>>>>
>>>> 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/h2645_parse.c | 2 +-
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/libavcodec/h2645_parse.c b/libavcodec/h2645_parse.c
>>>> index 0f98b49fbe..61105a6eb5 100644
>>>> --- a/libavcodec/h2645_parse.c
>>>> +++ b/libavcodec/h2645_parse.c
>>>> @@ -467,7 +467,7 @@ int ff_h2645_packet_split(H2645Packet *pkt, const uint8_t *buf, int length,
>>>>              memset(pkt->nals + pkt->nals_allocated, 0, sizeof(*pkt->nals));
>>>>  
>>>>              nal = &pkt->nals[pkt->nb_nals];
>>>> -            nal->skipped_bytes_pos_size = 1024; // initial buffer size
>>>> +            nal->skipped_bytes_pos_size = FFMIN(1024, 1+(extract_length>>4)); // initial buffer size
>>>
>>> Why is there even an initial buffer? Why not just let
>>> ff_h2645_extract_rbsp() allocate it when needed?
>>
>> i wondered that too and assumed it was done that way to avoid spending
>> cpu cycles on reallocations later
> 
> Many streams don't need to escape bytes, so for those, allocating
> anything at all is a waste. And IMO by using av_fast_realloc() in
> ff_h2645_extract_rbsp() there's no need for a big enough initial buffer
> either.

On second thought, even if most av_fast_realloc() calls will be no-ops,
they may end up being way too many to the point the current behavior
would be more efficient.

Does moving the allocation of the initial buffer to
ff_h2645_extract_rbsp() also help with this sample? I assume it's one
where it generates an absurd amount of NAL units in the packet, but most
would probably be small enough that they will not really contain bytes
that need to be escaped, and as such not require a skipped_bytes_pos buffer.

Something like

> diff --git a/libavcodec/h2645_parse.c b/libavcodec/h2645_parse.c
> index 0f98b49fbe..3e59fcaa5b 100644
> --- a/libavcodec/h2645_parse.c
> +++ b/libavcodec/h2645_parse.c
> @@ -108,9 +108,10 @@ int ff_h2645_extract_rbsp(const uint8_t *src, int length,
>                  dst[di++] = 0;
>                  si       += 3;
> 
> -                if (nal->skipped_bytes_pos) {
>                      nal->skipped_bytes++;
>                      if (nal->skipped_bytes_pos_size < nal->skipped_bytes) {
> +                        if (!nal->skipped_bytes_pos_size)
> +                            nal->skipped_bytes_pos_size = 512;
>                          nal->skipped_bytes_pos_size *= 2;
>                          av_assert0(nal->skipped_bytes_pos_size >= nal->skipped_bytes);
>                          av_reallocp_array(&nal->skipped_bytes_pos,
> @@ -123,7 +124,7 @@ int ff_h2645_extract_rbsp(const uint8_t *src, int length,
>                      }
>                      if (nal->skipped_bytes_pos)
>                          nal->skipped_bytes_pos[nal->skipped_bytes-1] = di - 1;
> -                }
> +
>                  continue;
>              } else // next start code
>                  goto nsc;
> @@ -466,12 +467,6 @@ int ff_h2645_packet_split(H2645Packet *pkt, const uint8_t *buf, int length,
>              pkt->nals = tmp;
>              memset(pkt->nals + pkt->nals_allocated, 0, sizeof(*pkt->nals));
> 
> -            nal = &pkt->nals[pkt->nb_nals];
> -            nal->skipped_bytes_pos_size = 1024; // initial buffer size
> -            nal->skipped_bytes_pos = av_malloc_array(nal->skipped_bytes_pos_size, sizeof(*nal->skipped_bytes_pos));
> -            if (!nal->skipped_bytes_pos)
> -                return AVERROR(ENOMEM);
> -
>              pkt->nals_allocated = new_size;
>          }
>          nal = &pkt->nals[pkt->nb_nals];