| Message ID | 20230929232001.23197-1-michael@niedermayer.cc |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,1/6] avformat/avidec: support huge durations | 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 Sat, 30 Sep 2023, Michael Niedermayer wrote: > Fixes: signed integer overflow: 109817402400 * 301990077 cannot be represented in type 'long long' > Fixes: 51896/clusterfuzz-testcase-minimized-ffmpeg_dem_AVI_fuzzer-6706191715139584 > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > --- > libavformat/avidec.c | 12 ++++++++++-- > 1 file changed, 10 insertions(+), 2 deletions(-) > > diff --git a/libavformat/avidec.c b/libavformat/avidec.c > index 00bd7a98a9d..cfc693842b7 100644 > --- a/libavformat/avidec.c > +++ b/libavformat/avidec.c > @@ -27,6 +27,7 @@ > #include "libavutil/avstring.h" > #include "libavutil/opt.h" > #include "libavutil/dict.h" > +#include "libavutil/integer.h" > #include "libavutil/internal.h" > #include "libavutil/intreadwrite.h" > #include "libavutil/mathematics.h" > @@ -476,7 +477,7 @@ static int calculate_bitrate(AVFormatContext *s) > AVStream *st = s->streams[i]; > FFStream *const sti = ffstream(st); > int64_t duration; > - int64_t bitrate; > + int64_t bitrate = 0; > > for (j = 0; j < sti->nb_index_entries; j++) > len += sti->index_entries[j].size; > @@ -484,7 +485,14 @@ static int calculate_bitrate(AVFormatContext *s) > if (sti->nb_index_entries < 2 || st->codecpar->bit_rate > 0) > continue; > duration = sti->index_entries[j-1].timestamp - sti->index_entries[0].timestamp; > - bitrate = av_rescale(8*len, st->time_base.den, duration * st->time_base.num); > + if (INT64_MAX / duration >= st->time_base.num) { > + bitrate = av_rescale(8*len, st->time_base.den, duration * st->time_base.num); Why not always use the AVInteger version? This is not performance sensitive as far as I see. Regards, Marton > + } else { > + AVInteger bitrate_i = av_div_i(av_mul_i(av_int2i(8*len), av_int2i(st->time_base.den)), > + av_mul_i(av_int2i(duration), av_int2i(st->time_base.num))); > + if (av_cmp_i(bitrate_i, av_int2i(INT64_MAX)) <= 0) > + bitrate = av_i2int(bitrate_i); > + } > if (bitrate > 0) { > st->codecpar->bit_rate = bitrate; > } > -- > 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". >
On Sat, Sep 30, 2023 at 11:35:19AM +0200, Marton Balint wrote: > > > On Sat, 30 Sep 2023, Michael Niedermayer wrote: > > > Fixes: signed integer overflow: 109817402400 * 301990077 cannot be represented in type 'long long' > > Fixes: 51896/clusterfuzz-testcase-minimized-ffmpeg_dem_AVI_fuzzer-6706191715139584 > > > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > --- > > libavformat/avidec.c | 12 ++++++++++-- > > 1 file changed, 10 insertions(+), 2 deletions(-) > > > > diff --git a/libavformat/avidec.c b/libavformat/avidec.c > > index 00bd7a98a9d..cfc693842b7 100644 > > --- a/libavformat/avidec.c > > +++ b/libavformat/avidec.c > > @@ -27,6 +27,7 @@ > > #include "libavutil/avstring.h" > > #include "libavutil/opt.h" > > #include "libavutil/dict.h" > > +#include "libavutil/integer.h" > > #include "libavutil/internal.h" > > #include "libavutil/intreadwrite.h" > > #include "libavutil/mathematics.h" > > @@ -476,7 +477,7 @@ static int calculate_bitrate(AVFormatContext *s) > > AVStream *st = s->streams[i]; > > FFStream *const sti = ffstream(st); > > int64_t duration; > > - int64_t bitrate; > > + int64_t bitrate = 0; > > > > for (j = 0; j < sti->nb_index_entries; j++) > > len += sti->index_entries[j].size; > > @@ -484,7 +485,14 @@ static int calculate_bitrate(AVFormatContext *s) > > if (sti->nb_index_entries < 2 || st->codecpar->bit_rate > 0) > > continue; > > duration = sti->index_entries[j-1].timestamp - sti->index_entries[0].timestamp; > > - bitrate = av_rescale(8*len, st->time_base.den, duration * st->time_base.num); > > + if (INT64_MAX / duration >= st->time_base.num) { > > + bitrate = av_rescale(8*len, st->time_base.den, duration * st->time_base.num); > > Why not always use the AVInteger version? This is not performance sensitive > as far as I see. We can, i will have to fix the rounding though so it matches av_rescale() [...]
On Sat, Sep 30, 2023 at 04:04:03PM +0200, Michael Niedermayer wrote: > On Sat, Sep 30, 2023 at 11:35:19AM +0200, Marton Balint wrote: > > > > > > On Sat, 30 Sep 2023, Michael Niedermayer wrote: > > > > > Fixes: signed integer overflow: 109817402400 * 301990077 cannot be represented in type 'long long' > > > Fixes: 51896/clusterfuzz-testcase-minimized-ffmpeg_dem_AVI_fuzzer-6706191715139584 > > > > > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > > --- > > > libavformat/avidec.c | 12 ++++++++++-- > > > 1 file changed, 10 insertions(+), 2 deletions(-) > > > > > > diff --git a/libavformat/avidec.c b/libavformat/avidec.c > > > index 00bd7a98a9d..cfc693842b7 100644 > > > --- a/libavformat/avidec.c > > > +++ b/libavformat/avidec.c > > > @@ -27,6 +27,7 @@ > > > #include "libavutil/avstring.h" > > > #include "libavutil/opt.h" > > > #include "libavutil/dict.h" > > > +#include "libavutil/integer.h" > > > #include "libavutil/internal.h" > > > #include "libavutil/intreadwrite.h" > > > #include "libavutil/mathematics.h" > > > @@ -476,7 +477,7 @@ static int calculate_bitrate(AVFormatContext *s) > > > AVStream *st = s->streams[i]; > > > FFStream *const sti = ffstream(st); > > > int64_t duration; > > > - int64_t bitrate; > > > + int64_t bitrate = 0; > > > > > > for (j = 0; j < sti->nb_index_entries; j++) > > > len += sti->index_entries[j].size; > > > @@ -484,7 +485,14 @@ static int calculate_bitrate(AVFormatContext *s) > > > if (sti->nb_index_entries < 2 || st->codecpar->bit_rate > 0) > > > continue; > > > duration = sti->index_entries[j-1].timestamp - sti->index_entries[0].timestamp; > > > - bitrate = av_rescale(8*len, st->time_base.den, duration * st->time_base.num); > > > + if (INT64_MAX / duration >= st->time_base.num) { > > > + bitrate = av_rescale(8*len, st->time_base.den, duration * st->time_base.num); > > > > Why not always use the AVInteger version? This is not performance sensitive > > as far as I see. > > We can, i will have to fix the rounding though so it matches av_rescale() will apply this with just AVInteger and fixed rounding with my next push probably thx [...]
Quoting Michael Niedermayer (2023-09-30 16:31:43) > On Sat, Sep 30, 2023 at 04:04:03PM +0200, Michael Niedermayer wrote: > > On Sat, Sep 30, 2023 at 11:35:19AM +0200, Marton Balint wrote: > > > > > > > > > On Sat, 30 Sep 2023, Michael Niedermayer wrote: > > > > > > > Fixes: signed integer overflow: 109817402400 * 301990077 cannot be represented in type 'long long' > > > > Fixes: 51896/clusterfuzz-testcase-minimized-ffmpeg_dem_AVI_fuzzer-6706191715139584 > > > > > > > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > > > --- > > > > libavformat/avidec.c | 12 ++++++++++-- > > > > 1 file changed, 10 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/libavformat/avidec.c b/libavformat/avidec.c > > > > index 00bd7a98a9d..cfc693842b7 100644 > > > > --- a/libavformat/avidec.c > > > > +++ b/libavformat/avidec.c > > > > @@ -27,6 +27,7 @@ > > > > #include "libavutil/avstring.h" > > > > #include "libavutil/opt.h" > > > > #include "libavutil/dict.h" > > > > +#include "libavutil/integer.h" > > > > #include "libavutil/internal.h" > > > > #include "libavutil/intreadwrite.h" > > > > #include "libavutil/mathematics.h" > > > > @@ -476,7 +477,7 @@ static int calculate_bitrate(AVFormatContext *s) > > > > AVStream *st = s->streams[i]; > > > > FFStream *const sti = ffstream(st); > > > > int64_t duration; > > > > - int64_t bitrate; > > > > + int64_t bitrate = 0; > > > > > > > > for (j = 0; j < sti->nb_index_entries; j++) > > > > len += sti->index_entries[j].size; > > > > @@ -484,7 +485,14 @@ static int calculate_bitrate(AVFormatContext *s) > > > > if (sti->nb_index_entries < 2 || st->codecpar->bit_rate > 0) > > > > continue; > > > > duration = sti->index_entries[j-1].timestamp - sti->index_entries[0].timestamp; > > > > - bitrate = av_rescale(8*len, st->time_base.den, duration * st->time_base.num); > > > > + if (INT64_MAX / duration >= st->time_base.num) { > > > > + bitrate = av_rescale(8*len, st->time_base.den, duration * st->time_base.num); > > > > > > Why not always use the AVInteger version? This is not performance sensitive > > > as far as I see. > > > > We can, i will have to fix the rounding though so it matches av_rescale() > > will apply this with just AVInteger and fixed rounding with my next push probably This seems MUCH less readable to me. Do we expect such huge durations in actual files rather than fuzzed ones? Would it not be better to just not export the duration at all when the computation would overflow?
On Sat, Sep 30, 2023 at 10:18:38PM +0200, Anton Khirnov wrote: > Quoting Michael Niedermayer (2023-09-30 16:31:43) > > On Sat, Sep 30, 2023 at 04:04:03PM +0200, Michael Niedermayer wrote: > > > On Sat, Sep 30, 2023 at 11:35:19AM +0200, Marton Balint wrote: > > > > > > > > > > > > On Sat, 30 Sep 2023, Michael Niedermayer wrote: > > > > > > > > > Fixes: signed integer overflow: 109817402400 * 301990077 cannot be represented in type 'long long' > > > > > Fixes: 51896/clusterfuzz-testcase-minimized-ffmpeg_dem_AVI_fuzzer-6706191715139584 > > > > > > > > > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > > > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > > > > --- > > > > > libavformat/avidec.c | 12 ++++++++++-- > > > > > 1 file changed, 10 insertions(+), 2 deletions(-) > > > > > > > > > > diff --git a/libavformat/avidec.c b/libavformat/avidec.c > > > > > index 00bd7a98a9d..cfc693842b7 100644 > > > > > --- a/libavformat/avidec.c > > > > > +++ b/libavformat/avidec.c > > > > > @@ -27,6 +27,7 @@ > > > > > #include "libavutil/avstring.h" > > > > > #include "libavutil/opt.h" > > > > > #include "libavutil/dict.h" > > > > > +#include "libavutil/integer.h" > > > > > #include "libavutil/internal.h" > > > > > #include "libavutil/intreadwrite.h" > > > > > #include "libavutil/mathematics.h" > > > > > @@ -476,7 +477,7 @@ static int calculate_bitrate(AVFormatContext *s) > > > > > AVStream *st = s->streams[i]; > > > > > FFStream *const sti = ffstream(st); > > > > > int64_t duration; > > > > > - int64_t bitrate; > > > > > + int64_t bitrate = 0; > > > > > > > > > > for (j = 0; j < sti->nb_index_entries; j++) > > > > > len += sti->index_entries[j].size; > > > > > @@ -484,7 +485,14 @@ static int calculate_bitrate(AVFormatContext *s) > > > > > if (sti->nb_index_entries < 2 || st->codecpar->bit_rate > 0) > > > > > continue; > > > > > duration = sti->index_entries[j-1].timestamp - sti->index_entries[0].timestamp; > > > > > - bitrate = av_rescale(8*len, st->time_base.den, duration * st->time_base.num); > > > > > + if (INT64_MAX / duration >= st->time_base.num) { > > > > > + bitrate = av_rescale(8*len, st->time_base.den, duration * st->time_base.num); > > > > > > > > Why not always use the AVInteger version? This is not performance sensitive > > > > as far as I see. > > > > > > We can, i will have to fix the rounding though so it matches av_rescale() > > > > will apply this with just AVInteger and fixed rounding with my next push probably > > This seems MUCH less readable to me. we can add a av_rescale_2den() > > Do we expect such huge durations in actual files rather than fuzzed > ones? no, i dont expect that. > Would it not be better to just not export the duration at all when > the computation would overflow? Better ? i dont know if i scale that idea up and replace EVERY computation with a subset that seems enough and prettier. I think that would not result in overall very robust or clean code. If its better to do it just here i dont know but ill do whatever people prefer thx [...]
Quoting Michael Niedermayer (2023-10-01 00:28:56) > On Sat, Sep 30, 2023 at 10:18:38PM +0200, Anton Khirnov wrote: > > Quoting Michael Niedermayer (2023-09-30 16:31:43) > > > On Sat, Sep 30, 2023 at 04:04:03PM +0200, Michael Niedermayer wrote: > > > > On Sat, Sep 30, 2023 at 11:35:19AM +0200, Marton Balint wrote: > > > > > > > > > > > > > > > On Sat, 30 Sep 2023, Michael Niedermayer wrote: > > > > > > > > > > > Fixes: signed integer overflow: 109817402400 * 301990077 cannot be represented in type 'long long' > > > > > > Fixes: 51896/clusterfuzz-testcase-minimized-ffmpeg_dem_AVI_fuzzer-6706191715139584 > > > > > > > > > > > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > > > > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > > > > > --- > > > > > > libavformat/avidec.c | 12 ++++++++++-- > > > > > > 1 file changed, 10 insertions(+), 2 deletions(-) > > > > > > > > > > > > diff --git a/libavformat/avidec.c b/libavformat/avidec.c > > > > > > index 00bd7a98a9d..cfc693842b7 100644 > > > > > > --- a/libavformat/avidec.c > > > > > > +++ b/libavformat/avidec.c > > > > > > @@ -27,6 +27,7 @@ > > > > > > #include "libavutil/avstring.h" > > > > > > #include "libavutil/opt.h" > > > > > > #include "libavutil/dict.h" > > > > > > +#include "libavutil/integer.h" > > > > > > #include "libavutil/internal.h" > > > > > > #include "libavutil/intreadwrite.h" > > > > > > #include "libavutil/mathematics.h" > > > > > > @@ -476,7 +477,7 @@ static int calculate_bitrate(AVFormatContext *s) > > > > > > AVStream *st = s->streams[i]; > > > > > > FFStream *const sti = ffstream(st); > > > > > > int64_t duration; > > > > > > - int64_t bitrate; > > > > > > + int64_t bitrate = 0; > > > > > > > > > > > > for (j = 0; j < sti->nb_index_entries; j++) > > > > > > len += sti->index_entries[j].size; > > > > > > @@ -484,7 +485,14 @@ static int calculate_bitrate(AVFormatContext *s) > > > > > > if (sti->nb_index_entries < 2 || st->codecpar->bit_rate > 0) > > > > > > continue; > > > > > > duration = sti->index_entries[j-1].timestamp - sti->index_entries[0].timestamp; > > > > > > - bitrate = av_rescale(8*len, st->time_base.den, duration * st->time_base.num); > > > > > > + if (INT64_MAX / duration >= st->time_base.num) { > > > > > > + bitrate = av_rescale(8*len, st->time_base.den, duration * st->time_base.num); > > > > > > > > > > Why not always use the AVInteger version? This is not performance sensitive > > > > > as far as I see. > > > > > > > > We can, i will have to fix the rounding though so it matches av_rescale() > > > > > > will apply this with just AVInteger and fixed rounding with my next push probably > > > > This seems MUCH less readable to me. > > we can add a av_rescale_2den() Won't av_rescale_q(len, (AVRational){8, duration}, st->time_base) achieve the same effect?
On Mon, Oct 02, 2023 at 11:07:47AM +0200, Anton Khirnov wrote: > Quoting Michael Niedermayer (2023-10-01 00:28:56) > > On Sat, Sep 30, 2023 at 10:18:38PM +0200, Anton Khirnov wrote: > > > Quoting Michael Niedermayer (2023-09-30 16:31:43) > > > > On Sat, Sep 30, 2023 at 04:04:03PM +0200, Michael Niedermayer wrote: > > > > > On Sat, Sep 30, 2023 at 11:35:19AM +0200, Marton Balint wrote: > > > > > > > > > > > > > > > > > > On Sat, 30 Sep 2023, Michael Niedermayer wrote: > > > > > > > > > > > > > Fixes: signed integer overflow: 109817402400 * 301990077 cannot be represented in type 'long long' > > > > > > > Fixes: 51896/clusterfuzz-testcase-minimized-ffmpeg_dem_AVI_fuzzer-6706191715139584 > > > > > > > > > > > > > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > > > > > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > > > > > > --- > > > > > > > libavformat/avidec.c | 12 ++++++++++-- > > > > > > > 1 file changed, 10 insertions(+), 2 deletions(-) > > > > > > > > > > > > > > diff --git a/libavformat/avidec.c b/libavformat/avidec.c > > > > > > > index 00bd7a98a9d..cfc693842b7 100644 > > > > > > > --- a/libavformat/avidec.c > > > > > > > +++ b/libavformat/avidec.c > > > > > > > @@ -27,6 +27,7 @@ > > > > > > > #include "libavutil/avstring.h" > > > > > > > #include "libavutil/opt.h" > > > > > > > #include "libavutil/dict.h" > > > > > > > +#include "libavutil/integer.h" > > > > > > > #include "libavutil/internal.h" > > > > > > > #include "libavutil/intreadwrite.h" > > > > > > > #include "libavutil/mathematics.h" > > > > > > > @@ -476,7 +477,7 @@ static int calculate_bitrate(AVFormatContext *s) > > > > > > > AVStream *st = s->streams[i]; > > > > > > > FFStream *const sti = ffstream(st); > > > > > > > int64_t duration; > > > > > > > - int64_t bitrate; > > > > > > > + int64_t bitrate = 0; > > > > > > > > > > > > > > for (j = 0; j < sti->nb_index_entries; j++) > > > > > > > len += sti->index_entries[j].size; > > > > > > > @@ -484,7 +485,14 @@ static int calculate_bitrate(AVFormatContext *s) > > > > > > > if (sti->nb_index_entries < 2 || st->codecpar->bit_rate > 0) > > > > > > > continue; > > > > > > > duration = sti->index_entries[j-1].timestamp - sti->index_entries[0].timestamp; > > > > > > > - bitrate = av_rescale(8*len, st->time_base.den, duration * st->time_base.num); > > > > > > > + if (INT64_MAX / duration >= st->time_base.num) { > > > > > > > + bitrate = av_rescale(8*len, st->time_base.den, duration * st->time_base.num); > > > > > > > > > > > > Why not always use the AVInteger version? This is not performance sensitive > > > > > > as far as I see. > > > > > > > > > > We can, i will have to fix the rounding though so it matches av_rescale() > > > > > > > > will apply this with just AVInteger and fixed rounding with my next push probably > > > > > > This seems MUCH less readable to me. > > > > we can add a av_rescale_2den() > > Won't av_rescale_q(len, (AVRational){8, duration}, st->time_base) > achieve the same effect? duration is 64bit AVRational is 32/32 bit, so i would expect that to not work. If duration was fitting in 32bit then duration * st->time_base.num would not have overflowed thx [...]
mån 2023-10-02 klockan 21:03 +0200 skrev Michael Niedermayer: > On Mon, Oct 02, 2023 at 11:07:47AM +0200, Anton Khirnov wrote: > > Quoting Michael Niedermayer (2023-10-01 00:28:56) > > > On Sat, Sep 30, 2023 at 10:18:38PM +0200, Anton Khirnov wrote: > > > > Quoting Michael Niedermayer (2023-09-30 16:31:43) > > > > > On Sat, Sep 30, 2023 at 04:04:03PM +0200, Michael Niedermayer > > > > > wrote: > > > > > > On Sat, Sep 30, 2023 at 11:35:19AM +0200, Marton Balint > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > On Sat, 30 Sep 2023, Michael Niedermayer wrote: > > > > > > > > > > > > > > > Fixes: signed integer overflow: 109817402400 * > > > > > > > > 301990077 cannot be represented in type 'long long' > > > > > > > > Fixes: 51896/clusterfuzz-testcase-minimized- > > > > > > > > ffmpeg_dem_AVI_fuzzer-6706191715139584 > > > > > > > > > > > > > > > > Found-by: continuous fuzzing process > > > > > > > > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > > > > > > > > Signed-off-by: Michael Niedermayer > > > > > > > > <michael@niedermayer.cc> > > > > > > > > --- > > > > > > > > libavformat/avidec.c | 12 ++++++++++-- > > > > > > > > 1 file changed, 10 insertions(+), 2 deletions(-) > > > > > > > > > > > > > > > > diff --git a/libavformat/avidec.c > > > > > > > > b/libavformat/avidec.c > > > > > > > > index 00bd7a98a9d..cfc693842b7 100644 > > > > > > > > --- a/libavformat/avidec.c > > > > > > > > +++ b/libavformat/avidec.c > > > > > > > > @@ -27,6 +27,7 @@ > > > > > > > > #include "libavutil/avstring.h" > > > > > > > > #include "libavutil/opt.h" > > > > > > > > #include "libavutil/dict.h" > > > > > > > > +#include "libavutil/integer.h" > > > > > > > > #include "libavutil/internal.h" > > > > > > > > #include "libavutil/intreadwrite.h" > > > > > > > > #include "libavutil/mathematics.h" > > > > > > > > @@ -476,7 +477,7 @@ static int > > > > > > > > calculate_bitrate(AVFormatContext *s) > > > > > > > > AVStream *st = s->streams[i]; > > > > > > > > FFStream *const sti = ffstream(st); > > > > > > > > int64_t duration; > > > > > > > > - int64_t bitrate; > > > > > > > > + int64_t bitrate = 0; > > > > > > > > > > > > > > > > for (j = 0; j < sti->nb_index_entries; j++) > > > > > > > > len += sti->index_entries[j].size; > > > > > > > > @@ -484,7 +485,14 @@ static int > > > > > > > > calculate_bitrate(AVFormatContext *s) > > > > > > > > if (sti->nb_index_entries < 2 || st->codecpar- > > > > > > > > >bit_rate > 0) > > > > > > > > continue; > > > > > > > > duration = sti->index_entries[j-1].timestamp - > > > > > > > > sti->index_entries[0].timestamp; > > > > > > > > - bitrate = av_rescale(8*len, st->time_base.den, > > > > > > > > duration * st->time_base.num); > > > > > > > > + if (INT64_MAX / duration >= st->time_base.num) > > > > > > > > { > > > > > > > > + bitrate = av_rescale(8*len, st- > > > > > > > > >time_base.den, duration * st->time_base.num); > > > > > > > > > > > > > > Why not always use the AVInteger version? This is not > > > > > > > performance sensitive > > > > > > > as far as I see. > > > > > > > > > > > > We can, i will have to fix the rounding though so it > > > > > > matches av_rescale() > > > > > > > > > > will apply this with just AVInteger and fixed rounding with > > > > > my next push probably > > > > > > > > This seems MUCH less readable to me. > > > > > > we can add a av_rescale_2den() > > > > Won't av_rescale_q(len, (AVRational){8, duration}, st->time_base) > > achieve the same effect? > > duration is 64bit AVRational is 32/32 bit, so i would expect that to > not > work. > If duration was fitting in 32bit then duration * st->time_base.num > would not > have overflowed Maybe we need a 64/64-bit version of AVRational.. /Tomas
On Tue, Oct 03, 2023 at 11:10:20AM +0200, Tomas Härdin wrote: > mån 2023-10-02 klockan 21:03 +0200 skrev Michael Niedermayer: > > On Mon, Oct 02, 2023 at 11:07:47AM +0200, Anton Khirnov wrote: > > > Quoting Michael Niedermayer (2023-10-01 00:28:56) > > > > On Sat, Sep 30, 2023 at 10:18:38PM +0200, Anton Khirnov wrote: > > > > > Quoting Michael Niedermayer (2023-09-30 16:31:43) > > > > > > On Sat, Sep 30, 2023 at 04:04:03PM +0200, Michael Niedermayer > > > > > > wrote: > > > > > > > On Sat, Sep 30, 2023 at 11:35:19AM +0200, Marton Balint > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > On Sat, 30 Sep 2023, Michael Niedermayer wrote: > > > > > > > > > > > > > > > > > Fixes: signed integer overflow: 109817402400 * > > > > > > > > > 301990077 cannot be represented in type 'long long' > > > > > > > > > Fixes: 51896/clusterfuzz-testcase-minimized- > > > > > > > > > ffmpeg_dem_AVI_fuzzer-6706191715139584 > > > > > > > > > > > > > > > > > > Found-by: continuous fuzzing process > > > > > > > > > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > > > > > > > > > Signed-off-by: Michael Niedermayer > > > > > > > > > <michael@niedermayer.cc> > > > > > > > > > --- > > > > > > > > > libavformat/avidec.c | 12 ++++++++++-- > > > > > > > > > 1 file changed, 10 insertions(+), 2 deletions(-) > > > > > > > > > > > > > > > > > > diff --git a/libavformat/avidec.c > > > > > > > > > b/libavformat/avidec.c > > > > > > > > > index 00bd7a98a9d..cfc693842b7 100644 > > > > > > > > > --- a/libavformat/avidec.c > > > > > > > > > +++ b/libavformat/avidec.c > > > > > > > > > @@ -27,6 +27,7 @@ > > > > > > > > > #include "libavutil/avstring.h" > > > > > > > > > #include "libavutil/opt.h" > > > > > > > > > #include "libavutil/dict.h" > > > > > > > > > +#include "libavutil/integer.h" > > > > > > > > > #include "libavutil/internal.h" > > > > > > > > > #include "libavutil/intreadwrite.h" > > > > > > > > > #include "libavutil/mathematics.h" > > > > > > > > > @@ -476,7 +477,7 @@ static int > > > > > > > > > calculate_bitrate(AVFormatContext *s) > > > > > > > > > AVStream *st = s->streams[i]; > > > > > > > > > FFStream *const sti = ffstream(st); > > > > > > > > > int64_t duration; > > > > > > > > > - int64_t bitrate; > > > > > > > > > + int64_t bitrate = 0; > > > > > > > > > > > > > > > > > > for (j = 0; j < sti->nb_index_entries; j++) > > > > > > > > > len += sti->index_entries[j].size; > > > > > > > > > @@ -484,7 +485,14 @@ static int > > > > > > > > > calculate_bitrate(AVFormatContext *s) > > > > > > > > > if (sti->nb_index_entries < 2 || st->codecpar- > > > > > > > > > >bit_rate > 0) > > > > > > > > > continue; > > > > > > > > > duration = sti->index_entries[j-1].timestamp - > > > > > > > > > sti->index_entries[0].timestamp; > > > > > > > > > - bitrate = av_rescale(8*len, st->time_base.den, > > > > > > > > > duration * st->time_base.num); > > > > > > > > > + if (INT64_MAX / duration >= st->time_base.num) > > > > > > > > > { > > > > > > > > > + bitrate = av_rescale(8*len, st- > > > > > > > > > >time_base.den, duration * st->time_base.num); > > > > > > > > > > > > > > > > Why not always use the AVInteger version? This is not > > > > > > > > performance sensitive > > > > > > > > as far as I see. > > > > > > > > > > > > > > We can, i will have to fix the rounding though so it > > > > > > > matches av_rescale() > > > > > > > > > > > > will apply this with just AVInteger and fixed rounding with > > > > > > my next push probably > > > > > > > > > > This seems MUCH less readable to me. > > > > > > > > we can add a av_rescale_2den() > > > > > > Won't av_rescale_q(len, (AVRational){8, duration}, st->time_base) > > > achieve the same effect? > > > > duration is 64bit AVRational is 32/32 bit, so i would expect that to > > not > > work. > > If duration was fitting in 32bit then duration * st->time_base.num > > would not > > have overflowed > > Maybe we need a 64/64-bit version of AVRational.. Iam not convinced at this point that we need that. But its not hard to do, our AVInteger code will do any size integers by just changing a single number. The real annoyance is if you have 32/32 and 64/64 rationals then suddenly you need many functions to handle both. Sofar only a fuzzed file exceeded our 64 * 64 / 64 == 64 * 32/32 * 32/32 rescale in one place adding a 64 * 64 / (64 * 64) rescale would fix that one which seems more limited in spreading complexity through the codebase thx [...]
On Tue, Oct 03, 2023 at 08:53:33PM +0200, Michael Niedermayer wrote: > On Tue, Oct 03, 2023 at 11:10:20AM +0200, Tomas Härdin wrote: > > mån 2023-10-02 klockan 21:03 +0200 skrev Michael Niedermayer: > > > On Mon, Oct 02, 2023 at 11:07:47AM +0200, Anton Khirnov wrote: > > > > Quoting Michael Niedermayer (2023-10-01 00:28:56) > > > > > On Sat, Sep 30, 2023 at 10:18:38PM +0200, Anton Khirnov wrote: > > > > > > Quoting Michael Niedermayer (2023-09-30 16:31:43) > > > > > > > On Sat, Sep 30, 2023 at 04:04:03PM +0200, Michael Niedermayer > > > > > > > wrote: > > > > > > > > On Sat, Sep 30, 2023 at 11:35:19AM +0200, Marton Balint > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > On Sat, 30 Sep 2023, Michael Niedermayer wrote: > > > > > > > > > > > > > > > > > > > Fixes: signed integer overflow: 109817402400 * > > > > > > > > > > 301990077 cannot be represented in type 'long long' > > > > > > > > > > Fixes: 51896/clusterfuzz-testcase-minimized- > > > > > > > > > > ffmpeg_dem_AVI_fuzzer-6706191715139584 > > > > > > > > > > > > > > > > > > > > Found-by: continuous fuzzing process > > > > > > > > > > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > > > > > > > > > > Signed-off-by: Michael Niedermayer > > > > > > > > > > <michael@niedermayer.cc> > > > > > > > > > > --- > > > > > > > > > > libavformat/avidec.c | 12 ++++++++++-- > > > > > > > > > > 1 file changed, 10 insertions(+), 2 deletions(-) > > > > > > > > > > > > > > > > > > > > diff --git a/libavformat/avidec.c > > > > > > > > > > b/libavformat/avidec.c > > > > > > > > > > index 00bd7a98a9d..cfc693842b7 100644 > > > > > > > > > > --- a/libavformat/avidec.c > > > > > > > > > > +++ b/libavformat/avidec.c > > > > > > > > > > @@ -27,6 +27,7 @@ > > > > > > > > > > #include "libavutil/avstring.h" > > > > > > > > > > #include "libavutil/opt.h" > > > > > > > > > > #include "libavutil/dict.h" > > > > > > > > > > +#include "libavutil/integer.h" > > > > > > > > > > #include "libavutil/internal.h" > > > > > > > > > > #include "libavutil/intreadwrite.h" > > > > > > > > > > #include "libavutil/mathematics.h" > > > > > > > > > > @@ -476,7 +477,7 @@ static int > > > > > > > > > > calculate_bitrate(AVFormatContext *s) > > > > > > > > > > AVStream *st = s->streams[i]; > > > > > > > > > > FFStream *const sti = ffstream(st); > > > > > > > > > > int64_t duration; > > > > > > > > > > - int64_t bitrate; > > > > > > > > > > + int64_t bitrate = 0; > > > > > > > > > > > > > > > > > > > > for (j = 0; j < sti->nb_index_entries; j++) > > > > > > > > > > len += sti->index_entries[j].size; > > > > > > > > > > @@ -484,7 +485,14 @@ static int > > > > > > > > > > calculate_bitrate(AVFormatContext *s) > > > > > > > > > > if (sti->nb_index_entries < 2 || st->codecpar- > > > > > > > > > > >bit_rate > 0) > > > > > > > > > > continue; > > > > > > > > > > duration = sti->index_entries[j-1].timestamp - > > > > > > > > > > sti->index_entries[0].timestamp; > > > > > > > > > > - bitrate = av_rescale(8*len, st->time_base.den, > > > > > > > > > > duration * st->time_base.num); > > > > > > > > > > + if (INT64_MAX / duration >= st->time_base.num) > > > > > > > > > > { > > > > > > > > > > + bitrate = av_rescale(8*len, st- > > > > > > > > > > >time_base.den, duration * st->time_base.num); > > > > > > > > > > > > > > > > > > Why not always use the AVInteger version? This is not > > > > > > > > > performance sensitive > > > > > > > > > as far as I see. > > > > > > > > > > > > > > > > We can, i will have to fix the rounding though so it > > > > > > > > matches av_rescale() > > > > > > > > > > > > > > will apply this with just AVInteger and fixed rounding with > > > > > > > my next push probably > > > > > > > > > > > > This seems MUCH less readable to me. > > > > > > > > > > we can add a av_rescale_2den() > > > > > > > > Won't av_rescale_q(len, (AVRational){8, duration}, st->time_base) > > > > achieve the same effect? > > > > > > duration is 64bit AVRational is 32/32 bit, so i would expect that to > > > not > > > work. > > > If duration was fitting in 32bit then duration * st->time_base.num > > > would not > > > have overflowed > > > > Maybe we need a 64/64-bit version of AVRational.. > > Iam not convinced at this point that we need that. > But its not hard to do, our AVInteger code will do any size integers by just > changing a single number. > > The real annoyance is if you have 32/32 and 64/64 rationals then suddenly > you need many functions to handle both. Sofar only a fuzzed file exceeded > our 64 * 64 / 64 == 64 * 32/32 * 32/32 rescale in one place > adding a 64 * 64 / (64 * 64) rescale would fix that one which seems more > limited in spreading complexity through the codebase Ill apply this below, because its undefined behavior and needs to be fixed. If someone has a better solution please replace my code with it. @@ -476,7 +477,7 @@ static int calculate_bitrate(AVFormatContext *s) AVStream *st = s->streams[i]; FFStream *const sti = ffstream(st); int64_t duration; - int64_t bitrate; + AVInteger bitrate_i, den_i, num_i; for (j = 0; j < sti->nb_index_entries; j++) len += sti->index_entries[j].size; @@ -484,9 +485,14 @@ static int calculate_bitrate(AVFormatContext *s) if (sti->nb_index_entries < 2 || st->codecpar->bit_rate > 0) continue; duration = sti->index_entries[j-1].timestamp - sti->index_entries[0].timestamp; - bitrate = av_rescale(8*len, st->time_base.den, duration * st->time_base.num); - if (bitrate > 0) { - st->codecpar->bit_rate = bitrate; + den_i = av_mul_i(av_int2i(duration), av_int2i(st->time_base.num)); + num_i = av_add_i(av_mul_i(av_int2i(8*len), av_int2i(st->time_base.den)), av_shr_i(den_i, 1)); + bitrate_i = av_div_i(num_i, den_i); + if (av_cmp_i(bitrate_i, av_int2i(INT64_MAX)) <= 0) { + int64_t bitrate = av_i2int(bitrate_i); + if (bitrate > 0) { + st->codecpar->bit_rate = bitrate; + } } } return 1; [...]
diff --git a/libavformat/avidec.c b/libavformat/avidec.c index 00bd7a98a9d..cfc693842b7 100644 --- a/libavformat/avidec.c +++ b/libavformat/avidec.c @@ -27,6 +27,7 @@ #include "libavutil/avstring.h" #include "libavutil/opt.h" #include "libavutil/dict.h" +#include "libavutil/integer.h" #include "libavutil/internal.h" #include "libavutil/intreadwrite.h" #include "libavutil/mathematics.h" @@ -476,7 +477,7 @@ static int calculate_bitrate(AVFormatContext *s) AVStream *st = s->streams[i]; FFStream *const sti = ffstream(st); int64_t duration; - int64_t bitrate; + int64_t bitrate = 0; for (j = 0; j < sti->nb_index_entries; j++) len += sti->index_entries[j].size; @@ -484,7 +485,14 @@ static int calculate_bitrate(AVFormatContext *s) if (sti->nb_index_entries < 2 || st->codecpar->bit_rate > 0) continue; duration = sti->index_entries[j-1].timestamp - sti->index_entries[0].timestamp; - bitrate = av_rescale(8*len, st->time_base.den, duration * st->time_base.num); + if (INT64_MAX / duration >= st->time_base.num) { + bitrate = av_rescale(8*len, st->time_base.den, duration * st->time_base.num); + } else { + AVInteger bitrate_i = av_div_i(av_mul_i(av_int2i(8*len), av_int2i(st->time_base.den)), + av_mul_i(av_int2i(duration), av_int2i(st->time_base.num))); + if (av_cmp_i(bitrate_i, av_int2i(INT64_MAX)) <= 0) + bitrate = av_i2int(bitrate_i); + } if (bitrate > 0) { st->codecpar->bit_rate = bitrate; }
Fixes: signed integer overflow: 109817402400 * 301990077 cannot be represented in type 'long long' Fixes: 51896/clusterfuzz-testcase-minimized-ffmpeg_dem_AVI_fuzzer-6706191715139584 Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavformat/avidec.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)