| Message ID | 20231216121619.19436-2-michael@niedermayer.cc |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,1/4] tools/target_dec_fuzzer: Adjust Threshold for VP6A | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/make_loongarch64 | success | Make finished |
| yinshiyou/make_fate_loongarch64 | success | Make fate finished |
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
On 12/16/2023 9:16 AM, Michael Niedermayer wrote: > Fixes: abort() > Fixes: 64232/clusterfuzz-testcase-minimized-ffmpeg_BSF_TRACE_HEADERS_fuzzer-5417957987319808 > > 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_vp8.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/libavcodec/cbs_vp8.c b/libavcodec/cbs_vp8.c > index 01d4b9cefef..b76cde98517 100644 > --- a/libavcodec/cbs_vp8.c > +++ b/libavcodec/cbs_vp8.c > @@ -329,7 +329,9 @@ static int cbs_vp8_read_unit(CodedBitstreamContext *ctx, > > pos = get_bits_count(&gbc); > pos /= 8; > - av_assert0(pos <= unit->data_size); > + > + if (pos > unit->data_size) > + return AVERROR_INVALIDDATA; Wouldn't this be hiding a bug in the parsing code? The assert is there to ensure no overread happened. > > frame->data_ref = av_buffer_ref(unit->data_ref); > if (!frame->data_ref)
On Sat, Dec 16, 2023 at 09:20:24AM -0300, James Almer wrote: > On 12/16/2023 9:16 AM, Michael Niedermayer wrote: > > Fixes: abort() > > Fixes: 64232/clusterfuzz-testcase-minimized-ffmpeg_BSF_TRACE_HEADERS_fuzzer-5417957987319808 > > > > 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_vp8.c | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/libavcodec/cbs_vp8.c b/libavcodec/cbs_vp8.c > > index 01d4b9cefef..b76cde98517 100644 > > --- a/libavcodec/cbs_vp8.c > > +++ b/libavcodec/cbs_vp8.c > > @@ -329,7 +329,9 @@ static int cbs_vp8_read_unit(CodedBitstreamContext *ctx, > > pos = get_bits_count(&gbc); > > pos /= 8; > > - av_assert0(pos <= unit->data_size); > > + > > + if (pos > unit->data_size) > > + return AVERROR_INVALIDDATA; > > Wouldn't this be hiding a bug in the parsing code? The assert is there to > ensure no overread happened. ill send a better patch, this patch is bad and wrong thx [...]
> -----Original Message----- > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Michael > Niedermayer > Sent: Saturday, December 16, 2023 8:16 PM > To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org> > Subject: [FFmpeg-devel] [PATCH 2/4] avcodec/cbs_vp8: Do not use assert to > check for end > > Fixes: abort() > Fixes: 64232/clusterfuzz-testcase-minimized- > ffmpeg_BSF_TRACE_HEADERS_fuzzer-5417957987319808 > > 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_vp8.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/libavcodec/cbs_vp8.c b/libavcodec/cbs_vp8.c index > 01d4b9cefef..b76cde98517 100644 > --- a/libavcodec/cbs_vp8.c > +++ b/libavcodec/cbs_vp8.c > @@ -329,7 +329,9 @@ static int cbs_vp8_read_unit(CodedBitstreamContext > *ctx, > > pos = get_bits_count(&gbc); > pos /= 8; > - av_assert0(pos <= unit->data_size); > + > + if (pos > unit->data_size) > + return AVERROR_INVALIDDATA; > This is a potentially fatal error caused by the parser overreading past the expected data. This should not occur after the fix GetBitContext setup patch was applied. BTW, the VP8 compressed header does not guarantee 8-bit alignment according to the SPEC. It could be better to check the bit pos. ``` pos = get_bits_count(&gbc); av_assert0(pos <= unit->data_size * 8); ``` > frame->data_ref = av_buffer_ref(unit->data_ref); > if (!frame->data_ref) > -- > 2.17.1 > > _______________________________________________ > 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".
diff --git a/libavcodec/cbs_vp8.c b/libavcodec/cbs_vp8.c index 01d4b9cefef..b76cde98517 100644 --- a/libavcodec/cbs_vp8.c +++ b/libavcodec/cbs_vp8.c @@ -329,7 +329,9 @@ static int cbs_vp8_read_unit(CodedBitstreamContext *ctx, pos = get_bits_count(&gbc); pos /= 8; - av_assert0(pos <= unit->data_size); + + if (pos > unit->data_size) + return AVERROR_INVALIDDATA; frame->data_ref = av_buffer_ref(unit->data_ref); if (!frame->data_ref)
Fixes: abort() Fixes: 64232/clusterfuzz-testcase-minimized-ffmpeg_BSF_TRACE_HEADERS_fuzzer-5417957987319808 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_vp8.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)