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

James Almer jamrial at gmail.com
Fri Oct 9 23:57:22 EEST 2020


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.

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



More information about the ffmpeg-devel mailing list