| Message ID | 20240201140055.63805-1-post@frankplowman.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel] lavc/vvc: Error pps_single_slice_per_subpic_flag | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/make_loongarch64 | success | Make finished |
| yinshiyou/make_fate_loongarch64 | success | Make fate finished |
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
On Thu, Feb 1, 2024 at 10:01 PM <post@frankplowman.com> wrote: > From: Frank Plowman <post@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@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. > > static void pps_ref_wraparound_offset(VVCPPS *pps, const VVCSPS *sps) > -- > 2.43.0 > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". >
On 02/02/2024 14:39, Nuo Mi wrote: > On Thu, Feb 1, 2024 at 10:01 PM <post@frankplowman.com> wrote: > >> From: Frank Plowman <post@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@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. I could remove the return from pps_rect_slice and pps_no_rect_slice which would get rid of a handful of changed lines but the changes are trivial so it is not a big deal imo.
On Sat, Feb 3, 2024 at 9:54 PM Frank Plowman <post@frankplowman.com> wrote: > On 02/02/2024 14:39, Nuo Mi wrote: > > On Thu, Feb 1, 2024 at 10:01 PM <post@frankplowman.com> wrote: > > > >> From: Frank Plowman <post@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@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. > I > could remove the return from pps_rect_slice and pps_no_rect_slice which > would get rid of a handful of changed lines but the changes are trivial > so it is not a big deal imo. > Once we implemented subpic, both pps_rect_slice and pps_no_rect_slice are not supposed to return a value. We need to change it back > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". >
On 03/02/2024 15:46, Nuo Mi wrote: > On Sat, Feb 3, 2024 at 9:54 PM Frank Plowman <post@frankplowman.com> wrote: > >> On 02/02/2024 14:39, Nuo Mi wrote: >>> On Thu, Feb 1, 2024 at 10:01 PM <post@frankplowman.com> wrote: >>> >>>> From: Frank Plowman <post@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@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? 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;
On Sat, Feb 3, 2024 at 11:51 PM Frank Plowman <post@frankplowman.com> wrote: > On 03/02/2024 15:46, Nuo Mi wrote: > > On Sat, Feb 3, 2024 at 9:54 PM Frank Plowman <post@frankplowman.com> > wrote: > > > >> On 02/02/2024 14:39, Nuo Mi wrote: > >>> On Thu, Feb 1, 2024 at 10:01 PM <post@frankplowman.com> wrote: > >>> > >>>> From: Frank Plowman <post@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@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@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". >
On 03/02/2024 16:56, Nuo Mi wrote: > On Sat, Feb 3, 2024 at 11:51 PM Frank Plowman <post@frankplowman.com> wrote: > >> On 03/02/2024 15:46, Nuo Mi wrote: >>> On Sat, Feb 3, 2024 at 9:54 PM Frank Plowman <post@frankplowman.com> >> wrote: >>> >>>> On 02/02/2024 14:39, Nuo Mi wrote: >>>>> On Thu, Feb 1, 2024 at 10:01 PM <post@frankplowman.com> wrote: >>>>> >>>>>> From: Frank Plowman <post@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@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.
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; } static void pps_ref_wraparound_offset(VVCPPS *pps, const VVCSPS *sps)