[FFmpeg-devel] [PATCH 3/3] avcodec/cbs_h265_syntax_template: Limit num_long_term_pics more strictly

[James Almer]

On 5/20/2020 6:16 PM, Michael Niedermayer wrote:
> On Wed, May 20, 2020 at 08:56:29PM +0200, Michael Niedermayer wrote:
>> On Mon, Apr 20, 2020 at 07:34:44PM -0300, James Almer wrote:
>>> On 4/20/2020 7:03 PM, Michael Niedermayer wrote:
>>>> The limit is based on hevcdec.c
>>>> Fixes: 20854/clusterfuzz-testcase-minimized-ffmpeg_BSF_HEVC_METADATA_fuzzer-5160442882424832
>>>> Fixes: out of array access
>>>>
>>>> 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/cbs_h265_syntax_template.c | 2 +-
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/libavcodec/cbs_h265_syntax_template.c b/libavcodec/cbs_h265_syntax_template.c
>>>> index 180a045c34..b74b9648c3 100644
>>>> --- a/libavcodec/cbs_h265_syntax_template.c
>>>> +++ b/libavcodec/cbs_h265_syntax_template.c
>>>> @@ -1367,7 +1367,7 @@ static int FUNC(slice_segment_header)(CodedBitstreamContext *ctx, RWContext *rw,
>>>>                      infer(num_long_term_sps, 0);
>>>>                      idx_size = 0;
>>>>                  }
>>>> -                ue(num_long_term_pics, 0, HEVC_MAX_LONG_TERM_REF_PICS);
>>>> +                ue(num_long_term_pics, 0, FFMIN(HEVC_MAX_LONG_TERM_REF_PICS, FF_ARRAY_ELEMS(current->poc_lsb_lt) - current->num_long_term_sps));
>>>
>>> Maybe poc_lsb_lt should also have HEVC_MAX_LONG_TERM_REF_PICS elems
>>> instead of HEVC_MAX_REFS, same as in the hevc decoder.
>>
>> ok, btw the decoder and cbs use completely unrelated variable names.
>> That should be cleaned up by someone i think ...
>>
>>
>>>
>>> Also the spec defines some specific limit to this field:
>>>
>>> "When nuh_layer_id is equal to 0, the value of num_long_term_pics shall
>>> be less than or equal to sps_max_dec_pic_buffering_minus1[TemporalId] −
>>> NumNegativePics[CurrRpsIdx] − NumPositivePics[CurrRpsIdx] −
>>> num_long_term_sps − TwoVersionsOfCurrDecPicFlag"
>>>
>>> With CurrRpsIdx derived as:
>>> – If short_term_ref_pic_set_sps_flag is equal to 1, CurrRpsIdx is set
>>> equal to short_term_ref_pic_set_idx.
>>> – Otherwise, CurrRpsIdx is set equal to num_short_term_ref_pic_sets.
>>>
>>> And TwoVersionsOfCurrDecPicFlag as:
>>> "TwoVersionsOfCurrDecPicFlag = pps_curr_pic_ref_enabled_flag &&
>>> (sample_adaptive_offset_enabled_flag ||
>>> !pps_deblocking_filter_disabled_flag ||
>>> deblocking_filter_override_enabled_flag)
>>> When sps_max_dec_pic_buffering_minus1[ TemporalId ] is equal to 0, the
>>> value of TwoVersionsOfCurrDecPicFlag shall be equal to 0."
>>>
>>> But i don't know if it's worth adding that many checks.
>>
>> I saw this too when i wrote the original patch, and i remember that
>> it at least felt like these checks would not cover all cases.
>> Maybe ive missed something but if they dont cover all then this would
>> be unrelated as it would neither be sufficient nor helpfull for this
>> bugfix
>>
>> will later apply this with the HEVC_MAX_LONG_TERM_REF_PICS as suggested
>> and backport unless i hear objections before
> 
> tried this, but it seems that the increased size spreads and requires
> other arrays to be enarged too
> and that starts feeling a bit uncomfortable as a easy backportable
> fix
> so are you ok with the original variant and do you see any bugs/problems
> in that ?
> or i can also go for the array enlargement if thats really preferred ?
> just wanted to make sure its understood that enlarging one array alone
> doesnt work on its own ...

This is not my module, so I prefer to leave this kind of decision to
Mark. But keep in mind in the past we have backported array size changes
that fixes issues, like d3fef1a3bd.

I'd say that for now apply your original patch and backport it, unless
Mark comments first. It can always be replaced later.