[FFmpeg-devel] [PATCH 1/6] avformat/avidec: support huge durations

[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 at 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

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

If the United States is serious about tackling the national security threats 
related to an insecure 5G network, it needs to rethink the extent to which it
values corporate profits and government espionage over security.-Bruce Schneier
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20231002/ce56bb89/attachment.sig>