On 21/11/2020 17:37, Michael Niedermayer wrote:
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 <mich...@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 <mich...@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.

I meant write a commit message which explains where in the standard the 
constraint is coming from.  I wrote that because I didn't see any extra 
constraint written in the standard for num_long_term_sps but 
num_long_term_ref_pics_sps is indeed bigger, so presumably it must be implied 
by something else.

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 <mich...@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));

Yes to this part.

                      idx_size = av_log2(sps->num_long_term_ref_pics_sps - 1) + 
1;
                  } else {
                      infer(num_long_term_sps, 0);

[...]

- Mark
_______________________________________________
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".

Reply via email to