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

[James Almer]

On 1/27/2024 9:05 PM, Michael Niedermayer wrote:
> On Sat, Jan 27, 2024 at 09:02:30PM -0300, James Almer wrote:
>> On 1/27/2024 8:56 PM, Michael Niedermayer wrote:
>>> On Sat, Jan 27, 2024 at 09:25:16AM -0300, James Almer wrote:
>>>> On 1/26/2024 6:46 PM, Michael Niedermayer wrote:
>>>>> It is not possible to encode a index into an empty list. Thus
>>>>> this must be invalid at this point or before.
>>>>> Its likely a broader earlier check can be used here, someone knowing
>>>>> VVC should look at that. Its not immedeatly obvious from the spec
>>>>> by looking for numlayerolss
>>>>
>>>> Can you check if the following fixes it?
>>>>
>>>>> diff --git a/libavcodec/cbs_h266_syntax_template.c b/libavcodec/cbs_h266_syntax_template.c
>>>>> index 549d021211..40572dadb5 100644
>>>>> --- a/libavcodec/cbs_h266_syntax_template.c
>>>>> +++ b/libavcodec/cbs_h266_syntax_template.c
>>>>> @@ -793,6 +793,7 @@ static int FUNC(vps) (CodedBitstreamContext *ctx, RWContext *rw,
>>>>>        {
>>>>>            //calc NumMultiLayerOlss
>>>>>            int m;
>>>>> +        int num_layers_in_ols = 0;
>>>>>            uint8_t dependency_flag[VVC_MAX_LAYERS][VVC_MAX_LAYERS];
>>>>>            uint16_t num_output_layers_in_ols[VVC_MAX_TOTAL_NUM_OLSS];
>>>>>            uint8_t num_sub_layers_in_layer_in_ols[VVC_MAX_TOTAL_NUM_OLSS][VVC_MAX_TOTAL_NUM_OLSS];
>>>>> @@ -895,7 +896,6 @@ static int FUNC(vps) (CodedBitstreamContext *ctx, RWContext *rw,
>>>>>                    return AVERROR_INVALIDDATA;
>>>>>            }
>>>>>            for (i = 1; i < total_num_olss; i++) {
>>>>> -            int num_layers_in_ols = 0;
>>>>>                if (current->vps_each_layer_is_an_ols_flag) {
>>>>>                    num_layers_in_ols = 1;
>>>>>                } else if (current->vps_ols_mode_idc == 0 ||
>>>>
>>>> num_layers_in_ols is not meant to be reset on every loop.
>>>
>>> replacing my patch by yours does not change
>>> num_multi_layer_olss from being 0
>>> and if its 0 then "num_multi_layer_olss - 1" causes problems as a limit
>>>
>>> more precissely this:
>>> i can also send you the file if you want?
>>
>> No, this should be looked at by someone more familiar with VVC.
> 
> ive already sent the fuzzer samples to nuomi and frank plowman
> 
> 
>> And my patch should be applied either way. The current code is wrong.
> 
> I did not suggest not to do that :)

Well, turns out the current code is fine and my suggested change above 
is wrong. Fun how that goes.

Can you test the following instead?

> diff --git a/libavcodec/cbs_h266_syntax_template.c b/libavcodec/cbs_h266_syntax_template.c
> index 549d021211..30b4ae3bc0 100644
> --- a/libavcodec/cbs_h266_syntax_template.c
> +++ b/libavcodec/cbs_h266_syntax_template.c
> @@ -764,7 +764,7 @@ static int FUNC(vps) (CodedBitstreamContext *ctx, RWContext *rw,
>              infer(vps_each_layer_is_an_ols_flag, 0);
>          if (!current->vps_each_layer_is_an_ols_flag) {
>              if (!current->vps_all_independent_layers_flag)
> -                ub(2, vps_ols_mode_idc);
> +                u(2, vps_ols_mode_idc, 0, 2);
>              else
>                  infer(vps_ols_mode_idc, 2);
>              if (current->vps_ols_mode_idc == 2) {
> @@ -902,11 +902,10 @@ static int FUNC(vps) (CodedBitstreamContext *ctx, RWContext *rw,
>                         current->vps_ols_mode_idc == 1) {
>                  num_layers_in_ols = i + 1;
>              } else if (current->vps_ols_mode_idc == 2) {
> -                for (k = 0, j = 0; k <= current->vps_max_layers_minus1; k++) {
> +                for (k = 0, j = 0; k <= current->vps_max_layers_minus1; k++)
>                      if (layer_included_in_ols_flag[i][k])
>                          j++;
> -                    num_layers_in_ols = j;
> -                }
> +                num_layers_in_ols = j;
>              }
>              if (num_layers_in_ols > 1) {
>                  num_multi_layer_olss++;