diff mbox series

[FFmpeg-devel,1/3] avcodec/cbs_h265_syntax_template: Check inter_ref_pic_set_prediction_flag

Message ID 20200606160347.16805-1-michael@niedermayer.cc
State New
Headers show
Series [FFmpeg-devel,1/3] avcodec/cbs_h265_syntax_template: Check inter_ref_pic_set_prediction_flag
Related show

Checks

Context Check Description
andriy/default pending
andriy/make success Make finished
andriy/make_fate success Make fate finished

Commit Message

Michael Niedermayer June 6, 2020, 4:03 p.m. UTC
Fixes: out of array access
Fixes: 23034/clusterfuzz-testcase-minimized-ffmpeg_BSF_HEVC_METADATA_fuzzer-5074645169733632.fuzz

Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 libavcodec/cbs_h265_syntax_template.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Comments

James Almer June 6, 2020, 4:12 p.m. UTC | #1
On 6/6/2020 1:03 PM, Michael Niedermayer wrote:
> Fixes: out of array access
> Fixes: 23034/clusterfuzz-testcase-minimized-ffmpeg_BSF_HEVC_METADATA_fuzzer-5074645169733632.fuzz
> 
> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavcodec/cbs_h265_syntax_template.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/libavcodec/cbs_h265_syntax_template.c b/libavcodec/cbs_h265_syntax_template.c
> index 5b7d1aa837..900764a3cf 100644
> --- a/libavcodec/cbs_h265_syntax_template.c
> +++ b/libavcodec/cbs_h265_syntax_template.c
> @@ -518,8 +518,11 @@ static int FUNC(st_ref_pic_set)(CodedBitstreamContext *ctx, RWContext *rw,
>  
>      if (st_rps_idx != 0)
>          flag(inter_ref_pic_set_prediction_flag);
> -    else
> +    else {
>          infer(inter_ref_pic_set_prediction_flag, 0);
> +        if (current->inter_ref_pic_set_prediction_flag)

This makes no sense. The infer(inter_ref_pic_set_prediction_flag, 0)
line sets current->inter_ref_pic_set_prediction_flag to 0. How can this
check even succeed?

Can you give some context?

> +            return AVERROR_INVALIDDATA;
> +    }
>  
>      if (current->inter_ref_pic_set_prediction_flag) {
>          unsigned int ref_rps_idx, num_delta_pocs, num_ref_pics;
>
James Almer June 6, 2020, 5:12 p.m. UTC | #2
On 6/6/2020 1:12 PM, James Almer wrote:
> On 6/6/2020 1:03 PM, Michael Niedermayer wrote:
>> Fixes: out of array access
>> Fixes: 23034/clusterfuzz-testcase-minimized-ffmpeg_BSF_HEVC_METADATA_fuzzer-5074645169733632.fuzz
>>
>> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
>> ---
>>  libavcodec/cbs_h265_syntax_template.c | 5 ++++-
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/libavcodec/cbs_h265_syntax_template.c b/libavcodec/cbs_h265_syntax_template.c
>> index 5b7d1aa837..900764a3cf 100644
>> --- a/libavcodec/cbs_h265_syntax_template.c
>> +++ b/libavcodec/cbs_h265_syntax_template.c
>> @@ -518,8 +518,11 @@ static int FUNC(st_ref_pic_set)(CodedBitstreamContext *ctx, RWContext *rw,
>>  
>>      if (st_rps_idx != 0)
>>          flag(inter_ref_pic_set_prediction_flag);
>> -    else
>> +    else {
>>          infer(inter_ref_pic_set_prediction_flag, 0);
>> +        if (current->inter_ref_pic_set_prediction_flag)
> 
> This makes no sense. The infer(inter_ref_pic_set_prediction_flag, 0)
> line sets current->inter_ref_pic_set_prediction_flag to 0. How can this
> check even succeed?
> 
> Can you give some context?

Oh, i guess this is during writing, not reading, where you just get a
warning about mismatching values.

Sounds like the bug is elsewhere, then, seeing
inter_ref_pic_set_prediction_flag was set to 1 in an scenario where it
should have been 0.

> 
>> +            return AVERROR_INVALIDDATA;
>> +    }
>>  
>>      if (current->inter_ref_pic_set_prediction_flag) {
>>          unsigned int ref_rps_idx, num_delta_pocs, num_ref_pics;
>>
>
Michael Niedermayer June 6, 2020, 5:57 p.m. UTC | #3
On Sat, Jun 06, 2020 at 01:12:04PM -0300, James Almer wrote:
> On 6/6/2020 1:03 PM, Michael Niedermayer wrote:
> > Fixes: out of array access
> > Fixes: 23034/clusterfuzz-testcase-minimized-ffmpeg_BSF_HEVC_METADATA_fuzzer-5074645169733632.fuzz
> > 
> > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > ---
> >  libavcodec/cbs_h265_syntax_template.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/libavcodec/cbs_h265_syntax_template.c b/libavcodec/cbs_h265_syntax_template.c
> > index 5b7d1aa837..900764a3cf 100644
> > --- a/libavcodec/cbs_h265_syntax_template.c
> > +++ b/libavcodec/cbs_h265_syntax_template.c
> > @@ -518,8 +518,11 @@ static int FUNC(st_ref_pic_set)(CodedBitstreamContext *ctx, RWContext *rw,
> >  
> >      if (st_rps_idx != 0)
> >          flag(inter_ref_pic_set_prediction_flag);
> > -    else
> > +    else {
> >          infer(inter_ref_pic_set_prediction_flag, 0);
> > +        if (current->inter_ref_pic_set_prediction_flag)
> 
> This makes no sense. The infer(inter_ref_pic_set_prediction_flag, 0)
> line sets current->inter_ref_pic_set_prediction_flag to 0. How can this
> check even succeed?
> 
> Can you give some context?

well
libavcodec/cbs_h2645.c
sets the value on read but on write it just prints a warning, it doesnt
set it nor does it error out.

#define infer(name, value) do { \
        if (current->name != (value)) { \
            av_log(ctx->log_ctx, AV_LOG_WARNING, "Warning: " \
                   "%s does not match inferred value: " \
                   "%"PRId64", but should be %"PRId64".\n", \
                   #name, (int64_t)current->name, (int64_t)(value)); \
        } \
    } while (0)

I do not know what the intend exactly was of this, but it doesnt make sense
to print a warning and then continue and crash.

either the warning should be a assert/abort() and no code should ever be
allowed to set this to such value. Or the code must not crash.
My check implements the 2nd case, I did hesitate a bit on the error code
but that seems what almost everything in the surrounding uses.
But EINVAL might be more correct, i can use that if preferred?

Thanks

[...]
James Almer June 6, 2020, 6:10 p.m. UTC | #4
On 6/6/2020 2:57 PM, Michael Niedermayer wrote:
> On Sat, Jun 06, 2020 at 01:12:04PM -0300, James Almer wrote:
>> On 6/6/2020 1:03 PM, Michael Niedermayer wrote:
>>> Fixes: out of array access
>>> Fixes: 23034/clusterfuzz-testcase-minimized-ffmpeg_BSF_HEVC_METADATA_fuzzer-5074645169733632.fuzz
>>>
>>> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
>>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
>>> ---
>>>  libavcodec/cbs_h265_syntax_template.c | 5 ++++-
>>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/libavcodec/cbs_h265_syntax_template.c b/libavcodec/cbs_h265_syntax_template.c
>>> index 5b7d1aa837..900764a3cf 100644
>>> --- a/libavcodec/cbs_h265_syntax_template.c
>>> +++ b/libavcodec/cbs_h265_syntax_template.c
>>> @@ -518,8 +518,11 @@ static int FUNC(st_ref_pic_set)(CodedBitstreamContext *ctx, RWContext *rw,
>>>  
>>>      if (st_rps_idx != 0)
>>>          flag(inter_ref_pic_set_prediction_flag);
>>> -    else
>>> +    else {
>>>          infer(inter_ref_pic_set_prediction_flag, 0);
>>> +        if (current->inter_ref_pic_set_prediction_flag)
>>
>> This makes no sense. The infer(inter_ref_pic_set_prediction_flag, 0)
>> line sets current->inter_ref_pic_set_prediction_flag to 0. How can this
>> check even succeed?
>>
>> Can you give some context?
> 
> well
> libavcodec/cbs_h2645.c
> sets the value on read but on write it just prints a warning, it doesnt
> set it nor does it error out.
> 
> #define infer(name, value) do { \
>         if (current->name != (value)) { \
>             av_log(ctx->log_ctx, AV_LOG_WARNING, "Warning: " \
>                    "%s does not match inferred value: " \
>                    "%"PRId64", but should be %"PRId64".\n", \
>                    #name, (int64_t)current->name, (int64_t)(value)); \
>         } \
>     } while (0)
> 
> I do not know what the intend exactly was of this, but it doesnt make sense
> to print a warning and then continue and crash.
> 
> either the warning should be a assert/abort() and no code should ever be
> allowed to set this to such value. Or the code must not crash.
> My check implements the 2nd case, I did hesitate a bit on the error code
> but that seems what almost everything in the surrounding uses.
> But EINVAL might be more correct, i can use that if preferred?

As i said in my second email, the fact it's set to 1 when it should be 0
hints that the bug is elsewhere. Is this triggered from the call in
cbs_h265_write_slice_segment_header()? It's the only one i see could
somehow end up behaving like this if for example the active SPS it uses
to parse the slice header is the wrong one (Where
sps->num_short_term_ref_pic_sets is unexpectedly 0 and thus
inter_ref_pic_set_prediction_flag is read using infer()).
The hevc_metadata bsf doesn't let you manually modify
inter_ref_pic_set_prediction_flag, so the value should remain untouched
between read and write.

As for infer() asserting and/or aborting, I'd ask Mark. BSFs shouldn't
let you set values that would result in infer() failing during writing,
so it might be a good idea to abort in those cases seeing it means
there's an internal parsing bug.

> 
> Thanks
> 
> [...]
> 
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>
Michael Niedermayer June 6, 2020, 7:08 p.m. UTC | #5
On Sat, Jun 06, 2020 at 03:10:56PM -0300, James Almer wrote:
> On 6/6/2020 2:57 PM, Michael Niedermayer wrote:
> > On Sat, Jun 06, 2020 at 01:12:04PM -0300, James Almer wrote:
> >> On 6/6/2020 1:03 PM, Michael Niedermayer wrote:
> >>> Fixes: out of array access
> >>> Fixes: 23034/clusterfuzz-testcase-minimized-ffmpeg_BSF_HEVC_METADATA_fuzzer-5074645169733632.fuzz
> >>>
> >>> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> >>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> >>> ---
> >>>  libavcodec/cbs_h265_syntax_template.c | 5 ++++-
> >>>  1 file changed, 4 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/libavcodec/cbs_h265_syntax_template.c b/libavcodec/cbs_h265_syntax_template.c
> >>> index 5b7d1aa837..900764a3cf 100644
> >>> --- a/libavcodec/cbs_h265_syntax_template.c
> >>> +++ b/libavcodec/cbs_h265_syntax_template.c
> >>> @@ -518,8 +518,11 @@ static int FUNC(st_ref_pic_set)(CodedBitstreamContext *ctx, RWContext *rw,
> >>>  
> >>>      if (st_rps_idx != 0)
> >>>          flag(inter_ref_pic_set_prediction_flag);
> >>> -    else
> >>> +    else {
> >>>          infer(inter_ref_pic_set_prediction_flag, 0);
> >>> +        if (current->inter_ref_pic_set_prediction_flag)
> >>
> >> This makes no sense. The infer(inter_ref_pic_set_prediction_flag, 0)
> >> line sets current->inter_ref_pic_set_prediction_flag to 0. How can this
> >> check even succeed?
> >>
> >> Can you give some context?
> > 
> > well
> > libavcodec/cbs_h2645.c
> > sets the value on read but on write it just prints a warning, it doesnt
> > set it nor does it error out.
> > 
> > #define infer(name, value) do { \
> >         if (current->name != (value)) { \
> >             av_log(ctx->log_ctx, AV_LOG_WARNING, "Warning: " \
> >                    "%s does not match inferred value: " \
> >                    "%"PRId64", but should be %"PRId64".\n", \
> >                    #name, (int64_t)current->name, (int64_t)(value)); \
> >         } \
> >     } while (0)
> > 
> > I do not know what the intend exactly was of this, but it doesnt make sense
> > to print a warning and then continue and crash.
> > 
> > either the warning should be a assert/abort() and no code should ever be
> > allowed to set this to such value. Or the code must not crash.
> > My check implements the 2nd case, I did hesitate a bit on the error code
> > but that seems what almost everything in the surrounding uses.
> > But EINVAL might be more correct, i can use that if preferred?
> 
> As i said in my second email, the fact it's set to 1 when it should be 0
> hints that the bug is elsewhere. Is this triggered from the call in
> cbs_h265_write_slice_segment_header()? It's the only one i see could

yes


> somehow end up behaving like this if for example the active SPS it uses
> to parse the slice header is the wrong one (Where
> sps->num_short_term_ref_pic_sets is unexpectedly 0 and thus

there are 2 SPS, with num_short_term_ref_pic_sets was read as 13 and as 0 


> inter_ref_pic_set_prediction_flag is read using infer()).
> The hevc_metadata bsf doesn't let you manually modify
> inter_ref_pic_set_prediction_flag, so the value should remain untouched
> between read and write.

Iam not sure if all saftey checks the writer need should be in the reader.
Especially with the complexity of these headers.
Either way, ill send you the testcase so you can look at it instead of
having to guess. You seem to understand this code quite well

thx

[...]
Michael Niedermayer June 6, 2020, 7:16 p.m. UTC | #6
On Sat, Jun 06, 2020 at 03:10:56PM -0300, James Almer wrote:
> On 6/6/2020 2:57 PM, Michael Niedermayer wrote:
> > On Sat, Jun 06, 2020 at 01:12:04PM -0300, James Almer wrote:
> >> On 6/6/2020 1:03 PM, Michael Niedermayer wrote:
> >>> Fixes: out of array access
> >>> Fixes: 23034/clusterfuzz-testcase-minimized-ffmpeg_BSF_HEVC_METADATA_fuzzer-5074645169733632.fuzz
> >>>
> >>> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> >>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> >>> ---
> >>>  libavcodec/cbs_h265_syntax_template.c | 5 ++++-
> >>>  1 file changed, 4 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/libavcodec/cbs_h265_syntax_template.c b/libavcodec/cbs_h265_syntax_template.c
> >>> index 5b7d1aa837..900764a3cf 100644
> >>> --- a/libavcodec/cbs_h265_syntax_template.c
> >>> +++ b/libavcodec/cbs_h265_syntax_template.c
> >>> @@ -518,8 +518,11 @@ static int FUNC(st_ref_pic_set)(CodedBitstreamContext *ctx, RWContext *rw,
> >>>  
> >>>      if (st_rps_idx != 0)
> >>>          flag(inter_ref_pic_set_prediction_flag);
> >>> -    else
> >>> +    else {
> >>>          infer(inter_ref_pic_set_prediction_flag, 0);
> >>> +        if (current->inter_ref_pic_set_prediction_flag)
> >>
> >> This makes no sense. The infer(inter_ref_pic_set_prediction_flag, 0)
> >> line sets current->inter_ref_pic_set_prediction_flag to 0. How can this
> >> check even succeed?
> >>
> >> Can you give some context?
> > 
> > well
> > libavcodec/cbs_h2645.c
> > sets the value on read but on write it just prints a warning, it doesnt
> > set it nor does it error out.
> > 
> > #define infer(name, value) do { \
> >         if (current->name != (value)) { \
> >             av_log(ctx->log_ctx, AV_LOG_WARNING, "Warning: " \
> >                    "%s does not match inferred value: " \
> >                    "%"PRId64", but should be %"PRId64".\n", \
> >                    #name, (int64_t)current->name, (int64_t)(value)); \
> >         } \
> >     } while (0)
> > 
> > I do not know what the intend exactly was of this, but it doesnt make sense
> > to print a warning and then continue and crash.
> > 
> > either the warning should be a assert/abort() and no code should ever be
> > allowed to set this to such value. Or the code must not crash.
> > My check implements the 2nd case, I did hesitate a bit on the error code
> > but that seems what almost everything in the surrounding uses.
> > But EINVAL might be more correct, i can use that if preferred?
> 

> As i said in my second email, the fact it's set to 1 when it should be 0

it seems your second mail appeard just now (2h late), hadnt seen it before


[...]
diff mbox series

Patch

diff --git a/libavcodec/cbs_h265_syntax_template.c b/libavcodec/cbs_h265_syntax_template.c
index 5b7d1aa837..900764a3cf 100644
--- a/libavcodec/cbs_h265_syntax_template.c
+++ b/libavcodec/cbs_h265_syntax_template.c
@@ -518,8 +518,11 @@  static int FUNC(st_ref_pic_set)(CodedBitstreamContext *ctx, RWContext *rw,
 
     if (st_rps_idx != 0)
         flag(inter_ref_pic_set_prediction_flag);
-    else
+    else {
         infer(inter_ref_pic_set_prediction_flag, 0);
+        if (current->inter_ref_pic_set_prediction_flag)
+            return AVERROR_INVALIDDATA;
+    }
 
     if (current->inter_ref_pic_set_prediction_flag) {
         unsigned int ref_rps_idx, num_delta_pocs, num_ref_pics;