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

[James Almer]

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 at 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 at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
>