| Message ID | 20200114143645.10358-1-michael@niedermayer.cc |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel] avcodec/binkaudio: Check sample_rate to avoid integer overflow | expand |
| Context | Check | Description |
|---|---|---|
| andriy/ffmpeg-patchwork | success | Make fate finished |
This better belong to generic code. On 1/14/20, Michael Niedermayer <michael@niedermayer.cc> wrote: > Fixes: signed integer overflow: 2147483647 + 1 cannot be represented in type > 'int' > Fixes: > 19950/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_BINKAUDIO_DCT_fuzzer-5765514337189888 > > Found-by: continuous fuzzing process > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > --- > libavcodec/binkaudio.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/libavcodec/binkaudio.c b/libavcodec/binkaudio.c > index 64a08b8608..2df3dc645a 100644 > --- a/libavcodec/binkaudio.c > +++ b/libavcodec/binkaudio.c > @@ -106,6 +106,9 @@ static av_cold int decode_init(AVCodecContext *avctx) > avctx->sample_fmt = AV_SAMPLE_FMT_FLTP; > } > > + if (sample_rate >= INT_MAX) > + return AVERROR_INVALIDDATA; > + > s->frame_len = 1 << frame_len_bits; > s->overlap_len = s->frame_len / 16; > s->block_size = (s->frame_len - s->overlap_len) * s->channels; > -- > 2.24.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".
On Tue, Jan 14, 2020 at 04:04:29PM +0100, Paul B Mahol wrote:
> This better belong to generic code.
This specific check (which checks for INT_MAX) is specific to our
bink audio code which does a +1
so it would not fit in generic code
We could arbitrarily decide on a maximum sample rate hardcode that
and check for that in generic code.
I can implement that if people prefer. It would not avoid all
sample rate checks in codecs though ...
Thanks
[...]
On 2/1/20, Michael Niedermayer <michael@niedermayer.cc> wrote: > On Tue, Jan 14, 2020 at 04:04:29PM +0100, Paul B Mahol wrote: >> This better belong to generic code. > > This specific check (which checks for INT_MAX) is specific to our > bink audio code which does a +1 > so it would not fit in generic code > > We could arbitrarily decide on a maximum sample rate hardcode that > and check for that in generic code. > I can implement that if people prefer. It would not avoid all > sample rate checks in codecs though ... sample rate can not be > INT_MAX > Thanks > > [...] > -- > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB > > In a rich man's house there is no place to spit but his face. > -- Diogenes of Sinope >
On Sat, Feb 01, 2020 at 04:17:10PM +0100, Paul B Mahol wrote: > On 2/1/20, Michael Niedermayer <michael@niedermayer.cc> wrote: > > On Tue, Jan 14, 2020 at 04:04:29PM +0100, Paul B Mahol wrote: > >> This better belong to generic code. > > > > This specific check (which checks for INT_MAX) is specific to our > > bink audio code which does a +1 > > so it would not fit in generic code > > > > We could arbitrarily decide on a maximum sample rate hardcode that > > and check for that in generic code. > > I can implement that if people prefer. It would not avoid all > > sample rate checks in codecs though ... > > sample rate can not be > INT_MAX no and the code also doesnt check > INT_MAX I think you maybe missed the = in >= theres a +1 and INT_MAX+1 is bad so INT_MAX is checked for we can do that in generic code but its only this decoder that has this issue other decoders may have other limits. That makes this specific check threshold bad for a check in generic code. Another threshold would work in generic code, it would be arbitrary though and limit most decoders more than needed Iam happy to implement what people prefer but the check as it is makes not much sense if its moved as is into generic code Thanks [...]
On Sat, Feb 01, 2020 at 11:48:06PM +0100, Michael Niedermayer wrote: > On Sat, Feb 01, 2020 at 04:17:10PM +0100, Paul B Mahol wrote: > > On 2/1/20, Michael Niedermayer <michael@niedermayer.cc> wrote: > > > On Tue, Jan 14, 2020 at 04:04:29PM +0100, Paul B Mahol wrote: > > >> This better belong to generic code. > > > > > > This specific check (which checks for INT_MAX) is specific to our > > > bink audio code which does a +1 > > > so it would not fit in generic code > > > > > > We could arbitrarily decide on a maximum sample rate hardcode that > > > and check for that in generic code. > > > I can implement that if people prefer. It would not avoid all > > > sample rate checks in codecs though ... > > > > sample rate can not be > INT_MAX > > no and the code also doesnt check > INT_MAX > I think you maybe missed the = in >= > theres a +1 and INT_MAX+1 is bad so INT_MAX is checked for > we can do that in generic code but its only this decoder that has this > issue other decoders may have other limits. That makes this specific > check threshold bad for a check in generic code. Another threshold > would work in generic code, it would be arbitrary though and limit > most decoders more than needed > Iam happy to implement what people prefer but the check as it is > makes not much sense if its moved as is into generic code any preferrance on how to solve this ? or you are ok with the patch ? thanks [...]
On Sun, Feb 09, 2020 at 09:28:48PM +0100, Michael Niedermayer wrote: > On Sat, Feb 01, 2020 at 11:48:06PM +0100, Michael Niedermayer wrote: > > On Sat, Feb 01, 2020 at 04:17:10PM +0100, Paul B Mahol wrote: > > > On 2/1/20, Michael Niedermayer <michael@niedermayer.cc> wrote: > > > > On Tue, Jan 14, 2020 at 04:04:29PM +0100, Paul B Mahol wrote: > > > >> This better belong to generic code. > > > > > > > > This specific check (which checks for INT_MAX) is specific to our > > > > bink audio code which does a +1 > > > > so it would not fit in generic code > > > > > > > > We could arbitrarily decide on a maximum sample rate hardcode that > > > > and check for that in generic code. > > > > I can implement that if people prefer. It would not avoid all > > > > sample rate checks in codecs though ... > > > > > > sample rate can not be > INT_MAX > > > > no and the code also doesnt check > INT_MAX > > I think you maybe missed the = in >= > > theres a +1 and INT_MAX+1 is bad so INT_MAX is checked for > > we can do that in generic code but its only this decoder that has this > > issue other decoders may have other limits. That makes this specific > > check threshold bad for a check in generic code. Another threshold > > would work in generic code, it would be arbitrary though and limit > > most decoders more than needed > > Iam happy to implement what people prefer but the check as it is > > makes not much sense if its moved as is into generic code > > any preferrance on how to solve this ? > or you are ok with the patch ? ping [...]
On Sat, Apr 04, 2020 at 11:38:43PM +0200, Michael Niedermayer wrote: > On Sun, Feb 09, 2020 at 09:28:48PM +0100, Michael Niedermayer wrote: > > On Sat, Feb 01, 2020 at 11:48:06PM +0100, Michael Niedermayer wrote: > > > On Sat, Feb 01, 2020 at 04:17:10PM +0100, Paul B Mahol wrote: > > > > On 2/1/20, Michael Niedermayer <michael@niedermayer.cc> wrote: > > > > > On Tue, Jan 14, 2020 at 04:04:29PM +0100, Paul B Mahol wrote: > > > > >> This better belong to generic code. > > > > > > > > > > This specific check (which checks for INT_MAX) is specific to our > > > > > bink audio code which does a +1 > > > > > so it would not fit in generic code > > > > > > > > > > We could arbitrarily decide on a maximum sample rate hardcode that > > > > > and check for that in generic code. > > > > > I can implement that if people prefer. It would not avoid all > > > > > sample rate checks in codecs though ... > > > > > > > > sample rate can not be > INT_MAX > > > > > > no and the code also doesnt check > INT_MAX > > > I think you maybe missed the = in >= > > > theres a +1 and INT_MAX+1 is bad so INT_MAX is checked for > > > we can do that in generic code but its only this decoder that has this > > > issue other decoders may have other limits. That makes this specific > > > check threshold bad for a check in generic code. Another threshold > > > would work in generic code, it would be arbitrary though and limit > > > most decoders more than needed > > > Iam happy to implement what people prefer but the check as it is > > > makes not much sense if its moved as is into generic code > > > > any preferrance on how to solve this ? > > or you are ok with the patch ? > > ping paul, are you ok with me applying the patch ? it seems to me that its the most obvious fix here thanks [...]
diff --git a/libavcodec/binkaudio.c b/libavcodec/binkaudio.c index 64a08b8608..2df3dc645a 100644 --- a/libavcodec/binkaudio.c +++ b/libavcodec/binkaudio.c @@ -106,6 +106,9 @@ static av_cold int decode_init(AVCodecContext *avctx) avctx->sample_fmt = AV_SAMPLE_FMT_FLTP; } + if (sample_rate >= INT_MAX) + return AVERROR_INVALIDDATA; + s->frame_len = 1 << frame_len_bits; s->overlap_len = s->frame_len / 16; s->block_size = (s->frame_len - s->overlap_len) * s->channels;
Fixes: signed integer overflow: 2147483647 + 1 cannot be represented in type 'int' Fixes: 19950/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_BINKAUDIO_DCT_fuzzer-5765514337189888 Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavcodec/binkaudio.c | 3 +++ 1 file changed, 3 insertions(+)