| 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 | expand |
| Context | Check | Description |
|---|---|---|
| andriy/default | pending | |
| andriy/make | success | Make finished |
| andriy/make_fate | success | Make fate finished |
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; >
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; >> >
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 [...]
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". >
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 [...]
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 --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;
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(-)