[FFmpeg-devel] [PATCH 2/2] avcodec/h2645_parse: Check HEVC NAL size

[Andreas Rheinhardt]

Andreas Rheinhardt:
> Michael Niedermayer:
>> Fixes: Assertion failure
>> Fixes: 46662/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_HEVC_fuzzer-4947860854013952
>>
>> This also results in more frames to be decoded from fate samples
>>
>> 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 +-
>>  .../ref/fate/hevc-conformance-NoOutPrior_A_Qualcomm_1  | 10 ++++++++++
>>  tests/ref/fate/hevc-conformance-RAP_B_Bossen_1         |  3 +++
>>  3 files changed, 14 insertions(+), 1 deletion(-)
>>
>> diff --git a/libavcodec/h2645_parse.c b/libavcodec/h2645_parse.c
>> index 03780680c6..78ab22b76e 100644
>> --- a/libavcodec/h2645_parse.c
>> +++ b/libavcodec/h2645_parse.c
>> @@ -292,7 +292,7 @@ static int hevc_parse_nal_header(H2645NAL *nal, void *logctx)
>>  {
>>      GetBitContext *gb = &nal->gb;
>>  
>> -    if (get_bits1(gb) != 0)
>> +    if (get_bits_left(gb) < 16 || get_bits1(gb) != 0)
>>          return AVERROR_INVALIDDATA;
>>  
>>      nal->type = get_bits(gb, 6);
>> diff --git a/tests/ref/fate/hevc-conformance-NoOutPrior_A_Qualcomm_1 b/tests/ref/fate/hevc-conformance-NoOutPrior_A_Qualcomm_1
>> index 0c930f6556..3283925e38 100644
>> --- a/tests/ref/fate/hevc-conformance-NoOutPrior_A_Qualcomm_1
>> +++ b/tests/ref/fate/hevc-conformance-NoOutPrior_A_Qualcomm_1
>> @@ -25,6 +25,16 @@
>>  0,         19,         19,        1,   599040, 0x4227009b
>>  0,         20,         20,        1,   599040, 0x1bda8be4
>>  0,         21,         21,        1,   599040, 0xd1d5dcb4
>> +0,         22,         22,        1,   599040, 0x58b2edb3
>> +0,         23,         23,        1,   599040, 0xd1f795d8
>> +0,         24,         24,        1,   599040, 0x3331d5e6
>> +0,         25,         25,        1,   599040, 0x5e5ec2c9
>> +0,         26,         26,        1,   599040, 0x3b907bf5
>> +0,         27,         27,        1,   599040, 0xefcbf471
>> +0,         28,         28,        1,   599040, 0x2769a578
>> +0,         29,         29,        1,   599040, 0x812ce986
>> +0,         30,         30,        1,   599040, 0xf07c212c
>> +0,         31,         31,        1,   599040, 0xb5476890
>>  0,         32,         32,        1,   599040, 0x00a0249f
>>  0,         33,         33,        1,   599040, 0x7263f7cf
>>  0,         34,         34,        1,   599040, 0x47054be4
>> diff --git a/tests/ref/fate/hevc-conformance-RAP_B_Bossen_1 b/tests/ref/fate/hevc-conformance-RAP_B_Bossen_1
>> index e661ff245e..776267b59c 100644
>> --- a/tests/ref/fate/hevc-conformance-RAP_B_Bossen_1
>> +++ b/tests/ref/fate/hevc-conformance-RAP_B_Bossen_1
>> @@ -70,6 +70,9 @@
>>  0,         64,         64,        1,   149760, 0x3362678b
>>  0,         65,         65,        1,   149760, 0x6e7fc851
>>  0,         66,         66,        1,   149760, 0x33f96449
>> +0,         67,         67,        1,   149760, 0xd9d05007
>> +0,         75,         75,        1,   149760, 0x477f2cf2
>> +0,         76,         76,        1,   149760, 0xe1f9ccd0
>>  0,         77,         77,        1,   149760, 0xb3ba8cfb
>>  0,         78,         78,        1,   149760, 0x64787995
>>  0,         79,         79,        1,   149760, 0xc10de4c4
> 
> get_bit_length currently presumes every NALU to contain
> rbsp_trailing_bits. Yet this is not true for the End of
> Sequence/Bitstream units which are just headers without RBSP. For these
> units, get_bit_length might truncate them -- it does so for end of
> sequence units in H.264. It would not be a serious issue for H.265, as
> the semantics of nuh_temporal_id_plus1 require nuh_temporal_id_plus1 to
> be 1 for End of Sequence/Bitstream units. Nevertheless I think this
> should be coupled with a patch that does not truncate the NAL unit if it
> is just a header.
> 

1. I just sent a patch implementing the above:
https://ffmpeg.org/pipermail/ffmpeg-devel/2022-June/297923.html
Please confirm that it actually fixes the testcase its commit message
claims to fix.
2. The RAP_B_Bossen_1 and NoOutPrior_A_Qualcomm_1 (where the testcases
change due to your patch) contain completely fine end of sequence NALUs.
Because they are valid, stripping them (as your patch does) is not ok
(e.g. these units would even be discarded when using hevc_metadata).
There are two bugs with these units:
a) Our parser puts them at the beginning of their NALUs, yet they should
be at the end of the (preceding) NALU.
b) When remuxing the samples to Matroska with mkvmerge (which puts these
units at the end of their packets), the output is the same as with raw
input, i.e. decoder still misses some frames. So somehow these units
confuse the decoder.

- Andreas