[FFmpeg-devel] [PATCH 1/5] avcodec/wavarc: fix signed integer overflow in block type 6/19

[FFmpeg-devel] [PATCH 1/5] avcodec/wavarc: fix signed integer overflow in block type 6/19

[Michael Niedermayer]

Fixes: signed integer overflow: -2088796289 + -91276551 cannot be represented in type 'int'
Fixes: 67772/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_WAVARC_fuzzer-6533568953122816

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

diff --git a/libavcodec/wavarc.c b/libavcodec/wavarc.c
index 7083494cd81..b4b26958e6f 100644
--- a/libavcodec/wavarc.c
+++ b/libavcodec/wavarc.c
@@ -647,7 +647,7 @@ static int decode_5elp(AVCodecContext *avctx,
                 for (int o = 0; o < order; o++)
                     sum += s->filter[ch][o] * (unsigned)samples[n + 70 - o - 1];
 
-                samples[n + 70] += ac_out[n] + (sum >> 4);
+                samples[n + 70] += ac_out[n] + (unsigned)(sum >> 4);
             }
 
             for (int n = 0; n < 70; n++)
-- 
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/5] avformat/iamf_parse: Check sound_system

[Michael Niedermayer]

Fixes: index 13 out of bounds for type 'const struct IAMFSoundSystemMap [13]'
Fixes: 67796/clusterfuzz-testcase-minimized-ffmpeg_dem_IAMF_fuzzer-4554553191104512

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

diff --git a/libavformat/iamf_parse.c b/libavformat/iamf_parse.c
index 3867adb1172..f8074c2de1c 100644
--- a/libavformat/iamf_parse.c
+++ b/libavformat/iamf_parse.c
@@ -934,6 +934,10 @@ static int mix_presentation_obu(void *s, IAMFContext *c, AVIOContext *pb, int le
             if (submix_layout->layout_type == 2) {
                 int sound_system;
                 sound_system = (byte >> 2) & 0xF;
+                if (sound_system >= FF_ARRAY_ELEMS(ff_iamf_sound_system_map)) {
+                    ret = AVERROR_INVALIDDATA;
+                    goto fail;
+                }
                 av_channel_layout_copy(&submix_layout->sound_system, &ff_iamf_sound_system_map[sound_system].layout);
             }
 
-- 
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/5] swscale/utils: Fix xInc overflow

[Michael Niedermayer]

Fixes: signed integer overflow: 2 * 1073741824 cannot be represented in type 'int'
Fixes: 67802/clusterfuzz-testcase-minimized-ffmpeg_SWS_fuzzer-6249515855183872

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

diff --git a/libswscale/utils.c b/libswscale/utils.c
index d34c8d1641e..df14eb016ce 100644
--- a/libswscale/utils.c
+++ b/libswscale/utils.c
@@ -593,7 +593,7 @@ static av_cold int initFilter(int16_t **outFilter, int32_t **filterPos,
                 filter[i * filterSize + j] = coeff;
                 xx++;
             }
-            xDstInSrc += 2 * xInc;
+            xDstInSrc += 2LL * xInc;
         }
     }
 
-- 
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/5] avformat/mxfdec: Check index_edit_rate

[Michael Niedermayer]

Fixes: Assertion b >=0 failed at libavutil/mathematics.c:62
Fixes: 67811/clusterfuzz-testcase-minimized-ffmpeg_dem_MXF_fuzzer-5108429687422976

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

diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c
index 04de4c1d5e3..233d614f783 100644
--- a/libavformat/mxfdec.c
+++ b/libavformat/mxfdec.c
@@ -1264,6 +1264,9 @@ static int mxf_read_index_table_segment(void *arg, AVIOContext *pb, int tag, int
     case 0x3F0B:
         segment->index_edit_rate.num = avio_rb32(pb);
         segment->index_edit_rate.den = avio_rb32(pb);
+        if (segment->index_edit_rate.num <= 0 ||
+            segment->index_edit_rate.den <= 0)
+            return AVERROR_INVALIDDATA;
         av_log(NULL, AV_LOG_TRACE, "IndexEditRate %d/%d\n", segment->index_edit_rate.num,
                 segment->index_edit_rate.den);
         break;
-- 
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/5] avformat/mxfdec: Check index_edit_rate

[Tomas Härdin]

tor 2024-04-04 klockan 00:51 +0200 skrev Michael Niedermayer:
> Fixes: Assertion b >=0 failed at libavutil/mathematics.c:62
> Fixes: 67811/clusterfuzz-testcase-minimized-ffmpeg_dem_MXF_fuzzer-
> 5108429687422976
> 
> Found-by: continuous fuzzing process
> https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavformat/mxfdec.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c
> index 04de4c1d5e3..233d614f783 100644
> --- a/libavformat/mxfdec.c
> +++ b/libavformat/mxfdec.c
> @@ -1264,6 +1264,9 @@ static int mxf_read_index_table_segment(void
> *arg, AVIOContext *pb, int tag, int
>      case 0x3F0B:
>          segment->index_edit_rate.num = avio_rb32(pb);
>          segment->index_edit_rate.den = avio_rb32(pb);
> +        if (segment->index_edit_rate.num <= 0 ||
> +            segment->index_edit_rate.den <= 0)
> +            return AVERROR_INVALIDDATA;

mxf_compute_index_tables() has a check for index_edit_rate that you
probably want to remove as well. It was introduced in c6fff3d, but the
files it supposedly fixes aren't in FATE. We shouldn't encourage broken
muxers.

/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 4/5] avformat/mxfdec: Check index_edit_rate

[Marton Balint]

On Mon, 8 Apr 2024, Tomas Härdin wrote:

> tor 2024-04-04 klockan 00:51 +0200 skrev Michael Niedermayer:
>> Fixes: Assertion b >=0 failed at libavutil/mathematics.c:62
>> Fixes: 67811/clusterfuzz-testcase-minimized-ffmpeg_dem_MXF_fuzzer-
>> 5108429687422976
>> 
>> Found-by: continuous fuzzing process
>> https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
>> ---
>>  libavformat/mxfdec.c | 3 +++
>>  1 file changed, 3 insertions(+)
>> 
>> diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c
>> index 04de4c1d5e3..233d614f783 100644
>> --- a/libavformat/mxfdec.c
>> +++ b/libavformat/mxfdec.c
>> @@ -1264,6 +1264,9 @@ static int mxf_read_index_table_segment(void
>> *arg, AVIOContext *pb, int tag, int
>>      case 0x3F0B:
>>          segment->index_edit_rate.num = avio_rb32(pb);
>>          segment->index_edit_rate.den = avio_rb32(pb);
>> +        if (segment->index_edit_rate.num <= 0 ||
>> +            segment->index_edit_rate.den <= 0)
>> +            return AVERROR_INVALIDDATA;
>
> mxf_compute_index_tables() has a check for index_edit_rate that you
> probably want to remove as well. It was introduced in c6fff3d, but the
> files it supposedly fixes aren't in FATE. We shouldn't encourage broken
> muxers.

I don't quite get what FATE has to do with it. And the samples mentioned 
in the patch has valid index segment edit rates, only they are different 
from the track edit rate, and the patch was intended to fix that case.

Regards,
Marton
_______________________________________________
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/5] avformat/mxfdec: Check index_edit_rate

[Tomas Härdin]

mån 2024-04-08 klockan 21:46 +0200 skrev Marton Balint:
> 
> 
> On Mon, 8 Apr 2024, Tomas Härdin wrote:
> 
> > tor 2024-04-04 klockan 00:51 +0200 skrev Michael Niedermayer:
> > > Fixes: Assertion b >=0 failed at libavutil/mathematics.c:62
> > > Fixes: 67811/clusterfuzz-testcase-minimized-
> > > ffmpeg_dem_MXF_fuzzer-
> > > 5108429687422976
> > > 
> > > Found-by: continuous fuzzing process
> > > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > > ---
> > >  libavformat/mxfdec.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > > 
> > > diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c
> > > index 04de4c1d5e3..233d614f783 100644
> > > --- a/libavformat/mxfdec.c
> > > +++ b/libavformat/mxfdec.c
> > > @@ -1264,6 +1264,9 @@ static int
> > > mxf_read_index_table_segment(void
> > > *arg, AVIOContext *pb, int tag, int
> > >      case 0x3F0B:
> > >          segment->index_edit_rate.num = avio_rb32(pb);
> > >          segment->index_edit_rate.den = avio_rb32(pb);
> > > +        if (segment->index_edit_rate.num <= 0 ||
> > > +            segment->index_edit_rate.den <= 0)
> > > +            return AVERROR_INVALIDDATA;
> > 
> > mxf_compute_index_tables() has a check for index_edit_rate that you
> > probably want to remove as well. It was introduced in c6fff3d, but
> > the
> > files it supposedly fixes aren't in FATE. We shouldn't encourage
> > broken
> > muxers.
> 
> I don't quite get what FATE has to do with it. And the samples
> mentioned 
> in the patch has valid index segment edit rates, only they are
> different 
> from the track edit rate, and the patch was intended to fix that
> case.

Then why does it check against 0/0?

/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 4/5] avformat/mxfdec: Check index_edit_rate

[Marton Balint]

On Tue, 9 Apr 2024, Tomas Härdin wrote:

> mån 2024-04-08 klockan 21:46 +0200 skrev Marton Balint:
>> 
>> 
>> On Mon, 8 Apr 2024, Tomas Härdin wrote:
>> 
>> > tor 2024-04-04 klockan 00:51 +0200 skrev Michael Niedermayer:
>> > > Fixes: Assertion b >=0 failed at libavutil/mathematics.c:62
>> > > Fixes: 67811/clusterfuzz-testcase-minimized-
>> > > ffmpeg_dem_MXF_fuzzer-
>> > > 5108429687422976
>> > > 
>> > > Found-by: continuous fuzzing process
>> > > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
>> > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
>> > > ---
>> > >  libavformat/mxfdec.c | 3 +++
>> > >  1 file changed, 3 insertions(+)
>> > > 
>> > > diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c
>> > > index 04de4c1d5e3..233d614f783 100644
>> > > --- a/libavformat/mxfdec.c
>> > > +++ b/libavformat/mxfdec.c
>> > > @@ -1264,6 +1264,9 @@ static int
>> > > mxf_read_index_table_segment(void
>> > > *arg, AVIOContext *pb, int tag, int
>> > >      case 0x3F0B:
>> > >          segment->index_edit_rate.num = avio_rb32(pb);
>> > >          segment->index_edit_rate.den = avio_rb32(pb);
>> > > +        if (segment->index_edit_rate.num <= 0 ||
>> > > +            segment->index_edit_rate.den <= 0)
>> > > +            return AVERROR_INVALIDDATA;
>> > 
>> > mxf_compute_index_tables() has a check for index_edit_rate that you
>> > probably want to remove as well. It was introduced in c6fff3d, but
>> > the
>> > files it supposedly fixes aren't in FATE. We shouldn't encourage
>> > broken
>> > muxers.
>> 
>> I don't quite get what FATE has to do with it. And the samples
>> mentioned 
>> in the patch has valid index segment edit rates, only they are
>> different 
>> from the track edit rate, and the patch was intended to fix that
>> case.
>
> Then why does it check against 0/0?

Probably to avoid divison by zero.

Regards,
Marton
_______________________________________________
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/5] avformat/mxfdec: Check index_edit_rate

[Tomas Härdin]

tis 2024-04-09 klockan 22:58 +0200 skrev Marton Balint:
> 
> 
> On Tue, 9 Apr 2024, Tomas Härdin wrote:
> 
> > mån 2024-04-08 klockan 21:46 +0200 skrev Marton Balint:
> > > 
> > > 
> > > On Mon, 8 Apr 2024, Tomas Härdin wrote:
> > > 
> > > > tor 2024-04-04 klockan 00:51 +0200 skrev Michael Niedermayer:
> > > > > Fixes: Assertion b >=0 failed at libavutil/mathematics.c:62
> > > > > Fixes: 67811/clusterfuzz-testcase-minimized-
> > > > > ffmpeg_dem_MXF_fuzzer-
> > > > > 5108429687422976
> > > > > 
> > > > > Found-by: continuous fuzzing process
> > > > > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > > > > ---
> > > > >  libavformat/mxfdec.c | 3 +++
> > > > >  1 file changed, 3 insertions(+)
> > > > > 
> > > > > diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c
> > > > > index 04de4c1d5e3..233d614f783 100644
> > > > > --- a/libavformat/mxfdec.c
> > > > > +++ b/libavformat/mxfdec.c
> > > > > @@ -1264,6 +1264,9 @@ static int
> > > > > mxf_read_index_table_segment(void
> > > > > *arg, AVIOContext *pb, int tag, int
> > > > >      case 0x3F0B:
> > > > >          segment->index_edit_rate.num = avio_rb32(pb);
> > > > >          segment->index_edit_rate.den = avio_rb32(pb);
> > > > > +        if (segment->index_edit_rate.num <= 0 ||
> > > > > +            segment->index_edit_rate.den <= 0)
> > > > > +            return AVERROR_INVALIDDATA;
> > > > 
> > > > mxf_compute_index_tables() has a check for index_edit_rate that
> > > > you
> > > > probably want to remove as well. It was introduced in c6fff3d,
> > > > but
> > > > the
> > > > files it supposedly fixes aren't in FATE. We shouldn't
> > > > encourage
> > > > broken
> > > > muxers.
> > > 
> > > I don't quite get what FATE has to do with it. And the samples
> > > mentioned 
> > > in the patch has valid index segment edit rates, only they are
> > > different 
> > > from the track edit rate, and the patch was intended to fix that
> > > case.
> > 
> > Then why does it check against 0/0?
> 
> Probably to avoid divison by zero.

I think it's safe to say that EditRates with zero in the numerator or
denominator are not allowed. We currently default to 25/1 in this case
for Tracks, but I am skeptical of this since it encourages broken
muxers.

As for IndexEditRate, here's what ST 377-1:2019 has to say:


> Edit Rate copied from the Essence Tracks of the
> Essence Container
> [Note: SMPTE RP 210 definition Specifies the
> indexing rate in hertz]

It's possible to encode a file that does not specify IndexEditRate, but
this is not allowed since the field is marked Required in Table 26.
mxfdec.c will default to 0/0 since the segment is calloc'd. Michael's
fix won't work if one changes the IndexEditRate local tag in the file
to something else, say FFFF instead of 3F0B.

In short, IndexEditRate MUST be set and it MUST equal the EditRate of
the associated Essence Track (confusingly called source_track in the
code). Section 11 is even more explicit:

> An Index Table shall be used to index a single Essence Container.
> Each Index Table shall index Edit Units
> stored Essence of the Essence Container. The Edit Unit rate of an
> Index Table is defined by the Edit Rate of the
> Essence Tracks of the Package that describes the Essence Container
> that the Index Table indexes.

EditRate MAY be different between MaterialPackage and FilePackage
however. This is a consequence of MXF's AAF heritage. MXF is really an
NLE format.

Section 11.6.2 "Look-up Algorithm for Conversion of Index Position to
Stream Offset" is also of relevance. It doesn't make use of
IndexEditRate at all.

/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 4/5] avformat/mxfdec: Check index_edit_rate

[Marton Balint]

On Wed, 10 Apr 2024, Tomas Härdin wrote:

> tis 2024-04-09 klockan 22:58 +0200 skrev Marton Balint:
>> 
>> 
>> On Tue, 9 Apr 2024, Tomas Härdin wrote:
>> 
>> > mån 2024-04-08 klockan 21:46 +0200 skrev Marton Balint:
>> > > 
>> > > 
>> > > On Mon, 8 Apr 2024, Tomas Härdin wrote:
>> > > 
>> > > > tor 2024-04-04 klockan 00:51 +0200 skrev Michael Niedermayer:
>> > > > > Fixes: Assertion b >=0 failed at libavutil/mathematics.c:62
>> > > > > Fixes: 67811/clusterfuzz-testcase-minimized-
>> > > > > ffmpeg_dem_MXF_fuzzer-
>> > > > > 5108429687422976
>> > > > > 
>> > > > > Found-by: continuous fuzzing process
>> > > > > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
>> > > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
>> > > > > ---
>> > > > >  libavformat/mxfdec.c | 3 +++
>> > > > >  1 file changed, 3 insertions(+)
>> > > > > 
>> > > > > diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c
>> > > > > index 04de4c1d5e3..233d614f783 100644
>> > > > > --- a/libavformat/mxfdec.c
>> > > > > +++ b/libavformat/mxfdec.c
>> > > > > @@ -1264,6 +1264,9 @@ static int
>> > > > > mxf_read_index_table_segment(void
>> > > > > *arg, AVIOContext *pb, int tag, int
>> > > > >      case 0x3F0B:
>> > > > >          segment->index_edit_rate.num = avio_rb32(pb);
>> > > > >          segment->index_edit_rate.den = avio_rb32(pb);
>> > > > > +        if (segment->index_edit_rate.num <= 0 ||
>> > > > > +            segment->index_edit_rate.den <= 0)
>> > > > > +            return AVERROR_INVALIDDATA;
>> > > > 
>> > > > mxf_compute_index_tables() has a check for index_edit_rate that
>> > > > you
>> > > > probably want to remove as well. It was introduced in c6fff3d,
>> > > > but
>> > > > the
>> > > > files it supposedly fixes aren't in FATE. We shouldn't
>> > > > encourage
>> > > > broken
>> > > > muxers.
>> > > 
>> > > I don't quite get what FATE has to do with it. And the samples
>> > > mentioned 
>> > > in the patch has valid index segment edit rates, only they are
>> > > different 
>> > > from the track edit rate, and the patch was intended to fix that
>> > > case.
>> > 
>> > Then why does it check against 0/0?
>> 
>> Probably to avoid divison by zero.
>
> I think it's safe to say that EditRates with zero in the numerator or
> denominator are not allowed. We currently default to 25/1 in this case
> for Tracks, but I am skeptical of this since it encourages broken
> muxers.

In general, I don't like the idea of rejecting everything which is not 
following the standard to the letter. Decoding and demuxing should be 
based on the "Robustness principle", as in being liberal in what we accept 
and strict in what we generate.

I am also not sure about your reasoning that rejecting files will force 
vendors to fix their muxers, because the users will have to pay the price 
for this approach. Users may well already have their archives full of 
non-compliant files, their camera, phone, whatever is likely out of 
warranty/support, so they might not even be in a position to request 
anything from vendors.

Sure, I get it, some issues cannot be worked around easily, and I am not 
saying that everything must be supported with huge hacks if needed. But an 
effort should be made to not break existing real files, and support what 
we reasonably can.

>
> As for IndexEditRate, here's what ST 377-1:2019 has to say:
>
>
>> Edit Rate copied from the Essence Tracks of the
>> Essence Container
>> [Note: SMPTE RP 210 definition Specifies the
>> indexing rate in hertz]
>
> It's possible to encode a file that does not specify IndexEditRate, but
> this is not allowed since the field is marked Required in Table 26.
> mxfdec.c will default to 0/0 since the segment is calloc'd. Michael's
> fix won't work if one changes the IndexEditRate local tag in the file
> to something else, say FFFF instead of 3F0B.

Yes, you are right about this. To be honest, I'd rather fix the invalid 
index edit rate issue by dropping the invalid segments when sorting them. 
That should work for both the explicitly and implicitly invalid index edit 
rates.

>
> In short, IndexEditRate MUST be set and it MUST equal the EditRate of
> the associated Essence Track (confusingly called source_track in the
> code). Section 11 is even more explicit:
>
>> An Index Table shall be used to index a single Essence Container.
>> Each Index Table shall index Edit Units
>> stored Essence of the Essence Container. The Edit Unit rate of an
>> Index Table is defined by the Edit Rate of the
>> Essence Tracks of the Package that describes the Essence Container
>> that the Index Table indexes.
>
> EditRate MAY be different between MaterialPackage and FilePackage
> however. This is a consequence of MXF's AAF heritage. MXF is really an
> NLE format.
>
> Section 11.6.2 "Look-up Algorithm for Conversion of Index Position to
> Stream Offset" is also of relevance. It doesn't make use of
> IndexEditRate at all.

Regards,
Marton
_______________________________________________
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/5] avformat/mxfdec: Check index_edit_rate

[Tomas Härdin]

sön 2024-04-14 klockan 22:55 +0200 skrev Marton Balint:
> 
> 
> On Wed, 10 Apr 2024, Tomas Härdin wrote:
> 
> > tis 2024-04-09 klockan 22:58 +0200 skrev Marton Balint:
> > > 
> > > 
> > > On Tue, 9 Apr 2024, Tomas Härdin wrote:
> > > 
> > > > mån 2024-04-08 klockan 21:46 +0200 skrev Marton Balint:
> > > > > 
> > > > > 
> > > > > On Mon, 8 Apr 2024, Tomas Härdin wrote:
> > > > > 
> > > > > > tor 2024-04-04 klockan 00:51 +0200 skrev Michael
> > > > > > Niedermayer:
> > > > > > > Fixes: Assertion b >=0 failed at
> > > > > > > libavutil/mathematics.c:62
> > > > > > > Fixes: 67811/clusterfuzz-testcase-minimized-
> > > > > > > ffmpeg_dem_MXF_fuzzer-
> > > > > > > 5108429687422976
> > > > > > > 
> > > > > > > Found-by: continuous fuzzing process
> > > > > > > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > > > > > > Signed-off-by: Michael Niedermayer
> > > > > > > <michael@niedermayer.cc>
> > > > > > > ---
> > > > > > >  libavformat/mxfdec.c | 3 +++
> > > > > > >  1 file changed, 3 insertions(+)
> > > > > > > 
> > > > > > > diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c
> > > > > > > index 04de4c1d5e3..233d614f783 100644
> > > > > > > --- a/libavformat/mxfdec.c
> > > > > > > +++ b/libavformat/mxfdec.c
> > > > > > > @@ -1264,6 +1264,9 @@ static int
> > > > > > > mxf_read_index_table_segment(void
> > > > > > > *arg, AVIOContext *pb, int tag, int
> > > > > > >      case 0x3F0B:
> > > > > > >          segment->index_edit_rate.num = avio_rb32(pb);
> > > > > > >          segment->index_edit_rate.den = avio_rb32(pb);
> > > > > > > +        if (segment->index_edit_rate.num <= 0 ||
> > > > > > > +            segment->index_edit_rate.den <= 0)
> > > > > > > +            return AVERROR_INVALIDDATA;
> > > > > > 
> > > > > > mxf_compute_index_tables() has a check for index_edit_rate
> > > > > > that
> > > > > > you
> > > > > > probably want to remove as well. It was introduced in
> > > > > > c6fff3d,
> > > > > > but
> > > > > > the
> > > > > > files it supposedly fixes aren't in FATE. We shouldn't
> > > > > > encourage
> > > > > > broken
> > > > > > muxers.
> > > > > 
> > > > > I don't quite get what FATE has to do with it. And the
> > > > > samples
> > > > > mentioned 
> > > > > in the patch has valid index segment edit rates, only they
> > > > > are
> > > > > different 
> > > > > from the track edit rate, and the patch was intended to fix
> > > > > that
> > > > > case.
> > > > 
> > > > Then why does it check against 0/0?
> > > 
> > > Probably to avoid divison by zero.
> > 
> > I think it's safe to say that EditRates with zero in the numerator
> > or
> > denominator are not allowed. We currently default to 25/1 in this
> > case
> > for Tracks, but I am skeptical of this since it encourages broken
> > muxers.
> 
> In general, I don't like the idea of rejecting everything which is
> not 
> following the standard to the letter. Decoding and demuxing should be
> based on the "Robustness principle", as in being liberal in what we
> accept 
> and strict in what we generate.

No. We should not encourage proliferation of broken muxers. This leads
to compounding headaches down the line.

> I am also not sure about your reasoning that rejecting files will
> force 
> vendors to fix their muxers, because the users will have to pay the
> price 
> for this approach. Users may well already have their archives full of
> non-compliant files, their camera, phone, whatever is likely out of 
> warranty/support, so they might not even be in a position to request 
> anything from vendors.

These users can sign an SLA if they want support for these cameras.

> Sure, I get it, some issues cannot be worked around easily, and I am
> not 
> saying that everything must be supported with huge hacks if needed.
> But an 
> effort should be made to not break existing real files, and support
> what 
> we reasonably can.

I mean, at the very least we need such samples in FATE or we can't
refactor mxfdec with any level of confidence. Restricting hacks to
specific vendors as identified by the Identification set would help
bring some order to them.

> > As for IndexEditRate, here's what ST 377-1:2019 has to say:
> > 
> > 
> > > Edit Rate copied from the Essence Tracks of the
> > > Essence Container
> > > [Note: SMPTE RP 210 definition Specifies the
> > > indexing rate in hertz]
> > 
> > It's possible to encode a file that does not specify IndexEditRate,
> > but
> > this is not allowed since the field is marked Required in Table 26.
> > mxfdec.c will default to 0/0 since the segment is calloc'd.
> > Michael's
> > fix won't work if one changes the IndexEditRate local tag in the
> > file
> > to something else, say FFFF instead of 3F0B.
> 
> Yes, you are right about this. To be honest, I'd rather fix the
> invalid 
> index edit rate issue by dropping the invalid segments when sorting
> them. 
> That should work for both the explicitly and implicitly invalid index
> edit 
> rates.

This is in contradiction with your desire to support broken muxers (:

If IndexEditRate doesn't match the IndexRate of the Essence Track then
we can't in general know what to do. We can know what to do for files
coming out of *specific muxers*, assuming they actually identify
themselves. Without knowing this, and without samples, we're just
engaging in cargo cult programming, or fear driven development.

/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 4/5] avformat/mxfdec: Check index_edit_rate

[Marton Balint]

On Mon, 15 Apr 2024, Tomas Härdin wrote:

> sön 2024-04-14 klockan 22:55 +0200 skrev Marton Balint:
>> 
>> 
>> On Wed, 10 Apr 2024, Tomas Härdin wrote:
>> 
>> > tis 2024-04-09 klockan 22:58 +0200 skrev Marton Balint:
>> > > 
>> > > 
>> > > On Tue, 9 Apr 2024, Tomas Härdin wrote:
>> > > 
>> > > > mån 2024-04-08 klockan 21:46 +0200 skrev Marton Balint:
>> > > > > 
>> > > > > 
>> > > > > On Mon, 8 Apr 2024, Tomas Härdin wrote:
>> > > > > 
>> > > > > > tor 2024-04-04 klockan 00:51 +0200 skrev Michael
>> > > > > > Niedermayer:
>> > > > > > > Fixes: Assertion b >=0 failed at
>> > > > > > > libavutil/mathematics.c:62
>> > > > > > > Fixes: 67811/clusterfuzz-testcase-minimized-
>> > > > > > > ffmpeg_dem_MXF_fuzzer-
>> > > > > > > 5108429687422976
>> > > > > > > 
>> > > > > > > Found-by: continuous fuzzing process
>> > > > > > > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
>> > > > > > > Signed-off-by: Michael Niedermayer
>> > > > > > > <michael@niedermayer.cc>
>> > > > > > > ---
>> > > > > > >  libavformat/mxfdec.c | 3 +++
>> > > > > > >  1 file changed, 3 insertions(+)
>> > > > > > > 
>> > > > > > > diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c
>> > > > > > > index 04de4c1d5e3..233d614f783 100644
>> > > > > > > --- a/libavformat/mxfdec.c
>> > > > > > > +++ b/libavformat/mxfdec.c
>> > > > > > > @@ -1264,6 +1264,9 @@ static int
>> > > > > > > mxf_read_index_table_segment(void
>> > > > > > > *arg, AVIOContext *pb, int tag, int
>> > > > > > >      case 0x3F0B:
>> > > > > > >          segment->index_edit_rate.num = avio_rb32(pb);
>> > > > > > >          segment->index_edit_rate.den = avio_rb32(pb);
>> > > > > > > +        if (segment->index_edit_rate.num <= 0 ||
>> > > > > > > +            segment->index_edit_rate.den <= 0)
>> > > > > > > +            return AVERROR_INVALIDDATA;
>> > > > > > 
>> > > > > > mxf_compute_index_tables() has a check for index_edit_rate
>> > > > > > that
>> > > > > > you
>> > > > > > probably want to remove as well. It was introduced in
>> > > > > > c6fff3d,
>> > > > > > but
>> > > > > > the
>> > > > > > files it supposedly fixes aren't in FATE. We shouldn't
>> > > > > > encourage
>> > > > > > broken
>> > > > > > muxers.
>> > > > > 
>> > > > > I don't quite get what FATE has to do with it. And the
>> > > > > samples
>> > > > > mentioned 
>> > > > > in the patch has valid index segment edit rates, only they
>> > > > > are
>> > > > > different 
>> > > > > from the track edit rate, and the patch was intended to fix
>> > > > > that
>> > > > > case.
>> > > > 
>> > > > Then why does it check against 0/0?
>> > > 
>> > > Probably to avoid divison by zero.
>> > 
>> > I think it's safe to say that EditRates with zero in the numerator
>> > or
>> > denominator are not allowed. We currently default to 25/1 in this
>> > case
>> > for Tracks, but I am skeptical of this since it encourages broken
>> > muxers.
>> 
>> In general, I don't like the idea of rejecting everything which is
>> not 
>> following the standard to the letter. Decoding and demuxing should be
>> based on the "Robustness principle", as in being liberal in what we
>> accept 
>> and strict in what we generate.
>
> No. We should not encourage proliferation of broken muxers. This leads
> to compounding headaches down the line.
>
>> I am also not sure about your reasoning that rejecting files will
>> force 
>> vendors to fix their muxers, because the users will have to pay the
>> price 
>> for this approach. Users may well already have their archives full of
>> non-compliant files, their camera, phone, whatever is likely out of 
>> warranty/support, so they might not even be in a position to request 
>> anything from vendors.
>
> These users can sign an SLA if they want support for these cameras.

You only want to support non-compliant files if the users pay for it?

Regards,
Marton
_______________________________________________
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/5] avformat/pcm: Use 64bit in bitrate computation

[Michael Niedermayer]

Fixes: signed integer overflow: 65792 * 65312 cannot be represented in type 'int'
Fixes: 67819/clusterfuzz-testcase-minimized-ffmpeg_dem_WADY_fuzzer-5236100912185344

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

diff --git a/libavformat/pcm.c b/libavformat/pcm.c
index 051e86dd464..a774dbc3726 100644
--- a/libavformat/pcm.c
+++ b/libavformat/pcm.c
@@ -41,7 +41,7 @@ int ff_pcm_default_packet_size(AVCodecParameters *par)
     /* Don't trust the codecpar bitrate if we can calculate it ourselves */
     if (bits_per_sample > 0 && par->sample_rate > 0 && par->ch_layout.nb_channels > 0)
         if ((int64_t)par->sample_rate * par->ch_layout.nb_channels < INT64_MAX / bits_per_sample)
-            bitrate = bits_per_sample * par->sample_rate * par->ch_layout.nb_channels;
+            bitrate = bits_per_sample * (int64_t)par->sample_rate * par->ch_layout.nb_channels;
 
     if (bitrate > 0) {
         nb_samples = av_clip64(bitrate / 8 / PCM_DEMUX_TARGET_FPS / par->block_align, 1, max_samples);
-- 
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 5/5] avformat/pcm: Use 64bit in bitrate computation

[Marton Balint]

On Thu, 4 Apr 2024, Michael Niedermayer wrote:

> Fixes: signed integer overflow: 65792 * 65312 cannot be represented in type 'int'
> Fixes: 67819/clusterfuzz-testcase-minimized-ffmpeg_dem_WADY_fuzzer-5236100912185344
>
> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
> libavformat/pcm.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/libavformat/pcm.c b/libavformat/pcm.c
> index 051e86dd464..a774dbc3726 100644
> --- a/libavformat/pcm.c
> +++ b/libavformat/pcm.c
> @@ -41,7 +41,7 @@ int ff_pcm_default_packet_size(AVCodecParameters *par)
>     /* Don't trust the codecpar bitrate if we can calculate it ourselves */
>     if (bits_per_sample > 0 && par->sample_rate > 0 && par->ch_layout.nb_channels > 0)
>         if ((int64_t)par->sample_rate * par->ch_layout.nb_channels < INT64_MAX / bits_per_sample)
> -            bitrate = bits_per_sample * par->sample_rate * par->ch_layout.nb_channels;
> +            bitrate = bits_per_sample * (int64_t)par->sample_rate * par->ch_layout.nb_channels;

LGTM, thanks.

I wonder why we usually cast the second operand and not the first to 64 
bit, since cast has higher precedence than multiplication, it should not 
matter, should it?

Regards,
Marton
_______________________________________________
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 5/5] avformat/pcm: Use 64bit in bitrate computation

[Andreas Rheinhardt]

Marton Balint:
> 
> 
> On Thu, 4 Apr 2024, Michael Niedermayer wrote:
> 
>> Fixes: signed integer overflow: 65792 * 65312 cannot be represented in
>> type 'int'
>> Fixes:
>> 67819/clusterfuzz-testcase-minimized-ffmpeg_dem_WADY_fuzzer-5236100912185344
>>
>> Found-by: continuous fuzzing process
>> https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
>> ---
>> libavformat/pcm.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/libavformat/pcm.c b/libavformat/pcm.c
>> index 051e86dd464..a774dbc3726 100644
>> --- a/libavformat/pcm.c
>> +++ b/libavformat/pcm.c
>> @@ -41,7 +41,7 @@ int ff_pcm_default_packet_size(AVCodecParameters *par)
>>     /* Don't trust the codecpar bitrate if we can calculate it
>> ourselves */
>>     if (bits_per_sample > 0 && par->sample_rate > 0 &&
>> par->ch_layout.nb_channels > 0)
>>         if ((int64_t)par->sample_rate * par->ch_layout.nb_channels <
>> INT64_MAX / bits_per_sample)
>> -            bitrate = bits_per_sample * par->sample_rate *
>> par->ch_layout.nb_channels;
>> +            bitrate = bits_per_sample * (int64_t)par->sample_rate *
>> par->ch_layout.nb_channels;
> 
> LGTM, thanks.
> 
> I wonder why we usually cast the second operand and not the first to 64
> bit, since cast has higher precedence than multiplication, it should not
> matter, should it?
> 

Presuming that all variables here have integer conversion rank <=
int64_t (true here for normal systems), then it does not matter: Casting
the first operand would automatically promote all the other operands to
int64_t, too; but if you add the cast to the last operand only, the
first multiplication will be performed with ints only.

- Andreas

_______________________________________________
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 5/5] avformat/pcm: Use 64bit in bitrate computation

[Michael Niedermayer]

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

On Thu, Apr 04, 2024 at 07:12:40PM +0200, Marton Balint wrote:
> 
> 
> On Thu, 4 Apr 2024, Michael Niedermayer wrote:
> 
> > Fixes: signed integer overflow: 65792 * 65312 cannot be represented in type 'int'
> > Fixes: 67819/clusterfuzz-testcase-minimized-ffmpeg_dem_WADY_fuzzer-5236100912185344
> > 
> > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > ---
> > libavformat/pcm.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/libavformat/pcm.c b/libavformat/pcm.c
> > index 051e86dd464..a774dbc3726 100644
> > --- a/libavformat/pcm.c
> > +++ b/libavformat/pcm.c
> > @@ -41,7 +41,7 @@ int ff_pcm_default_packet_size(AVCodecParameters *par)
> >     /* Don't trust the codecpar bitrate if we can calculate it ourselves */
> >     if (bits_per_sample > 0 && par->sample_rate > 0 && par->ch_layout.nb_channels > 0)
> >         if ((int64_t)par->sample_rate * par->ch_layout.nb_channels < INT64_MAX / bits_per_sample)
> > -            bitrate = bits_per_sample * par->sample_rate * par->ch_layout.nb_channels;
> > +            bitrate = bits_per_sample * (int64_t)par->sample_rate * par->ch_layout.nb_channels;
> 
> LGTM, thanks.
> 
> I wonder why we usually cast the second operand and not the first to 64 bit,
> since cast has higher precedence than multiplication, it should not matter,
> should it?

If the reader isnt sure about the precedence, writing it as
a * (int64_t)b
has the advantage that precedence doesnt matter, so the code is easier to
understand and verify than

(int64_t)a * b

thx


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

In a rich man's house there is no place to spit but his face.
-- Diogenes of Sinope

[-- 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/5] avcodec/wavarc: fix signed integer overflow in block type 6/19

[Michael Niedermayer]

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

On Thu, Apr 04, 2024 at 12:51:30AM +0200, Michael Niedermayer wrote:
> Fixes: signed integer overflow: -2088796289 + -91276551 cannot be represented in type 'int'
> Fixes: 67772/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_WAVARC_fuzzer-6533568953122816
> 
> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavcodec/wavarc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

will apply and backport these

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

Breaking DRM is a little like attempting to break through a door even
though the window is wide open and the only thing in the house is a bunch
of things you dont want and which you would get tomorrow for free anyway

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