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 <mich...@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.
signature.asc
Description: PGP signature
_______________________________________________ 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".
