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

[Nuo Mi]

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

>
> diff --git a/libavcodec/vvc/vvc_ps.c b/libavcodec/vvc/vvc_ps.c
> index 2cf156b323..4ef8f9f9b9 100644
> --- a/libavcodec/vvc/vvc_ps.c
> +++ b/libavcodec/vvc/vvc_ps.c
> @@ -413,13 +413,20 @@ static void pps_no_rect_slice(VVCPPS* pps)
>
>   static int pps_slice_map(VVCPPS *pps)
>   {
> +    const H266RawPPS* r = pps->r;
> +
>       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)
> +    if (pps->r->pps_rect_slice_flag) {
> +        if (r->pps_single_slice_per_subpic_flag) {
> +            avpriv_report_missing_feature(NULL,
> "pps_single_slice_per_subpic_flag");
> +            return AVERROR_PATCHWELCOME;
> +        }
> +
>           pps_rect_slice(pps);
> -    else
> +    } else
>           pps_no_rect_slice(pps);
>
>       return 0;
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
>