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

[Michael Niedermayer]

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


[...]
-- 
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.
-------------- 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/20231003/7e7d03b2/attachment.sig>