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

[Michael Niedermayer]

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 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

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


[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Any man who breaks a law that conscience tells him is unjust and willingly 
accepts the penalty by staying in jail in order to arouse the conscience of 
the community on the injustice of the law is at that moment expressing the 
very highest respect for law. - Martin Luther King Jr
-------------- 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/20200606/3344cd81/attachment.sig>