[FFmpeg-devel] [PATCH] avcodec/h2645_parse: remove initial skipped_bytes_pos buffer

[James Almer]

On 10/9/2020 6:16 PM, Andreas Rheinhardt wrote:
> James Almer:
>> On 10/9/2020 1:36 PM, Michael Niedermayer wrote:
>>> On Thu, Oct 08, 2020 at 10:25:11AM -0300, James Almer wrote:
>>>> Allocate it only when needed, and instead of giving it a fixed initial size
>>>> that's doubled on each realloc, ensure it's always big enough for the NAL
>>>> currently being parsed.
>>>>
>>>> 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: James Almer <jamrial at gmail.com>
>>>> ---
>>>>  libavcodec/h2645_parse.c | 28 ++++++++++------------------
>>>>  1 file changed, 10 insertions(+), 18 deletions(-)
>>>>
>>>> diff --git a/libavcodec/h2645_parse.c b/libavcodec/h2645_parse.c
>>>> index 0f98b49fbe..f5c76323c1 100644
>>>> --- a/libavcodec/h2645_parse.c
>>>> +++ b/libavcodec/h2645_parse.c
>>>> @@ -108,22 +108,20 @@ 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) {
>>>> -                        nal->skipped_bytes_pos_size *= 2;
>>>> -                        av_assert0(nal->skipped_bytes_pos_size >= nal->skipped_bytes);
>>>> -                        av_reallocp_array(&nal->skipped_bytes_pos,
>>>> +                nal->skipped_bytes++;
>>>> +                if (nal->skipped_bytes_pos_size < nal->skipped_bytes) {
>>>> +                    nal->skipped_bytes_pos_size = length / 3;
>>>
>>> This would allocate a much larger than needed skipped_bytes_pos for 
>>> probably nearly all of the real world h264 files to fix an issue with
>>> crafted streams.
>>>
>>> The initial size should be choosen so as to be large enough for real world
>>> streams. If that turns out to be too small then length /3 sounds reasonable
>>> as the first reallocation.
>>
>> At most 1/3 of the bytes in a NAL would be removed by this code, hence
>> length / 3. I could make it length / 16 like in your fix if you prefer,
>> or maybe nal->skipped_bytes * 2 to keep it closer to the current
>> behavior, but real world samples don't have more than a handful of NALs
>> per packet, and not all have escaped bytes that need to be removed (If
>> there are none, nothing will be allocated).
>>
>> I looked at a 2160p hevc sample, and the biggest packet is about 26kb,
>> which assuming it's a single slice NAL, it will be about 8kb for the
>> skipped_bytes_pos buffer with length / 3.
>>
> Your calculation seems to be off by a factor of four because you ignore
> that every byte removed needs an entry of type int*.

True, forgot each byte position was saved as an int in the array.

> Furthermore 26kB (I
> presume you meant kB and not kb) seems very, very small for a normal
> file these days; it feels more like 320p streaming or so.

Yes, kilobytes. And as i said, it was a 2160p sample where the biggest
packet was 26kB. Maybe it's not the most complex stream out there, i
don't know.

> 
> Furthermore, there is a bigger issue with your patch: It can lead to
> quadratic memory consumption for annex B input: Because the length of
> the NAL units is not known in advance, length in the above code refers
> to the length from the beginning of the NAL unit to the end of the
> packet. So this code will allocate gigantic amounts of memory for a
> packet containing lots of small NAL units, each with a single 0x000003.
> This is an issue similar to the one fixed by
> 03b82b3ab9883cef017e513c7d0b3b986b3b3e7b.

Alright, will use nal->skipped_bytes * 2 then.

> 
> - Andreas
> 
> *: Yes, I know there is no guarantee that sizeof(int) is four.
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
>