[FFmpeg-devel] [PATCH 1/2] avcodec/cbs_h265_syntax_template: Better check for num_long_term_sps

[Michael Niedermayer]

On Sun, Nov 15, 2020 at 09:37:45PM +0000, Mark Thompson wrote:
> On 14/11/2020 10:18, Michael Niedermayer wrote:
> > Fixes: index 26 out of bounds for type 'uint8_t [16]'
> > Fixes: 24913/clusterfuzz-testcase-minimized-ffmpeg_BSF_HEVC_METADATA_fuzzer-6261760693370880
> > 
> > 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, 2 insertions(+)
> > 
> > diff --git a/libavcodec/cbs_h265_syntax_template.c b/libavcodec/cbs_h265_syntax_template.c
> > index 48fae82d04..8eb6e159f4 100644
> > --- a/libavcodec/cbs_h265_syntax_template.c
> > +++ b/libavcodec/cbs_h265_syntax_template.c
> > @@ -1405,6 +1405,8 @@ static int FUNC(slice_segment_header)(CodedBitstreamContext *ctx, RWContext *rw,
> >                       infer(num_long_term_sps, 0);
> >                       idx_size = 0;
> >                   }
> > +                if (HEVC_MAX_REFS < current->num_long_term_sps)
> > +                    return AVERROR_INVALIDDATA;
> 
> Please don't put isolated tests in the middle of the template.  If it constrains a value then add it to the constraints on that value.
> 
> >                   ue(num_long_term_pics, 0, HEVC_MAX_REFS - current->num_long_term_sps);
> >                   for (i = 0; i < current->num_long_term_sps +
> > 
> 
> It would be good if the commit message could include an explanation derived from the standard, too.
> 
> As far as I can tell the constraint doesn't appear explicitly, but the SPS is allowed to define more possible long term frames than are actually allowed to be present at any given moment so we need the tighter bound.

Is the change below what you had in mind ?

commit 72c6c46bb2b31b2822331aff461acccd0a4f9159 (HEAD -> master)
Author: Michael Niedermayer <michael at niedermayer.cc>
Date:   Fri Nov 13 23:15:52 2020 +0100

    avcodec/cbs_h265_syntax_template: Better check for num_long_term_sps
    
    As far as we can tell the constraint doesn't appear explicitly, but the SPS is allowed to
    define more possible long term frames than are actually allowed to be present at any given moment so we need the tighter bound.
    
    Fixes: index 26 out of bounds for type 'uint8_t [16]'
    Fixes: 24913/clusterfuzz-testcase-minimized-ffmpeg_BSF_HEVC_METADATA_fuzzer-6261760693370880
    
    Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
    Signed-off-by: Michael Niedermayer <michael at niedermayer.cc>

diff --git a/libavcodec/cbs_h265_syntax_template.c b/libavcodec/cbs_h265_syntax_template.c
index 48fae82d04..30f2bd328c 100644
--- a/libavcodec/cbs_h265_syntax_template.c
+++ b/libavcodec/cbs_h265_syntax_template.c
@@ -1399,7 +1399,7 @@ static int FUNC(slice_segment_header)(CodedBitstreamContext *ctx, RWContext *rw,
                 unsigned int idx_size;
 
                 if (sps->num_long_term_ref_pics_sps > 0) {
-                    ue(num_long_term_sps, 0, sps->num_long_term_ref_pics_sps);
+                    ue(num_long_term_sps, 0, FFMIN(sps->num_long_term_ref_pics_sps, HEVC_MAX_REFS));
                     idx_size = av_log2(sps->num_long_term_ref_pics_sps - 1) + 1;
                 } else {
                     infer(num_long_term_sps, 0);

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Elect your leaders based on what they did after the last election, not
based on what they say before an election.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20201121/ae4415cd/attachment.sig>