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 <mich...@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.
signature.asc
Description: PGP signature
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
