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

[Michael Niedermayer]

On Wed, May 20, 2020 at 08:56:29PM +0200, Michael Niedermayer wrote:
> 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

tried this, but it seems that the increased size spreads and requires
other arrays to be enarged too
and that starts feeling a bit uncomfortable as a easy backportable
fix
so are you ok with the original variant and do you see any bugs/problems
in that ?
or i can also go for the array enlargement if thats really preferred ?
just wanted to make sure its understood that enlarging one array alone
doesnt work on its own ...

diff --git a/libavcodec/cbs_h265.h b/libavcodec/cbs_h265.h
index 73897f77a42..9b1895d460e 100644
--- a/libavcodec/cbs_h265.h
+++ b/libavcodec/cbs_h265.h
@@ -473,10 +473,10 @@ typedef struct  H265RawSliceHeader {
     uint8_t num_long_term_sps;
     uint8_t num_long_term_pics;
     uint8_t lt_idx_sps[HEVC_MAX_REFS];
-    uint8_t poc_lsb_lt[HEVC_MAX_REFS];
-    uint8_t used_by_curr_pic_lt_flag[HEVC_MAX_REFS];
-    uint8_t delta_poc_msb_present_flag[HEVC_MAX_REFS];
-    uint32_t delta_poc_msb_cycle_lt[HEVC_MAX_REFS];
+    uint8_t poc_lsb_lt[HEVC_MAX_LONG_TERM_REF_PICS];
+    uint8_t used_by_curr_pic_lt_flag[HEVC_MAX_LONG_TERM_REF_PICS];
+    uint8_t delta_poc_msb_present_flag[HEVC_MAX_LONG_TERM_REF_PICS];
+    uint32_t delta_poc_msb_cycle_lt[HEVC_MAX_LONG_TERM_REF_PICS];
 
     uint8_t slice_temporal_mvp_enabled_flag;
 
@@ -488,9 +488,9 @@ typedef struct  H265RawSliceHeader {
     uint8_t num_ref_idx_l1_active_minus1;
 
     uint8_t ref_pic_list_modification_flag_l0;
-    uint8_t list_entry_l0[HEVC_MAX_REFS];
+    uint8_t list_entry_l0[HEVC_MAX_LONG_TERM_REF_PICS];
     uint8_t ref_pic_list_modification_flag_l1;
-    uint8_t list_entry_l1[HEVC_MAX_REFS];
+    uint8_t list_entry_l1[HEVC_MAX_LONG_TERM_REF_PICS];
 
     uint8_t mvd_l1_zero_flag;
     uint8_t cabac_init_flag;

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

You can kill me, but you cannot change the truth.
-------------- 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/a52290d2/attachment.sig>