| Message ID | 20211005205125.4831-1-michael@niedermayer.cc |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel] avcodec/amr_parser: passthrough if channels is unknown | expand |
| Context | Check | Description |
|---|---|---|
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
| andriy/make_ppc | success | Make finished |
| andriy/make_fate_ppc | success | Make fate finished |
On 10/5/2021 5:51 PM, Michael Niedermayer wrote: > Fixes: division by 0 > Fixes: 39562/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_AMRWB_fuzzer-5448834960982016 > Fixes: 39589/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_AMRWB_fuzzer-6119205334810624 > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > --- > libavcodec/amr_parser.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/libavcodec/amr_parser.c b/libavcodec/amr_parser.c > index 79258d4d0cf..9fb7711fee0 100644 > --- a/libavcodec/amr_parser.c > +++ b/libavcodec/amr_parser.c > @@ -62,7 +62,7 @@ static int amr_parse(AVCodecParserContext *s1, > *poutbuf_size = 0; > *poutbuf = NULL; > > - if (s1->flags & PARSER_FLAG_COMPLETE_FRAMES) { > + if (s1->flags & PARSER_FLAG_COMPLETE_FRAMES || !avctx->channels) { No line in the parser divides by avctx->channels. There's however one doing a modulo avctx->channels. Wouldn't this mean the compiler is being overzealous with optimizations? Also, instead of this please do the same as the decoder, and set channels to 1 and layout to mono if nothing is already set. Forcing the "complete frames" path when no channels are set feels like a hacky workaround. If the relevant parser flag is not set, then you must not treat packets as if they contained complete frames. > next = buf_size; > } else { > int ch, offset = 0; >
On Tue, Oct 5, 2021 at 11:03 PM James Almer <jamrial@gmail.com> wrote: > On 10/5/2021 5:51 PM, Michael Niedermayer wrote: > > Fixes: division by 0 > > Fixes: > 39562/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_AMRWB_fuzzer-5448834960982016 > > Fixes: > 39589/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_AMRWB_fuzzer-6119205334810624 > > > > Found-by: continuous fuzzing process > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > --- > > libavcodec/amr_parser.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/libavcodec/amr_parser.c b/libavcodec/amr_parser.c > > index 79258d4d0cf..9fb7711fee0 100644 > > --- a/libavcodec/amr_parser.c > > +++ b/libavcodec/amr_parser.c > > @@ -62,7 +62,7 @@ static int amr_parse(AVCodecParserContext *s1, > > *poutbuf_size = 0; > > *poutbuf = NULL; > > > > - if (s1->flags & PARSER_FLAG_COMPLETE_FRAMES) { > > + if (s1->flags & PARSER_FLAG_COMPLETE_FRAMES || !avctx->channels) { > > No line in the parser divides by avctx->channels. There's however one > doing a modulo avctx->channels. Wouldn't this mean the compiler is being > overzealous with optimizations? > > Also, instead of this please do the same as the decoder, and set > channels to 1 and layout to mono if nothing is already set. Forcing the > "complete frames" path when no channels are set feels like a hacky > workaround. > If the relevant parser flag is not set, then you must not treat packets > as if they contained complete frames. > Agreed also with that solution. > > > next = buf_size; > > } else { > > int ch, offset = 0; > > > > _______________________________________________ > 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". >
James Almer: > On 10/5/2021 5:51 PM, Michael Niedermayer wrote: >> Fixes: division by 0 >> Fixes: >> 39562/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_AMRWB_fuzzer-5448834960982016 >> >> Fixes: >> 39589/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_AMRWB_fuzzer-6119205334810624 >> >> >> Found-by: continuous fuzzing process >> https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg >> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> >> --- >> libavcodec/amr_parser.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/libavcodec/amr_parser.c b/libavcodec/amr_parser.c >> index 79258d4d0cf..9fb7711fee0 100644 >> --- a/libavcodec/amr_parser.c >> +++ b/libavcodec/amr_parser.c >> @@ -62,7 +62,7 @@ static int amr_parse(AVCodecParserContext *s1, >> *poutbuf_size = 0; >> *poutbuf = NULL; >> - if (s1->flags & PARSER_FLAG_COMPLETE_FRAMES) { >> + if (s1->flags & PARSER_FLAG_COMPLETE_FRAMES || !avctx->channels) { > > No line in the parser divides by avctx->channels. There's however one > doing a modulo avctx->channels. Wouldn't this mean the compiler is being > overzealous with optimizations? > No. foo % 0 makes no sense and is undefined behaviour; this is even stated in the same sentence of the spec: "In both operations, if the value of the second operand is zero, the behavior is undefined." - Andreas
On Tue, Oct 05, 2021 at 11:08:19PM +0200, Paul B Mahol wrote: > On Tue, Oct 5, 2021 at 11:03 PM James Almer <jamrial@gmail.com> wrote: > > > On 10/5/2021 5:51 PM, Michael Niedermayer wrote: > > > Fixes: division by 0 > > > Fixes: > > 39562/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_AMRWB_fuzzer-5448834960982016 > > > Fixes: > > 39589/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_AMRWB_fuzzer-6119205334810624 > > > > > > Found-by: continuous fuzzing process > > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > > --- > > > libavcodec/amr_parser.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/libavcodec/amr_parser.c b/libavcodec/amr_parser.c > > > index 79258d4d0cf..9fb7711fee0 100644 > > > --- a/libavcodec/amr_parser.c > > > +++ b/libavcodec/amr_parser.c > > > @@ -62,7 +62,7 @@ static int amr_parse(AVCodecParserContext *s1, > > > *poutbuf_size = 0; > > > *poutbuf = NULL; > > > > > > - if (s1->flags & PARSER_FLAG_COMPLETE_FRAMES) { > > > + if (s1->flags & PARSER_FLAG_COMPLETE_FRAMES || !avctx->channels) { > > > > No line in the parser divides by avctx->channels. There's however one > > doing a modulo avctx->channels. Wouldn't this mean the compiler is being > > overzealous with optimizations? > > > > Also, instead of this please do the same as the decoder, and set > > channels to 1 and layout to mono if nothing is already set. Forcing the > > "complete frames" path when no channels are set feels like a hacky > > workaround. > > If the relevant parser flag is not set, then you must not treat packets > > as if they contained complete frames. > > > > Agreed also with that solution. will apply that then thx [...]
diff --git a/libavcodec/amr_parser.c b/libavcodec/amr_parser.c index 79258d4d0cf..9fb7711fee0 100644 --- a/libavcodec/amr_parser.c +++ b/libavcodec/amr_parser.c @@ -62,7 +62,7 @@ static int amr_parse(AVCodecParserContext *s1, *poutbuf_size = 0; *poutbuf = NULL; - if (s1->flags & PARSER_FLAG_COMPLETE_FRAMES) { + if (s1->flags & PARSER_FLAG_COMPLETE_FRAMES || !avctx->channels) { next = buf_size; } else { int ch, offset = 0;
Fixes: division by 0 Fixes: 39562/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_AMRWB_fuzzer-5448834960982016 Fixes: 39589/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_AMRWB_fuzzer-6119205334810624 Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavcodec/amr_parser.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)