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

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

[Michael Niedermayer]

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);
+        } 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".

[FFmpeg-devel] [PATCH 2/6] avformat/avidec: Correct unsigend type

[Michael Niedermayer]

Fixes: signed integer overflow: 9223372036854775807 + 1 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 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libavformat/avidec.c b/libavformat/avidec.c
index cfc693842b7..95a6cbe930e 100644
--- a/libavformat/avidec.c
+++ b/libavformat/avidec.c
@@ -1704,7 +1704,7 @@ static int check_stream_max_drift(AVFormatContext *s)
     int *idx = av_calloc(s->nb_streams, sizeof(*idx));
     if (!idx)
         return AVERROR(ENOMEM);
-    for (min_pos = pos = 0; min_pos != INT64_MAX; pos = min_pos + 1LU) {
+    for (min_pos = pos = 0; min_pos != INT64_MAX; pos = min_pos + 1llu) {
         int64_t max_dts = INT64_MIN / 2;
         int64_t min_dts = INT64_MAX / 2;
         int64_t max_buffer = 0;
-- 
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".

[FFmpeg-devel] [PATCH 3/6] vformat/cafdec: dont seek beyond 64bit

[Michael Niedermayer]

Fixes: signed integer overflow: 64 + 9223372036854775807 cannot be represented in type 'long long'
Fixes: 51896/clusterfuzz-testcase-minimized-ffmpeg_dem_CAF_fuzzer-6418242730328064

Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 libavformat/cafdec.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libavformat/cafdec.c b/libavformat/cafdec.c
index f5ba0f41084..e92e3279fc6 100644
--- a/libavformat/cafdec.c
+++ b/libavformat/cafdec.c
@@ -271,7 +271,7 @@ static int read_pakt_chunk(AVFormatContext *s, int64_t size)
         }
     }
 
-    if (avio_tell(pb) - ccount > size) {
+    if (avio_tell(pb) - ccount > size || size > INT64_MAX - ccount) {
         av_log(s, AV_LOG_ERROR, "error reading packet table\n");
         return AVERROR_INVALIDDATA;
     }
-- 
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".

[FFmpeg-devel] [PATCH 4/6] avformat/cafdec: saturate start + size into 64bit

[Michael Niedermayer]

Fixes: signed integer overflow: 64 + 9223372036854775803 cannot be represented in type 'long long'
Fixes: 51896/clusterfuzz-testcase-minimized-ffmpeg_dem_CAF_fuzzer-6536881135550464

Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 libavformat/cafdec.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libavformat/cafdec.c b/libavformat/cafdec.c
index e92e3279fc6..0e50a3cfe68 100644
--- a/libavformat/cafdec.c
+++ b/libavformat/cafdec.c
@@ -435,7 +435,7 @@ static int read_packet(AVFormatContext *s, AVPacket *pkt)
 
     /* don't read past end of data chunk */
     if (caf->data_size > 0) {
-        left = (caf->data_start + caf->data_size) - avio_tell(pb);
+        left = av_sat_add64(caf->data_start, caf->data_size) - avio_tell(pb);
         if (!left)
             return AVERROR_EOF;
         if (left < 0)
-- 
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".

Re: [FFmpeg-devel] [PATCH 4/6] avformat/cafdec: saturate start + size into 64bit

[James Almer]

On 9/29/2023 8:19 PM, Michael Niedermayer wrote:
> Fixes: signed integer overflow: 64 + 9223372036854775803 cannot be represented in type 'long long'
> Fixes: 51896/clusterfuzz-testcase-minimized-ffmpeg_dem_CAF_fuzzer-6536881135550464
> 
> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>   libavformat/cafdec.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/libavformat/cafdec.c b/libavformat/cafdec.c
> index e92e3279fc6..0e50a3cfe68 100644
> --- a/libavformat/cafdec.c
> +++ b/libavformat/cafdec.c
> @@ -435,7 +435,7 @@ static int read_packet(AVFormatContext *s, AVPacket *pkt)
>   
>       /* don't read past end of data chunk */
>       if (caf->data_size > 0) {
> -        left = (caf->data_start + caf->data_size) - avio_tell(pb);
> +        left = av_sat_add64(caf->data_start, caf->data_size) - avio_tell(pb);

avio_tell(pb) here is guaranteed to be bigger than caf->data_start, 
which is the offset where the DATA chunk starts, so the result of this 
calculation will be <= INT64_MAX even if you don't saturate it and 
instead cast it to uint64_t. Nonetheless, if the DATA chunk ends at an 
offset that would not fit in an int64_t, then we can't parse it to begin 
with due to AVIOContext API limitations.

Maybe we should just abort in read_header() if data_start + data_size > 
INT64_MAX, instead of pretending we can parse the file, invalid or not, 
by saturating values.

>           if (!left)
>               return AVERROR_EOF;
>           if (left < 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".

Re: [FFmpeg-devel] [PATCH 4/6] avformat/cafdec: saturate start + size into 64bit

[Michael Niedermayer]

[-- Attachment #1.1: Type: text/plain, Size: 1903 bytes --]

On Fri, Sep 29, 2023 at 08:41:23PM -0300, James Almer wrote:
> On 9/29/2023 8:19 PM, Michael Niedermayer wrote:
> > Fixes: signed integer overflow: 64 + 9223372036854775803 cannot be represented in type 'long long'
> > Fixes: 51896/clusterfuzz-testcase-minimized-ffmpeg_dem_CAF_fuzzer-6536881135550464
> > 
> > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > ---
> >   libavformat/cafdec.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/libavformat/cafdec.c b/libavformat/cafdec.c
> > index e92e3279fc6..0e50a3cfe68 100644
> > --- a/libavformat/cafdec.c
> > +++ b/libavformat/cafdec.c
> > @@ -435,7 +435,7 @@ static int read_packet(AVFormatContext *s, AVPacket *pkt)
> >       /* don't read past end of data chunk */
> >       if (caf->data_size > 0) {
> > -        left = (caf->data_start + caf->data_size) - avio_tell(pb);
> > +        left = av_sat_add64(caf->data_start, caf->data_size) - avio_tell(pb);
> 
> avio_tell(pb) here is guaranteed to be bigger than caf->data_start, which is
> the offset where the DATA chunk starts, so the result of this calculation
> will be <= INT64_MAX even if you don't saturate it and instead cast it to
> uint64_t. Nonetheless, if the DATA chunk ends at an offset that would not
> fit in an int64_t, then we can't parse it to begin with due to AVIOContext
> API limitations.
> 

> Maybe we should just abort in read_header() if data_start + data_size >
> INT64_MAX, instead of pretending we can parse the file, invalid or not, by
> saturating values.

yes ill try that

thx

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

If you think the mosad wants you dead since a long time then you are either
wrong or dead since a long time.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH 4/6] avformat/cafdec: saturate start + size into 64bit

[Michael Niedermayer]

[-- Attachment #1.1: Type: text/plain, Size: 2924 bytes --]

On Sat, Sep 30, 2023 at 06:32:07PM +0200, Michael Niedermayer wrote:
> On Fri, Sep 29, 2023 at 08:41:23PM -0300, James Almer wrote:
> > On 9/29/2023 8:19 PM, Michael Niedermayer wrote:
> > > Fixes: signed integer overflow: 64 + 9223372036854775803 cannot be represented in type 'long long'
> > > Fixes: 51896/clusterfuzz-testcase-minimized-ffmpeg_dem_CAF_fuzzer-6536881135550464
> > > 
> > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > > ---
> > >   libavformat/cafdec.c | 2 +-
> > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/libavformat/cafdec.c b/libavformat/cafdec.c
> > > index e92e3279fc6..0e50a3cfe68 100644
> > > --- a/libavformat/cafdec.c
> > > +++ b/libavformat/cafdec.c
> > > @@ -435,7 +435,7 @@ static int read_packet(AVFormatContext *s, AVPacket *pkt)
> > >       /* don't read past end of data chunk */
> > >       if (caf->data_size > 0) {
> > > -        left = (caf->data_start + caf->data_size) - avio_tell(pb);
> > > +        left = av_sat_add64(caf->data_start, caf->data_size) - avio_tell(pb);
> > 
> > avio_tell(pb) here is guaranteed to be bigger than caf->data_start, which is
> > the offset where the DATA chunk starts, so the result of this calculation
> > will be <= INT64_MAX even if you don't saturate it and instead cast it to
> > uint64_t. Nonetheless, if the DATA chunk ends at an offset that would not
> > fit in an int64_t, then we can't parse it to begin with due to AVIOContext
> > API limitations.
> > 
> 
> > Maybe we should just abort in read_header() if data_start + data_size >
> > INT64_MAX, instead of pretending we can parse the file, invalid or not, by
> > saturating values.
> 
> yes ill try that

ossfuzz moved the testcases all to another unrelated issues (62276)

ill apply this as suggested with the check in read_header() below:

diff --git a/libavformat/cafdec.c b/libavformat/cafdec.c
index 72809fd1de2..07a2939a7a6 100644
--- a/libavformat/cafdec.c
+++ b/libavformat/cafdec.c
@@ -343,6 +343,9 @@ static int read_header(AVFormatContext *s)
             avio_skip(pb, 4); /* edit count */
             caf->data_start = avio_tell(pb);
             caf->data_size  = size < 0 ? -1 : size - 4;
+            if (caf->data_start < 0 || caf->data_size > INT64_MAX - caf->data_start)
+                return AVERROR_INVALIDDATA;
+
             if (caf->data_size > 0 && (pb->seekable & AVIO_SEEKABLE_NORMAL))
                 avio_skip(pb, caf->data_size);
             found_data = 1;

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Concerning the gods, I have no means of knowing whether they exist or not
or of what sort they may be, because of the obscurity of the subject, and
the brevity of human life -- Protagoras

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
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".

[FFmpeg-devel] [PATCH 5/6] avformat/dxa: Adjust order of operations around block align

[Michael Niedermayer]

Fixes: 51896/clusterfuzz-testcase-minimized-ffmpeg_dem_DXA_fuzzer-5730576523198464
Fixes: signed integer overflow: 2147483566 + 82 cannot be represented in type 'int'

Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 libavformat/dxa.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libavformat/dxa.c b/libavformat/dxa.c
index 474b85270ae..b4d9d005290 100644
--- a/libavformat/dxa.c
+++ b/libavformat/dxa.c
@@ -122,7 +122,7 @@ static int dxa_read_header(AVFormatContext *s)
         if(ast->codecpar->block_align) {
             if (c->bpc > INT_MAX - ast->codecpar->block_align + 1)
                 return AVERROR_INVALIDDATA;
-            c->bpc = ((c->bpc + ast->codecpar->block_align - 1) / ast->codecpar->block_align) * ast->codecpar->block_align;
+            c->bpc = ((c->bpc - 1 + ast->codecpar->block_align) / ast->codecpar->block_align) * ast->codecpar->block_align;
         }
         c->bytes_left = fsize;
         c->wavpos = avio_tell(pb);
-- 
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".

[FFmpeg-devel] [PATCH 6/6] avformat/iff: Saturate avio_tell() + 12

[Michael Niedermayer]

Fixes: signed integer overflow: 9223372036854775796 + 12 cannot be represented in type 'long long'
Fixes: 51896/clusterfuzz-testcase-minimized-ffmpeg_dem_IFF_fuzzer-4898373660704768

Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 libavformat/iff.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/libavformat/iff.c b/libavformat/iff.c
index b8e8bffe03f..5bff0e9b6c1 100644
--- a/libavformat/iff.c
+++ b/libavformat/iff.c
@@ -217,7 +217,7 @@ static int parse_dsd_diin(AVFormatContext *s, AVStream *st, uint64_t eof)
 {
     AVIOContext *pb = s->pb;
 
-    while (avio_tell(pb) + 12 <= eof && !avio_feof(pb)) {
+    while (av_sat_add64(avio_tell(pb), 12) <= eof && !avio_feof(pb)) {
         uint32_t tag      = avio_rl32(pb);
         uint64_t size     = avio_rb64(pb);
         uint64_t orig_pos = avio_tell(pb);
@@ -254,7 +254,7 @@ static int parse_dsd_prop(AVFormatContext *s, AVStream *st, uint64_t eof)
     int dsd_layout[6];
     ID3v2ExtraMeta *id3v2_extra_meta;
 
-    while (avio_tell(pb) + 12 <= eof && !avio_feof(pb)) {
+    while (av_sat_add64(avio_tell(pb), 12) <= eof && !avio_feof(pb)) {
         uint32_t tag      = avio_rl32(pb);
         uint64_t size     = avio_rb64(pb);
         uint64_t orig_pos = avio_tell(pb);
-- 
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".

Re: [FFmpeg-devel] [PATCH 6/6] avformat/iff: Saturate avio_tell() + 12

[Michael Niedermayer]

[-- Attachment #1.1: Type: text/plain, Size: 857 bytes --]

On Sat, Sep 30, 2023 at 01:20:01AM +0200, Michael Niedermayer wrote:
> Fixes: signed integer overflow: 9223372036854775796 + 12 cannot be represented in type 'long long'
> Fixes: 51896/clusterfuzz-testcase-minimized-ffmpeg_dem_IFF_fuzzer-4898373660704768
> 
> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavformat/iff.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

will apply the patches from this patchset that have received no comments

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Homeopathy is like voting while filling the ballot out with transparent ink.
Sometimes the outcome one wanted occurs. Rarely its worse than filling out
a ballot properly.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
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".

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

[Marton Balint]

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".
>
_______________________________________________
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".

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

[Michael Niedermayer]

[-- Attachment #1.1: Type: text/plain, Size: 2413 bytes --]

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

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Let us carefully observe those good qualities wherein our enemies excel us
and endeavor to excel them, by avoiding what is faulty, and imitating what
is excellent in them. -- Plutarch

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
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".

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

[Michael Niedermayer]

[-- Attachment #1.1: Type: text/plain, Size: 2623 bytes --]

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

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

If a bugfix only changes things apparently unrelated to the bug with no
further explanation, that is a good sign that the bugfix is wrong.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
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".

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

[Anton Khirnov]

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?

-- 
Anton Khirnov
_______________________________________________
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".

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

[Michael Niedermayer]

[-- Attachment #1.1: Type: text/plain, Size: 3494 bytes --]

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

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

I know you won't believe me, but the highest form of Human Excellence is
to question oneself and others. -- Socrates

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
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".

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

[Anton Khirnov]

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?

-- 
Anton Khirnov
_______________________________________________
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".

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

[Michael Niedermayer]

[-- Attachment #1.1: Type: text/plain, Size: 3736 bytes --]

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

[...]

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

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
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".

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

[Tomas Härdin]

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
_______________________________________________
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".

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

[Michael Niedermayer]

[-- Attachment #1.1: Type: text/plain, Size: 4941 bytes --]

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


[...]
-- 
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.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
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".

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

[Michael Niedermayer]

[-- Attachment #1.1: Type: text/plain, Size: 6596 bytes --]

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;

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

The bravest are surely those who have the clearest vision
of what is before them, glory and danger alike, and yet
notwithstanding go out to meet it. -- Thucydides

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
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".