| Message ID | 20240203172109.20149-1-post@frankplowman.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel] lavc/vvc: Validate alf_list indexes | 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 Sun, Feb 4, 2024 at 1:22 AM <post@frankplowman.com> wrote: > From: Frank Plowman <post@frankplowman.com> > > Fixes crashes when decoding illegal bitstreams found by fuzzing. > > Signed-off-by: Frank Plowman <post@frankplowman.com> > --- > libavcodec/vvc/vvc_ctu.c | 17 +++++++++++++++-- > 1 file changed, 15 insertions(+), 2 deletions(-) > > diff --git a/libavcodec/vvc/vvc_ctu.c b/libavcodec/vvc/vvc_ctu.c > index d166b16a19..f601c6c95e 100644 > --- a/libavcodec/vvc/vvc_ctu.c > +++ b/libavcodec/vvc/vvc_ctu.c > @@ -2207,7 +2207,7 @@ static void hls_sao(VVCLocalContext *lc, const int > rx, const int ry) > } > } > > -static void alf_params(VVCLocalContext *lc, const int rx, const int ry) > +static int alf_params(VVCLocalContext *lc, const int rx, const int ry) > { > const VVCFrameContext *fc = lc->fc; > const H266RawSliceHeader *sh = lc->sc->sh.r; > @@ -2233,6 +2233,11 @@ static void alf_params(VVCLocalContext *lc, const > int rx, const int ry) > c_idx == CB ? sh->sh_alf_cb_enabled_flag : > sh->sh_alf_cr_enabled_flag; > if (alf_enabled_flag) { > const VVCALF *aps = > fc->ps.alf_list[sh->sh_alf_aps_id_chroma]; > + if (!aps) { > + // Slice header refers to an APS which does not exist > + return AVERROR_INVALIDDATA; > + } > + > Thank you Frank. Since all "if conditions" are related to slices, could we perform the check at sh_derive()? > alf->ctb_flag[c_idx] = ff_vvc_alf_ctb_flag(lc, rx, ry, > c_idx); > alf->alf_ctb_filter_alt_idx[c_idx - 1] = 0; > if (alf->ctb_flag[c_idx] && aps->num_chroma_filters > 1) > @@ -2247,10 +2252,16 @@ static void alf_params(VVCLocalContext *lc, const > int rx, const int ry) > alf->ctb_cc_idc[i] = 0; > if (cc_enabled[i]) { > const VVCALF *aps = fc->ps.alf_list[cc_aps_id[i]]; > + if (!aps) { > + // Slice header refers to an APS which does not exist > + return AVERROR_INVALIDDATA; > + } > alf->ctb_cc_idc[i] = ff_vvc_alf_ctb_cc_idc(lc, rx, ry, i, > aps->num_cc_filters[i]); > } > } > } > + > + return 0; > } > > static void deblock_params(VVCLocalContext *lc, const int rx, const int > ry) > @@ -2274,7 +2285,9 @@ static int hls_coding_tree_unit(VVCLocalContext *lc, > memset(lc->parse.chroma_qp_offset, 0, > sizeof(lc->parse.chroma_qp_offset)); > > hls_sao(lc, x0 >> sps->ctb_log2_size_y, y0 >> sps->ctb_log2_size_y); > - alf_params(lc, x0 >> sps->ctb_log2_size_y, y0 >> > sps->ctb_log2_size_y); > + ret = alf_params(lc, x0 >> sps->ctb_log2_size_y, y0 >> > sps->ctb_log2_size_y); > + if (ret < 0) > + return ret; > deblock_params(lc, x0 >> sps->ctb_log2_size_y, y0 >> > sps->ctb_log2_size_y); > > if (IS_I(rsh) && sps->r->sps_qtbtt_dual_tree_intra_flag) > -- > 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 05/02/2024 13:45, Nuo Mi wrote: > On Sun, Feb 4, 2024 at 1:22 AM <post@frankplowman.com> wrote: > >> From: Frank Plowman <post@frankplowman.com> >> >> Fixes crashes when decoding illegal bitstreams found by fuzzing. >> >> Signed-off-by: Frank Plowman <post@frankplowman.com> >> --- >> libavcodec/vvc/vvc_ctu.c | 17 +++++++++++++++-- >> 1 file changed, 15 insertions(+), 2 deletions(-) >> >> diff --git a/libavcodec/vvc/vvc_ctu.c b/libavcodec/vvc/vvc_ctu.c >> index d166b16a19..f601c6c95e 100644 >> --- a/libavcodec/vvc/vvc_ctu.c >> +++ b/libavcodec/vvc/vvc_ctu.c >> @@ -2207,7 +2207,7 @@ static void hls_sao(VVCLocalContext *lc, const int >> rx, const int ry) >> } >> } >> >> -static void alf_params(VVCLocalContext *lc, const int rx, const int ry) >> +static int alf_params(VVCLocalContext *lc, const int rx, const int ry) >> { >> const VVCFrameContext *fc = lc->fc; >> const H266RawSliceHeader *sh = lc->sc->sh.r; >> @@ -2233,6 +2233,11 @@ static void alf_params(VVCLocalContext *lc, const >> int rx, const int ry) >> c_idx == CB ? sh->sh_alf_cb_enabled_flag : >> sh->sh_alf_cr_enabled_flag; >> if (alf_enabled_flag) { >> const VVCALF *aps = >> fc->ps.alf_list[sh->sh_alf_aps_id_chroma]; >> + if (!aps) { >> + // Slice header refers to an APS which does not exist >> + return AVERROR_INVALIDDATA; >> + } >> + >> > Thank you Frank. > Since all "if conditions" are related to slices, could we perform the check > at sh_derive()? Yes, I had another look at this yesterday and came to the same conclusion. Do you think it's better to derive pointers to the relevant APSs or just to validate the indices into fps->aps_list? > >> alf->ctb_flag[c_idx] = ff_vvc_alf_ctb_flag(lc, rx, ry, >> c_idx); >> alf->alf_ctb_filter_alt_idx[c_idx - 1] = 0; >> if (alf->ctb_flag[c_idx] && aps->num_chroma_filters > 1) >> @@ -2247,10 +2252,16 @@ static void alf_params(VVCLocalContext *lc, const >> int rx, const int ry) >> alf->ctb_cc_idc[i] = 0; >> if (cc_enabled[i]) { >> const VVCALF *aps = fc->ps.alf_list[cc_aps_id[i]]; >> + if (!aps) { >> + // Slice header refers to an APS which does not exist >> + return AVERROR_INVALIDDATA; >> + } >> alf->ctb_cc_idc[i] = ff_vvc_alf_ctb_cc_idc(lc, rx, ry, i, >> aps->num_cc_filters[i]); >> } >> } >> } >> + >> + return 0; >> } >> >> static void deblock_params(VVCLocalContext *lc, const int rx, const int >> ry) >> @@ -2274,7 +2285,9 @@ static int hls_coding_tree_unit(VVCLocalContext *lc, >> memset(lc->parse.chroma_qp_offset, 0, >> sizeof(lc->parse.chroma_qp_offset)); >> >> hls_sao(lc, x0 >> sps->ctb_log2_size_y, y0 >> sps->ctb_log2_size_y); >> - alf_params(lc, x0 >> sps->ctb_log2_size_y, y0 >> >> sps->ctb_log2_size_y); >> + ret = alf_params(lc, x0 >> sps->ctb_log2_size_y, y0 >> >> sps->ctb_log2_size_y); >> + if (ret < 0) >> + return ret; >> deblock_params(lc, x0 >> sps->ctb_log2_size_y, y0 >> >> sps->ctb_log2_size_y); >> >> if (IS_I(rsh) && sps->r->sps_qtbtt_dual_tree_intra_flag) >> -- >> 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". >> > _______________________________________________ > 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 Mon, Feb 5, 2024 at 10:04 PM Frank Plowman <post@frankplowman.com> wrote: > On 05/02/2024 13:45, Nuo Mi wrote: > > On Sun, Feb 4, 2024 at 1:22 AM <post@frankplowman.com> wrote: > > > >> From: Frank Plowman <post@frankplowman.com> > >> > >> Fixes crashes when decoding illegal bitstreams found by fuzzing. > >> > >> Signed-off-by: Frank Plowman <post@frankplowman.com> > >> --- > >> libavcodec/vvc/vvc_ctu.c | 17 +++++++++++++++-- > >> 1 file changed, 15 insertions(+), 2 deletions(-) > >> > >> diff --git a/libavcodec/vvc/vvc_ctu.c b/libavcodec/vvc/vvc_ctu.c > >> index d166b16a19..f601c6c95e 100644 > >> --- a/libavcodec/vvc/vvc_ctu.c > >> +++ b/libavcodec/vvc/vvc_ctu.c > >> @@ -2207,7 +2207,7 @@ static void hls_sao(VVCLocalContext *lc, const int > >> rx, const int ry) > >> } > >> } > >> > >> -static void alf_params(VVCLocalContext *lc, const int rx, const int ry) > >> +static int alf_params(VVCLocalContext *lc, const int rx, const int ry) > >> { > >> const VVCFrameContext *fc = lc->fc; > >> const H266RawSliceHeader *sh = lc->sc->sh.r; > >> @@ -2233,6 +2233,11 @@ static void alf_params(VVCLocalContext *lc, const > >> int rx, const int ry) > >> c_idx == CB ? sh->sh_alf_cb_enabled_flag : > >> sh->sh_alf_cr_enabled_flag; > >> if (alf_enabled_flag) { > >> const VVCALF *aps = > >> fc->ps.alf_list[sh->sh_alf_aps_id_chroma]; > >> + if (!aps) { > >> + // Slice header refers to an APS which does not > exist > >> + return AVERROR_INVALIDDATA; > >> + } > >> + > >> > > Thank you Frank. > > Since all "if conditions" are related to slices, could we perform the > check > > at sh_derive()? > > Yes, I had another look at this yesterday and came to the same > conclusion. Do you think it's better to derive pointers to the relevant > APSs or just to validate the indices into fps->aps_list? > > Check the fps->aps_list[i] should be enough. Make sure you run all the conformance tests, only conformance tests know what change is right :)
diff --git a/libavcodec/vvc/vvc_ctu.c b/libavcodec/vvc/vvc_ctu.c index d166b16a19..f601c6c95e 100644 --- a/libavcodec/vvc/vvc_ctu.c +++ b/libavcodec/vvc/vvc_ctu.c @@ -2207,7 +2207,7 @@ static void hls_sao(VVCLocalContext *lc, const int rx, const int ry) } } -static void alf_params(VVCLocalContext *lc, const int rx, const int ry) +static int alf_params(VVCLocalContext *lc, const int rx, const int ry) { const VVCFrameContext *fc = lc->fc; const H266RawSliceHeader *sh = lc->sc->sh.r; @@ -2233,6 +2233,11 @@ static void alf_params(VVCLocalContext *lc, const int rx, const int ry) c_idx == CB ? sh->sh_alf_cb_enabled_flag : sh->sh_alf_cr_enabled_flag; if (alf_enabled_flag) { const VVCALF *aps = fc->ps.alf_list[sh->sh_alf_aps_id_chroma]; + if (!aps) { + // Slice header refers to an APS which does not exist + return AVERROR_INVALIDDATA; + } + alf->ctb_flag[c_idx] = ff_vvc_alf_ctb_flag(lc, rx, ry, c_idx); alf->alf_ctb_filter_alt_idx[c_idx - 1] = 0; if (alf->ctb_flag[c_idx] && aps->num_chroma_filters > 1) @@ -2247,10 +2252,16 @@ static void alf_params(VVCLocalContext *lc, const int rx, const int ry) alf->ctb_cc_idc[i] = 0; if (cc_enabled[i]) { const VVCALF *aps = fc->ps.alf_list[cc_aps_id[i]]; + if (!aps) { + // Slice header refers to an APS which does not exist + return AVERROR_INVALIDDATA; + } alf->ctb_cc_idc[i] = ff_vvc_alf_ctb_cc_idc(lc, rx, ry, i, aps->num_cc_filters[i]); } } } + + return 0; } static void deblock_params(VVCLocalContext *lc, const int rx, const int ry) @@ -2274,7 +2285,9 @@ static int hls_coding_tree_unit(VVCLocalContext *lc, memset(lc->parse.chroma_qp_offset, 0, sizeof(lc->parse.chroma_qp_offset)); hls_sao(lc, x0 >> sps->ctb_log2_size_y, y0 >> sps->ctb_log2_size_y); - alf_params(lc, x0 >> sps->ctb_log2_size_y, y0 >> sps->ctb_log2_size_y); + ret = alf_params(lc, x0 >> sps->ctb_log2_size_y, y0 >> sps->ctb_log2_size_y); + if (ret < 0) + return ret; deblock_params(lc, x0 >> sps->ctb_log2_size_y, y0 >> sps->ctb_log2_size_y); if (IS_I(rsh) && sps->r->sps_qtbtt_dual_tree_intra_flag)