[FFmpeg-devel] [PATCH 2/3] avcodec/cbs_h266_syntax_template: sanity check num_multi_layer_olss

[James Almer]

On 1/29/2024 7:28 PM, Frank Plowman wrote:
> 
> On 29/01/2024 21:13, James Almer wrote:
>> On 1/29/2024 5:19 PM, Frank Plowman wrote:
>>> Below is a patch which addresses the issue, an integer overflow when 
>>> calculating the bounds for
>>> vps_num_ols_timing_hrd_params_minus1.  There's also a similar fix for 
>>> vps_num_dpb_params_minus1.
>>>
>>> diff --git a/libavcodec/cbs_h266_syntax_template.c 
>>> b/libavcodec/cbs_h266_syntax_template.c
>>> index 549d021211..49bf2e45ac 100644
>>> --- a/libavcodec/cbs_h266_syntax_template.c
>>> +++ b/libavcodec/cbs_h266_syntax_template.c
>>> @@ -946,7 +946,8 @@ static int FUNC(vps) (CodedBitstreamContext *ctx, 
>>> RWContext *rw,
>>>
>>>       if (!current->vps_each_layer_is_an_ols_flag) {
>>>           uint16_t vps_num_dpb_params;
>>> -        ue(vps_num_dpb_params_minus1, 0, num_multi_layer_olss - 1);
>>> +        ue(vps_num_dpb_params_minus1, 0,
>>> +           num_multi_layer_olss > 0 ? num_multi_layer_olss - 1 : 0);
>>
>> FFMAX(0, num_multi_layer_olss - 1); looks better.
>>
>> If the spec explicitly states num_multi_layer_olss - 1 should be used 
>> here, wouldn't that mean that it doesn't expect num_multi_layer_olss 
>> to be 0 at this point for vps_ols_mode_idc >= 0 && vps_ols_mode_idc < 3?
>> When vps_each_layer_is_an_ols_flag is true, num_multi_layer_olss is 
>> inferred as 1
> 
> num_multi_layer_olss is 0 if vps_each_layer_is_an_ols_flag is true, it 
> is num_layers_in_ols which must be 1.

You're right, i read it wrong. These names are all too similar :p

> 
>> so I'd expect it to also be at least 1 for 
>> vps_each_layer_is_an_ols_flag == false.
> 
> Yes I think you're right.  The spec isn't especially clear, but the 
> description of the vps_each_layer_is_an_ols_flag leads me to believe we 
> are safe to assert that.  Patch which does so below.
> 
> diff --git a/libavcodec/cbs_h266_syntax_template.c 
> b/libavcodec/cbs_h266_syntax_template.c
> index 549d021211..5ae3fb9f08 100644
> --- a/libavcodec/cbs_h266_syntax_template.c
> +++ b/libavcodec/cbs_h266_syntax_template.c
> @@ -912,6 +912,8 @@ static int FUNC(vps) (CodedBitstreamContext *ctx, 
> RWContext *rw,
>                   num_multi_layer_olss++;
>               }
>           }
> +        if (!current->vps_each_layer_is_an_ols_flag && 
> num_multi_layer_olss == 0)
> +            return AVERROR_INVALIDDATA;
>       }

LGTM, can you send a patch for it?