[FFmpeg-devel] [PATCH 3/3] avcodec/cbs_h265_syntax_template: Limit num_long_term_pics more strictly

[Michael Niedermayer]

On Mon, Apr 20, 2020 at 07:34:44PM -0300, James Almer wrote:
> On 4/20/2020 7:03 PM, Michael Niedermayer wrote:
> > The limit is based on hevcdec.c
> > Fixes: 20854/clusterfuzz-testcase-minimized-ffmpeg_BSF_HEVC_METADATA_fuzzer-5160442882424832
> > Fixes: out of array access
> > 
> > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > Signed-off-by: Michael Niedermayer <michael at niedermayer.cc>
> > ---
> >  libavcodec/cbs_h265_syntax_template.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/libavcodec/cbs_h265_syntax_template.c b/libavcodec/cbs_h265_syntax_template.c
> > index 180a045c34..b74b9648c3 100644
> > --- a/libavcodec/cbs_h265_syntax_template.c
> > +++ b/libavcodec/cbs_h265_syntax_template.c
> > @@ -1367,7 +1367,7 @@ static int FUNC(slice_segment_header)(CodedBitstreamContext *ctx, RWContext *rw,
> >                      infer(num_long_term_sps, 0);
> >                      idx_size = 0;
> >                  }
> > -                ue(num_long_term_pics, 0, HEVC_MAX_LONG_TERM_REF_PICS);
> > +                ue(num_long_term_pics, 0, FFMIN(HEVC_MAX_LONG_TERM_REF_PICS, FF_ARRAY_ELEMS(current->poc_lsb_lt) - current->num_long_term_sps));
> 
> Maybe poc_lsb_lt should also have HEVC_MAX_LONG_TERM_REF_PICS elems
> instead of HEVC_MAX_REFS, same as in the hevc decoder.

ok, btw the decoder and cbs use completely unrelated variable names.
That should be cleaned up by someone i think ...


> 
> Also the spec defines some specific limit to this field:
> 
> "When nuh_layer_id is equal to 0, the value of num_long_term_pics shall
> be less than or equal to sps_max_dec_pic_buffering_minus1[TemporalId] −
> NumNegativePics[CurrRpsIdx] − NumPositivePics[CurrRpsIdx] −
> num_long_term_sps − TwoVersionsOfCurrDecPicFlag"
> 
> With CurrRpsIdx derived as:
> – If short_term_ref_pic_set_sps_flag is equal to 1, CurrRpsIdx is set
> equal to short_term_ref_pic_set_idx.
> – Otherwise, CurrRpsIdx is set equal to num_short_term_ref_pic_sets.
> 
> And TwoVersionsOfCurrDecPicFlag as:
> "TwoVersionsOfCurrDecPicFlag = pps_curr_pic_ref_enabled_flag &&
> (sample_adaptive_offset_enabled_flag ||
> !pps_deblocking_filter_disabled_flag ||
> deblocking_filter_override_enabled_flag)
> When sps_max_dec_pic_buffering_minus1[ TemporalId ] is equal to 0, the
> value of TwoVersionsOfCurrDecPicFlag shall be equal to 0."
> 
> But i don't know if it's worth adding that many checks.

I saw this too when i wrote the original patch, and i remember that
it at least felt like these checks would not cover all cases.
Maybe ive missed something but if they dont cover all then this would
be unrelated as it would neither be sufficient nor helpfull for this
bugfix

will later apply this with the HEVC_MAX_LONG_TERM_REF_PICS as suggested
and backport unless i hear objections before

Thanks

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

I have never wished to cater to the crowd; for what I know they do not
approve, and what they approve I do not know. -- Epicurus
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20200520/013c63ca/attachment.sig>