| Message ID | 20210729214421.5608-1-michael@niedermayer.cc |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel] avcodec/argo: Check for end of input in decode_alcd() | expand |
| Context | Check | Description |
|---|---|---|
| andriy/x86_make | success | Make finished |
| andriy/x86_make_fate | success | Make fate finished |
| andriy/PPC64_make | success | Make finished |
| andriy/PPC64_make_fate | success | Make fate finished |
Michael Niedermayer: > Fixes: reading over the end > Fixes: 36346/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_ARGO_fuzzer-5366943107383296 > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > --- > libavcodec/argo.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/libavcodec/argo.c b/libavcodec/argo.c > index bbdb6ae15f..79a44d2583 100644 > --- a/libavcodec/argo.c > +++ b/libavcodec/argo.c > @@ -116,6 +116,8 @@ static int decode_alcd(AVCodecContext *avctx, AVFrame *frame) > int index; > > if (count == 0) { > + if (bytestream2_get_bytes_left(gb) < 1) > + return AVERROR_INVALIDDATA; > codes = bytestream2_get_byteu(&sb); > count = 8; > } > Does the following also fix the issue? diff --git a/libavcodec/argo.c b/libavcodec/argo.c index bbdb6ae15f..602c042568 100644 --- a/libavcodec/argo.c +++ b/libavcodec/argo.c @@ -102,13 +102,14 @@ static int decode_alcd(AVCodecContext *avctx, AVFrame *frame) uint8_t *dst = frame->data[0]; uint8_t codes = 0; int count = 0; + int num_codes = ((frame->width + 1) / 2 * (frame->height + 1) / 2 + 7) >> 3; - if (bytestream2_get_bytes_left(gb) < 1024 + (((frame->width / 2) * (frame->height / 2) + 7) >> 3)) + if (bytestream2_get_bytes_left(gb) < 1024 + num_codes) return AVERROR_INVALIDDATA; bytestream2_skipu(gb, 1024); sb = *gb; - bytestream2_skipu(gb, ((frame->width / 2) * (frame->height / 2) + 7) >> 3); + bytestream2_skipu(gb, num_codes); for (int y = 0; y < frame->height; y += 2) { for (int x = 0; x < frame->width; x += 2) { - Andreas
Andreas Rheinhardt: > Michael Niedermayer: >> Fixes: reading over the end >> Fixes: 36346/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_ARGO_fuzzer-5366943107383296 >> >> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg >> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> >> --- >> libavcodec/argo.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/libavcodec/argo.c b/libavcodec/argo.c >> index bbdb6ae15f..79a44d2583 100644 >> --- a/libavcodec/argo.c >> +++ b/libavcodec/argo.c >> @@ -116,6 +116,8 @@ static int decode_alcd(AVCodecContext *avctx, AVFrame *frame) >> int index; >> >> if (count == 0) { >> + if (bytestream2_get_bytes_left(gb) < 1) >> + return AVERROR_INVALIDDATA; >> codes = bytestream2_get_byteu(&sb); >> count = 8; >> } >> > Does the following also fix the issue? > > diff --git a/libavcodec/argo.c b/libavcodec/argo.c > > index bbdb6ae15f..602c042568 100644 > > --- a/libavcodec/argo.c > > +++ b/libavcodec/argo.c > > @@ -102,13 +102,14 @@ static int decode_alcd(AVCodecContext *avctx, > AVFrame *frame) > > uint8_t *dst = frame->data[0]; > > uint8_t codes = 0; > > int count = 0; > > + int num_codes = ((frame->width + 1) / 2 * (frame->height + 1) / 2 + > 7) >> 3; > > > > - if (bytestream2_get_bytes_left(gb) < 1024 + (((frame->width / 2) * > (frame->height / 2) + 7) >> 3)) > > + if (bytestream2_get_bytes_left(gb) < 1024 + num_codes) > > return AVERROR_INVALIDDATA; > > > > bytestream2_skipu(gb, 1024); > > sb = *gb; > > - bytestream2_skipu(gb, ((frame->width / 2) * (frame->height / 2) + > 7) >> 3); > > + bytestream2_skipu(gb, num_codes); > > > > for (int y = 0; y < frame->height; y += 2) { > > for (int x = 0; x < frame->width; x += 2) { > As can be seen from the above patch, my guess is that odd dimensions are the cause (because num_codes as above is the number of codes that is actually read); but my patch would not only change the criterion for when to error out, but also how much to skip (i.e. where the real data begins) and this makes me wonder whether we should not error out in this case (and ask for a sample). - Andreas
On Fri, Jul 30, 2021 at 12:16 AM Andreas Rheinhardt < andreas.rheinhardt@outlook.com> wrote: > Andreas Rheinhardt: > > Michael Niedermayer: > >> Fixes: reading over the end > >> Fixes: > 36346/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_ARGO_fuzzer-5366943107383296 > >> > >> Found-by: continuous fuzzing process > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > >> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > >> --- > >> libavcodec/argo.c | 2 ++ > >> 1 file changed, 2 insertions(+) > >> > >> diff --git a/libavcodec/argo.c b/libavcodec/argo.c > >> index bbdb6ae15f..79a44d2583 100644 > >> --- a/libavcodec/argo.c > >> +++ b/libavcodec/argo.c > >> @@ -116,6 +116,8 @@ static int decode_alcd(AVCodecContext *avctx, > AVFrame *frame) > >> int index; > >> > >> if (count == 0) { > >> + if (bytestream2_get_bytes_left(gb) < 1) > >> + return AVERROR_INVALIDDATA; > >> codes = bytestream2_get_byteu(&sb); > >> count = 8; > >> } > >> > > Does the following also fix the issue? > > > > diff --git a/libavcodec/argo.c b/libavcodec/argo.c > > > > index bbdb6ae15f..602c042568 100644 > > > > --- a/libavcodec/argo.c > > > > +++ b/libavcodec/argo.c > > > > @@ -102,13 +102,14 @@ static int decode_alcd(AVCodecContext *avctx, > > AVFrame *frame) > > > > uint8_t *dst = frame->data[0]; > > > > uint8_t codes = 0; > > > > int count = 0; > > > > + int num_codes = ((frame->width + 1) / 2 * (frame->height + 1) / 2 + > > 7) >> 3; > > > > > > > > - if (bytestream2_get_bytes_left(gb) < 1024 + (((frame->width / 2) * > > (frame->height / 2) + 7) >> 3)) > > > > + if (bytestream2_get_bytes_left(gb) < 1024 + num_codes) > > > > return AVERROR_INVALIDDATA; > > > > > > > > bytestream2_skipu(gb, 1024); > > > > sb = *gb; > > > > - bytestream2_skipu(gb, ((frame->width / 2) * (frame->height / 2) + > > 7) >> 3); > > > > + bytestream2_skipu(gb, num_codes); > > > > > > > > for (int y = 0; y < frame->height; y += 2) { > > > > for (int x = 0; x < frame->width; x += 2) { > > > As can be seen from the above patch, my guess is that odd dimensions are > the cause (because num_codes as above is the number of codes that is > actually read); but my patch would not only change the criterion for > when to error out, but also how much to skip (i.e. where the real data > begins) and this makes me wonder whether we should not error out in this > case (and ask for a sample). > I like Andreas solution much more, anway original video files are having even dimensions, so there is nothing to worry. > - Andreas > _______________________________________________ > 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 Fri, Jul 30, 2021 at 08:44:03AM +0200, Paul B Mahol wrote: > On Fri, Jul 30, 2021 at 12:16 AM Andreas Rheinhardt < > andreas.rheinhardt@outlook.com> wrote: > > > Andreas Rheinhardt: > > > Michael Niedermayer: > > >> Fixes: reading over the end > > >> Fixes: > > 36346/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_ARGO_fuzzer-5366943107383296 > > >> > > >> Found-by: continuous fuzzing process > > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > > >> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > >> --- > > >> libavcodec/argo.c | 2 ++ > > >> 1 file changed, 2 insertions(+) > > >> > > >> diff --git a/libavcodec/argo.c b/libavcodec/argo.c > > >> index bbdb6ae15f..79a44d2583 100644 > > >> --- a/libavcodec/argo.c > > >> +++ b/libavcodec/argo.c > > >> @@ -116,6 +116,8 @@ static int decode_alcd(AVCodecContext *avctx, > > AVFrame *frame) > > >> int index; > > >> > > >> if (count == 0) { > > >> + if (bytestream2_get_bytes_left(gb) < 1) > > >> + return AVERROR_INVALIDDATA; > > >> codes = bytestream2_get_byteu(&sb); > > >> count = 8; > > >> } > > >> > > > Does the following also fix the issue? > > > > > > diff --git a/libavcodec/argo.c b/libavcodec/argo.c > > > > > > index bbdb6ae15f..602c042568 100644 > > > > > > --- a/libavcodec/argo.c > > > > > > +++ b/libavcodec/argo.c > > > > > > @@ -102,13 +102,14 @@ static int decode_alcd(AVCodecContext *avctx, > > > AVFrame *frame) > > > > > > uint8_t *dst = frame->data[0]; > > > > > > uint8_t codes = 0; > > > > > > int count = 0; > > > > > > + int num_codes = ((frame->width + 1) / 2 * (frame->height + 1) / 2 + > > > 7) >> 3; > > > > > > > > > > > > - if (bytestream2_get_bytes_left(gb) < 1024 + (((frame->width / 2) * > > > (frame->height / 2) + 7) >> 3)) > > > > > > + if (bytestream2_get_bytes_left(gb) < 1024 + num_codes) > > > > > > return AVERROR_INVALIDDATA; > > > > > > > > > > > > bytestream2_skipu(gb, 1024); > > > > > > sb = *gb; > > > > > > - bytestream2_skipu(gb, ((frame->width / 2) * (frame->height / 2) + > > > 7) >> 3); > > > > > > + bytestream2_skipu(gb, num_codes); > > > > > > > > > > > > for (int y = 0; y < frame->height; y += 2) { > > > > > > for (int x = 0; x < frame->width; x += 2) { > > > > > As can be seen from the above patch, my guess is that odd dimensions are > > the cause (because num_codes as above is the number of codes that is > > actually read); but my patch would not only change the criterion for > > when to error out, but also how much to skip (i.e. where the real data > > begins) and this makes me wonder whether we should not error out in this > > case (and ask for a sample). > > > > I like Andreas solution much more, anway original video files are having > even dimensions, > so there is nothing to worry. I did misread the code, andreas patch or a check for even dimensions is better than what i posted should i post a patch checking for even dimensions ? if yes, which of the cases should be covered by a even check ? i assume alcd and avcf at least Thanks [...]
On Fri, Jul 30, 2021 at 1:59 PM Michael Niedermayer <michael@niedermayer.cc> wrote: > On Fri, Jul 30, 2021 at 08:44:03AM +0200, Paul B Mahol wrote: > > On Fri, Jul 30, 2021 at 12:16 AM Andreas Rheinhardt < > > andreas.rheinhardt@outlook.com> wrote: > > > > > Andreas Rheinhardt: > > > > Michael Niedermayer: > > > >> Fixes: reading over the end > > > >> Fixes: > > > > 36346/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_ARGO_fuzzer-5366943107383296 > > > >> > > > >> Found-by: continuous fuzzing process > > > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > > > >> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > > >> --- > > > >> libavcodec/argo.c | 2 ++ > > > >> 1 file changed, 2 insertions(+) > > > >> > > > >> diff --git a/libavcodec/argo.c b/libavcodec/argo.c > > > >> index bbdb6ae15f..79a44d2583 100644 > > > >> --- a/libavcodec/argo.c > > > >> +++ b/libavcodec/argo.c > > > >> @@ -116,6 +116,8 @@ static int decode_alcd(AVCodecContext *avctx, > > > AVFrame *frame) > > > >> int index; > > > >> > > > >> if (count == 0) { > > > >> + if (bytestream2_get_bytes_left(gb) < 1) > > > >> + return AVERROR_INVALIDDATA; > > > >> codes = bytestream2_get_byteu(&sb); > > > >> count = 8; > > > >> } > > > >> > > > > Does the following also fix the issue? > > > > > > > > diff --git a/libavcodec/argo.c b/libavcodec/argo.c > > > > > > > > index bbdb6ae15f..602c042568 100644 > > > > > > > > --- a/libavcodec/argo.c > > > > > > > > +++ b/libavcodec/argo.c > > > > > > > > @@ -102,13 +102,14 @@ static int decode_alcd(AVCodecContext *avctx, > > > > AVFrame *frame) > > > > > > > > uint8_t *dst = frame->data[0]; > > > > > > > > uint8_t codes = 0; > > > > > > > > int count = 0; > > > > > > > > + int num_codes = ((frame->width + 1) / 2 * (frame->height + 1) / > 2 + > > > > 7) >> 3; > > > > > > > > > > > > > > > > - if (bytestream2_get_bytes_left(gb) < 1024 + (((frame->width / > 2) * > > > > (frame->height / 2) + 7) >> 3)) > > > > > > > > + if (bytestream2_get_bytes_left(gb) < 1024 + num_codes) > > > > > > > > return AVERROR_INVALIDDATA; > > > > > > > > > > > > > > > > bytestream2_skipu(gb, 1024); > > > > > > > > sb = *gb; > > > > > > > > - bytestream2_skipu(gb, ((frame->width / 2) * (frame->height / 2) > + > > > > 7) >> 3); > > > > > > > > + bytestream2_skipu(gb, num_codes); > > > > > > > > > > > > > > > > for (int y = 0; y < frame->height; y += 2) { > > > > > > > > for (int x = 0; x < frame->width; x += 2) { > > > > > > > As can be seen from the above patch, my guess is that odd dimensions > are > > > the cause (because num_codes as above is the number of codes that is > > > actually read); but my patch would not only change the criterion for > > > when to error out, but also how much to skip (i.e. where the real data > > > begins) and this makes me wonder whether we should not error out in > this > > > case (and ask for a sample). > > > > > > > I like Andreas solution much more, anway original video files are having > > even dimensions, > > so there is nothing to worry. > > I did misread the code, andreas patch or a check for even dimensions is > better > than what i posted > > should i post a patch checking for even dimensions ? > Yes > if yes, which of the cases should be covered by a even check ? > just add check for even dimensions at init? > i assume alcd and avcf at least > > Thanks > > [...] > -- > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB > > Frequently ignored answer#1 FFmpeg bugs should be sent to our bugtracker. > User > questions about the command line tools should be sent to the ffmpeg-user > ML. > And questions about how to use libav* should be sent to the libav-user ML. > _______________________________________________ > 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/argo.c b/libavcodec/argo.c index bbdb6ae15f..79a44d2583 100644 --- a/libavcodec/argo.c +++ b/libavcodec/argo.c @@ -116,6 +116,8 @@ static int decode_alcd(AVCodecContext *avctx, AVFrame *frame) int index; if (count == 0) { + if (bytestream2_get_bytes_left(gb) < 1) + return AVERROR_INVALIDDATA; codes = bytestream2_get_byteu(&sb); count = 8; }
Fixes: reading over the end Fixes: 36346/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_ARGO_fuzzer-5366943107383296 Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavcodec/argo.c | 2 ++ 1 file changed, 2 insertions(+)