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

[James Almer]

On 10/9/2020 7:41 PM, Andreas Rheinhardt wrote:
> 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.
>>
> 
> How about the following approach which mimics 03b82b3ab: Just like the
> RBSP buffer the skipped_bytes_pos buffer is owned by an H2645Packet and
> shared between its NALs; each NAL only retains the index of the first
> entry in the list that belongs to it and the amount of entries of the
> list that belong to it. H2645Packet gets a struct equivalent to
> H2645RBSP for it and ff_h2645_extract_rbsp() gets a pointer to this as
> new parameter; if it is NULL, it means that the caller does not want to
> know the position of the removed 0x03. If not, the buffer is reallocated
> as needed (with overallocating, of course).
> 
> decode_nal_unit() as well as hls_slice_data_wpp() in hevcdec.c would
> need to be adapted to accept the skipped_bytes_pos buffer via other
> newly added function parameters.

Patch welcome. That seems overdesigned to maybe save some memory and I'm
not interested in implementing it.

> 
> - Andreas
> 
> PS: Orthogonally to this, one could add a flag to
> ff_h2645_packet_split() to disable skipped_bytes_pos altogether.

ff_h2645_packet_split() already has two optional functionality
arguments, so a third will make the signature really bloated.
Maybe they can all be replaced by a single flags argument, and use
constants to request small_padding, use_ref and this new one.