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

[Mark Thompson]

On 20/05/2020 22:16, 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 ...

I don't know how the decoder was written, but the intent in cbs has always been to use exactly the same names as the standard does (even when those names are long or redundant) so that code and variables can be precisely matched for verification.

>>>
>>> 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 ...
> 
> diff --git a/libavcodec/cbs_h265.h b/libavcodec/cbs_h265.h
> index 73897f77a42..9b1895d460e 100644
> --- a/libavcodec/cbs_h265.h
> +++ b/libavcodec/cbs_h265.h
> @@ -473,10 +473,10 @@ typedef struct  H265RawSliceHeader {
>      uint8_t num_long_term_sps;
>      uint8_t num_long_term_pics;
>      uint8_t lt_idx_sps[HEVC_MAX_REFS];
> -    uint8_t poc_lsb_lt[HEVC_MAX_REFS];
> -    uint8_t used_by_curr_pic_lt_flag[HEVC_MAX_REFS];
> -    uint8_t delta_poc_msb_present_flag[HEVC_MAX_REFS];
> -    uint32_t delta_poc_msb_cycle_lt[HEVC_MAX_REFS];
> +    uint8_t poc_lsb_lt[HEVC_MAX_LONG_TERM_REF_PICS];
> +    uint8_t used_by_curr_pic_lt_flag[HEVC_MAX_LONG_TERM_REF_PICS];
> +    uint8_t delta_poc_msb_present_flag[HEVC_MAX_LONG_TERM_REF_PICS];
> +    uint32_t delta_poc_msb_cycle_lt[HEVC_MAX_LONG_TERM_REF_PICS];
>  
>      uint8_t slice_temporal_mvp_enabled_flag;
>  
> @@ -488,9 +488,9 @@ typedef struct  H265RawSliceHeader {
>      uint8_t num_ref_idx_l1_active_minus1;
>  
>      uint8_t ref_pic_list_modification_flag_l0;
> -    uint8_t list_entry_l0[HEVC_MAX_REFS];
> +    uint8_t list_entry_l0[HEVC_MAX_LONG_TERM_REF_PICS];
>      uint8_t ref_pic_list_modification_flag_l1;
> -    uint8_t list_entry_l1[HEVC_MAX_REFS];
> +    uint8_t list_entry_l1[HEVC_MAX_LONG_TERM_REF_PICS];
>  
>      uint8_t mvd_l1_zero_flag;
>      uint8_t cabac_init_flag;
> 
> [...]

No, that doesn't make sense - the stream is invalid if it is trying to refer to more than HEVC_MAX_REFS long term pics, so these extra entries could only be used by invalid streams which are being incorrectly accepted.

Changing the upper bound on num_long_term_pics to be HEVC_MAX_REFS - num_long_term_sps would be sufficient to avoid this problem, though that will admit some invalid streams which also contain short-term or self-reference pics which together exceed the limit.  Enforcing the proper limit as James described is better, but adds more code to do it.

- Mark