[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
> 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     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Take away the freedom of one citizen and you will be jailed, take away
the freedom of all citizens and you will be congratulated by your peers
in Parliament.
-------------- 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/a994c673/attachment.sig>