[FFmpeg-devel] [PATCH] lavc/vvc: Error pps_single_slice_per_subpic_flag

[Frank Plowman]

On 03/02/2024 16:56, Nuo Mi wrote:
> On Sat, Feb 3, 2024 at 11:51 PM Frank Plowman <post at frankplowman.com> wrote:
> 
>> On 03/02/2024 15:46, Nuo Mi wrote:
>>> On Sat, Feb 3, 2024 at 9:54 PM Frank Plowman <post at frankplowman.com>
>> wrote:
>>>
>>>> On 02/02/2024 14:39, Nuo Mi wrote:
>>>>> On Thu, Feb 1, 2024 at 10:01 PM <post at frankplowman.com> wrote:
>>>>>
>>>>>> From: Frank Plowman <post at frankplowman.com>
>>>>>>
>>>>>> pps_single_slice_per_subpic_flag is not yet supported.  Support is
>> WIP,
>>>>>> but in the meantime throw an error when trying to decode a bitstream
>>>>>> with it set, avoiding an out-of-bounds array access.
>>>>>>
>>>>>> Fixes: out-of-bounds array access for conformance bitstreams
>>>>>> SUBPIC_C_ERICSSON_1, SUBPIC_D_ERICSSON_1, MNUT_A_Nokia_4 and
>>>>>> MNUT_B_Nokia_3.
>>>>>>
>>>>>> Signed-off-by: Frank Plowman <post at frankplowman.com>
>>>>>> ---
>>>>>>     libavcodec/vvc/vvc_ps.c | 21 ++++++++++++++++-----
>>>>>>     1 file changed, 16 insertions(+), 5 deletions(-)
>>>>>>
>>>>>> diff --git a/libavcodec/vvc/vvc_ps.c b/libavcodec/vvc/vvc_ps.c
>>>>>> index 2cf156b323..bd81d70e71 100644
>>>>>> --- a/libavcodec/vvc/vvc_ps.c
>>>>>> +++ b/libavcodec/vvc/vvc_ps.c
>>>>>> @@ -381,11 +381,16 @@ static void pps_multi_tiles_slice(VVCPPS *pps,
>>>> const
>>>>>> int tile_idx, const int i,
>>>>>>         }
>>>>>>     }
>>>>>>
>>>>>> -static void pps_rect_slice(VVCPPS* pps)
>>>>>> +static int pps_rect_slice(VVCPPS* pps)
>>>>>>     {
>>>>>>         const H266RawPPS* r = pps->r;
>>>>>>         int tile_idx = 0, off = 0;
>>>>>>
>>>>>> +    if (r->pps_single_slice_per_subpic_flag) {
>>>>>> +        avpriv_report_missing_feature(NULL,
>>>>>> "pps_single_slice_per_subpic_flag");
>>>>>> +        return AVERROR_PATCHWELCOME;
>>>>>> +    }
>>>>>> +
>>>>>>         for (int i = 0; i < r->pps_num_slices_in_pic_minus1 + 1; i++) {
>>>>>>             if (!r->pps_slice_width_in_tiles_minus1[i] &&
>>>>>>                 !r->pps_slice_height_in_tiles_minus1[i]) {
>>>>>> @@ -396,9 +401,11 @@ static void pps_rect_slice(VVCPPS* pps)
>>>>>>             }
>>>>>>             tile_idx = next_tile_idx(tile_idx, i, r);
>>>>>>         }
>>>>>> +
>>>>>> +    return 0;
>>>>>>     }
>>>>>>
>>>>>> -static void pps_no_rect_slice(VVCPPS* pps)
>>>>>> +static int pps_no_rect_slice(VVCPPS* pps)
>>>>>>     {
>>>>>>         const H266RawPPS* r = pps->r;
>>>>>>         int ctu_x, ctu_y, off = 0;
>>>>>> @@ -409,20 +416,24 @@ static void pps_no_rect_slice(VVCPPS* pps)
>>>>>>                 pps_add_ctus(pps, &off, ctu_x, ctu_y,
>>>>>> r->col_width_val[tile_x], r->row_height_val[tile_y]);
>>>>>>             }
>>>>>>         }
>>>>>> +
>>>>>> +    return 0;
>>>>>>     }
>>>>>>
>>>>>>     static int pps_slice_map(VVCPPS *pps)
>>>>>>     {
>>>>>> +    int ret;
>>>>>> +
>>>>>>         pps->ctb_addr_in_slice = av_calloc(pps->ctb_count,
>>>>>> sizeof(*pps->ctb_addr_in_slice));
>>>>>>         if (!pps->ctb_addr_in_slice)
>>>>>>             return AVERROR(ENOMEM);
>>>>>>
>>>>>>         if (pps->r->pps_rect_slice_flag)
>>>>>> -        pps_rect_slice(pps);
>>>>>> +        ret = pps_rect_slice(pps);
>>>>>>         else
>>>>>> -        pps_no_rect_slice(pps);
>>>>>> +        ret = pps_no_rect_slice(pps);
>>>>>>
>>>>>> -    return 0;
>>>>>> +    return ret;
>>>>>>     }
>>>>>>
>>>>> Thank you Frank. This changed  too much code.
>>>>> How about we only check the sps_num_subpics_minus1 in decode_sps.
>>>>
>>>> I wrote it like this so that the avpriv_report_missing_feature is where
>>>> the feature would need to be, helping readability and searchability.
>>>
>>> We need to make changes to both the cbs and the decoder for subpic
>> support.
>>> pps_slice_map is not the first place.
>>
>> There is nothing strictly missing in the CBS, only the derivation of
>> NumSlicesInSub needs to be moved which is quite subtle;  I think the
>> putting the error in the parameter set parser is clearer.
>>
>> How is the patch below as an alternative?
>>
> This fixes the single_slice_per_subpic_flag.
> But fuzzer may find another subpic-related issue. Highly possible they will
> crash too. :)
> check sub picture number is a safer way

This issue can cause a crash even with the minimum 
{s,p}ps_num_subpics_minus1 = 0 I believe, so this check is needed 
regardless.  We can add a PATCHWELCOME error if
{s,p}ps_num_subpics_minus1 > 0, but this should be a separate commit.