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

[James Almer]

On 10/5/2020 6:43 PM, Michael Niedermayer wrote:
> On Mon, Oct 05, 2020 at 01:30:07AM -0300, James Almer wrote:
>> 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.
> 
> your variant below works too, yes

What do you think about setting instead nal->skipped_bytes_pos_size to
length / 3 when it needs to be resized? The max possible amount of bytes
it will skip/strip per NAL is one third of the total NAL size.

It's probably better than my previous suggestion, as it still removes
the initial buffer outside of ff_h2645_extract_rbsp() and also avoids
the unconditional check for nal->skipped_bytes_pos_size == 0 and does
not duplicate the buffer's size on each realloc.

> the fuzzer testcase is a gazzilion of 1byte NAL units IIRC
> 
> thx
> 
> [...]
> 
> 
> _______________________________________________
> 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".
>