On Sun, Apr 19, 2020 at 05:52:01PM +0200, Lynne wrote: > Apr 19, 2020, 16:05 by mich...@niedermayer.cc: > > > 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 <mich...@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; > > > > Did you even bother to look at the checks you added in this decoder > previously? > Specifically 11 lines above? > > > if (sample_rate > INT_MAX / avctx->channels) > > return AVERROR_INVALIDDATA; > > sample_rate *= avctx->channels; > > To start with the sample rate of the avctx is already checked in utils.c, and > you > still haven't cleaned up any decoders from the checks made unnecessary by you,
The new check is needed, it overflows without: libavcodec/binkaudio.c:116:37: runtime error: signed integer overflow: 2147483647 + 1 cannot be represented in type 'int' The other check is also needed, if i remove it it still overflows libavcodec/binkaudio.c:100:22: runtime error: signed integer overflow: 1092624416 * 2 cannot be represented in type 'int' > so am reminding you again to clean up the codebase by getting rid of them. > At least you might get to clean the codebase for once rather than adding crap > like this. > > So there's only the branch which I quoted that's needed to be fixed, and > since there's a > check there already, there's no reason to have a check here as well. I posted exactly this same patch in january, there was a objection, and i asked what was preferred instead, and pinged it multiple times over the following months, in february and april: 189192 N F 0114 15:36 To FFmpeg devel (1,6K) [FFmpeg-devel] [PATCH] avcodec/binkaudio: Check sample_rate to avoid integer overflow 189193 r 0114 16:04 Paul B Mahol (2,1K) └─> 189194 sF 0201 16:14 To FFmpeg devel (1,7K) └─> 189195 r 0201 16:17 Paul B Mahol (1,3K) └─> 189196 rsF 0201 23:48 To FFmpeg devel (2,6K) └─> 189197 rs 0209 21:28 Michael Niederm (2,9K) └─> 189198 rsF 0404 23:38 To FFmpeg devel (2,8K) └─> 189199 sF 0407 23:17 To FFmpeg devel (3,1K) └─> but i failed to get a reply, so i tried reposting it So, if the patch is still not liked, can you explain what do you prefer ? i can put the sample_rate >= INT_MAX check in generic code but it is specific to this decoder it wont help anything else i can put a more aggressive check like sample_rate * channels > MAX_CHANNEL_SAMPLES in generic code, that will of course block some otherwise supported cases or we can have checks just for what is not supported or we can extend the code to support a wider range where it is possible Just say what you prefer, i dont mind at all what it is, i just want the issue fixed its already so many months open ... Thanks -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Many things microsoft did are stupid, but not doing something just because microsoft did it is even more stupid. If everything ms did were stupid they would be bankrupt already.
signature.asc
Description: PGP signature
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
