On Wed, May 20, 2020 at 10:29:17PM +0100, Mark Thompson wrote: > On 20/05/2020 22:16, Michael Niedermayer wrote: > > 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 ... > > I don't know how the decoder was written, but the intent in cbs has always > been to use exactly the same names as the standard does (even when those > names are long or redundant) so that code and variables can be precisely > matched for verification. > > >>> > >>> 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; > > > > [...] > > No, that doesn't make sense - the stream is invalid if it is trying to refer > to more than HEVC_MAX_REFS long term pics, so these extra entries could only > be used by invalid streams which are being incorrectly accepted. > > Changing the upper bound on num_long_term_pics to be HEVC_MAX_REFS - > num_long_term_sps would be sufficient to avoid this problem, though that will > admit some invalid streams which also contain short-term or self-reference > pics which together exceed the limit. Enforcing the proper limit as James > described is better, but adds more code to do it.
so thats then just this for the simple solution:
ill apply this tomorrow and backport unless i hear objections.
diff --git a/libavcodec/cbs_h265_syntax_template.c
b/libavcodec/cbs_h265_syntax_template.c
index d3ac618db60..9786a8dda23 100644
--- a/libavcodec/cbs_h265_syntax_template.c
+++ b/libavcodec/cbs_h265_syntax_template.c
@@ -1371,7 +1371,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, HEVC_MAX_REFS -
current->num_long_term_sps);
for (i = 0; i < current->num_long_term_sps +
current->num_long_term_pics; i++) {
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
If you think the mosad wants you dead since a long time then you are either
wrong or dead since a long time.
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".
