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

[Andreas Rheinhardt]

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*. 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.

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.

- Andreas

*: Yes, I know there is no guarantee that sizeof(int) is four.