Git Inbox Mirror of the ffmpeg-devel mailing list - see https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
 help / color / mirror / Atom feed
* [FFmpeg-devel] [PATCH] avformat/mxfdec: Check index_duration
@ 2022-12-24 22:50 Michael Niedermayer
  2022-12-26 10:32 ` Tomas Härdin
  0 siblings, 1 reply; 13+ messages in thread
From: Michael Niedermayer @ 2022-12-24 22:50 UTC (permalink / raw)
  To: FFmpeg development discussions and patches

Fixes: OOM
Fixes: 50551/clusterfuzz-testcase-minimized-ffmpeg_dem_MXF_fuzzer-6607795234930688

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 | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c
index 0553728253..64ac6a44b8 100644
--- a/libavformat/mxfdec.c
+++ b/libavformat/mxfdec.c
@@ -1943,6 +1943,10 @@ static int mxf_compute_ptses_fake_index(MXFContext *mxf, MXFIndexTable *index_ta
         }
 
         index_table->nb_ptses += s->index_duration;
+        // If index_duration is substantially larger than nb_index_entries then this algorithm which
+        // allocates index_duration elements is a bad idea. All files i tried have it equal
+        if (s->index_duration > 10LL * s->nb_index_entries)
+            return AVERROR_PATCHWELCOME;
     }
 
     /* paranoid check */
-- 
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".

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [FFmpeg-devel] [PATCH] avformat/mxfdec: Check index_duration
  2022-12-24 22:50 [FFmpeg-devel] [PATCH] avformat/mxfdec: Check index_duration Michael Niedermayer
@ 2022-12-26 10:32 ` Tomas Härdin
  2022-12-26 21:54   ` Michael Niedermayer
  0 siblings, 1 reply; 13+ messages in thread
From: Tomas Härdin @ 2022-12-26 10:32 UTC (permalink / raw)
  To: FFmpeg development discussions and patches

lör 2022-12-24 klockan 23:50 +0100 skrev Michael Niedermayer:
> 
>          index_table->nb_ptses += s->index_duration;
> +        // If index_duration is substantially larger than
> nb_index_entries then this algorithm which
> +        // allocates index_duration elements is a bad idea. All
> files i tried have it equal
> +        if (s->index_duration > 10LL * s->nb_index_entries)
> +            return AVERROR_PATCHWELCOME;

I was going to say this can overflow but the 10LL ensures it can't. So
looks OK.

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [FFmpeg-devel] [PATCH] avformat/mxfdec: Check index_duration
  2022-12-26 10:32 ` Tomas Härdin
@ 2022-12-26 21:54   ` Michael Niedermayer
  2022-12-27 18:05     ` Marton Balint
  0 siblings, 1 reply; 13+ messages in thread
From: Michael Niedermayer @ 2022-12-26 21:54 UTC (permalink / raw)
  To: FFmpeg development discussions and patches


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

On Mon, Dec 26, 2022 at 11:32:48AM +0100, Tomas Härdin wrote:
> lör 2022-12-24 klockan 23:50 +0100 skrev Michael Niedermayer:
> > 
> >          index_table->nb_ptses += s->index_duration;
> > +        // If index_duration is substantially larger than
> > nb_index_entries then this algorithm which
> > +        // allocates index_duration elements is a bad idea. All
> > files i tried have it equal
> > +        if (s->index_duration > 10LL * s->nb_index_entries)
> > +            return AVERROR_PATCHWELCOME;
> 
> I was going to say this can overflow but the 10LL ensures it can't. So
> looks OK.

will apply

thx

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

The real ebay dictionary, page 1
"Used only once"    - "Some unspecified defect prevented a second use"
"In good condition" - "Can be repaird by experienced expert"
"As is" - "You wouldnt want it even if you were payed for it, if you knew ..."

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [FFmpeg-devel] [PATCH] avformat/mxfdec: Check index_duration
  2022-12-26 21:54   ` Michael Niedermayer
@ 2022-12-27 18:05     ` Marton Balint
  2022-12-27 21:49       ` Michael Niedermayer
  0 siblings, 1 reply; 13+ messages in thread
From: Marton Balint @ 2022-12-27 18:05 UTC (permalink / raw)
  To: FFmpeg development discussions and patches



On Mon, 26 Dec 2022, Michael Niedermayer wrote:

> On Mon, Dec 26, 2022 at 11:32:48AM +0100, Tomas Härdin wrote:
>> lör 2022-12-24 klockan 23:50 +0100 skrev Michael Niedermayer:
>>>
>>>          index_table->nb_ptses += s->index_duration;
>>> +        // If index_duration is substantially larger than
>>> nb_index_entries then this algorithm which
>>> +        // allocates index_duration elements is a bad idea. All
>>> files i tried have it equal
>>> +        if (s->index_duration > 10LL * s->nb_index_entries)
>>> +            return AVERROR_PATCHWELCOME;
>>
>> I was going to say this can overflow but the 10LL ensures it can't. So
>> looks OK.
>
> will apply

Please don't, as far as I see this disallows the usage of partial index 
tables, so practically rejecting valid files, which is not OK.

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [FFmpeg-devel] [PATCH] avformat/mxfdec: Check index_duration
  2022-12-27 18:05     ` Marton Balint
@ 2022-12-27 21:49       ` Michael Niedermayer
  2022-12-27 22:32         ` Marton Balint
  2022-12-27 22:35         ` Tomas Härdin
  0 siblings, 2 replies; 13+ messages in thread
From: Michael Niedermayer @ 2022-12-27 21:49 UTC (permalink / raw)
  To: FFmpeg development discussions and patches


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

On Tue, Dec 27, 2022 at 07:05:44PM +0100, Marton Balint wrote:
> 
> 
> On Mon, 26 Dec 2022, Michael Niedermayer wrote:
> 
> > On Mon, Dec 26, 2022 at 11:32:48AM +0100, Tomas Härdin wrote:
> > > lör 2022-12-24 klockan 23:50 +0100 skrev Michael Niedermayer:
> > > > 
> > > >          index_table->nb_ptses += s->index_duration;
> > > > +        // If index_duration is substantially larger than
> > > > nb_index_entries then this algorithm which
> > > > +        // allocates index_duration elements is a bad idea. All
> > > > files i tried have it equal
> > > > +        if (s->index_duration > 10LL * s->nb_index_entries)
> > > > +            return AVERROR_PATCHWELCOME;
> > > 
> > > I was going to say this can overflow but the 10LL ensures it can't. So
> > > looks OK.
> > 
> > will apply
> 
> Please don't, as far as I see this disallows the usage of partial index
> tables, so practically rejecting valid files, which is not OK.

can you share a file that would break ?

thx

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

If you drop bombs on a foreign country and kill a hundred thousand
innocent people, expect your government to call the consequence
"unprovoked inhuman terrorist attacks" and use it to justify dropping
more bombs and killing more people. The technology changed, the idea is old.

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [FFmpeg-devel] [PATCH] avformat/mxfdec: Check index_duration
  2022-12-27 21:49       ` Michael Niedermayer
@ 2022-12-27 22:32         ` Marton Balint
  2022-12-28  2:26           ` Marton Balint
  2022-12-27 22:35         ` Tomas Härdin
  1 sibling, 1 reply; 13+ messages in thread
From: Marton Balint @ 2022-12-27 22:32 UTC (permalink / raw)
  To: FFmpeg development discussions and patches



On Tue, 27 Dec 2022, Michael Niedermayer wrote:

> On Tue, Dec 27, 2022 at 07:05:44PM +0100, Marton Balint wrote:
>>
>>
>> On Mon, 26 Dec 2022, Michael Niedermayer wrote:
>>
>>> On Mon, Dec 26, 2022 at 11:32:48AM +0100, Tomas Härdin wrote:
>>>> lör 2022-12-24 klockan 23:50 +0100 skrev Michael Niedermayer:
>>>>>
>>>>>          index_table->nb_ptses += s->index_duration;
>>>>> +        // If index_duration is substantially larger than
>>>>> nb_index_entries then this algorithm which
>>>>> +        // allocates index_duration elements is a bad idea. All
>>>>> files i tried have it equal
>>>>> +        if (s->index_duration > 10LL * s->nb_index_entries)
>>>>> +            return AVERROR_PATCHWELCOME;
>>>>
>>>> I was going to say this can overflow but the 10LL ensures it can't. So
>>>> looks OK.
>>>
>>> will apply
>>
>> Please don't, as far as I see this disallows the usage of partial index
>> tables, so practically rejecting valid files, which is not OK.
>
> can you share a file that would break ?

I don't have such file. But the MXF specs (SMPTE 377-1-2009) explictly 
defines the concept of partial index tables:

"Where all Index Table segments are contiguous, or there is only one 
segment, but not all Edit Units in the Essence Container are indexed, 
these tables are called Partial Index Tables."

As far as I see here nb_index_entries is corresponding to the number of 
indexed edit units, and the number is allowed to be smaller than the index 
duration, because not all edit units have to be indexed.

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [FFmpeg-devel] [PATCH] avformat/mxfdec: Check index_duration
  2022-12-27 21:49       ` Michael Niedermayer
  2022-12-27 22:32         ` Marton Balint
@ 2022-12-27 22:35         ` Tomas Härdin
  1 sibling, 0 replies; 13+ messages in thread
From: Tomas Härdin @ 2022-12-27 22:35 UTC (permalink / raw)
  To: FFmpeg development discussions and patches

tis 2022-12-27 klockan 22:49 +0100 skrev Michael Niedermayer:
> On Tue, Dec 27, 2022 at 07:05:44PM +0100, Marton Balint wrote:
> > 
> > 
> > On Mon, 26 Dec 2022, Michael Niedermayer wrote:
> > 
> > > On Mon, Dec 26, 2022 at 11:32:48AM +0100, Tomas Härdin wrote:
> > > > lör 2022-12-24 klockan 23:50 +0100 skrev Michael Niedermayer:
> > > > > 
> > > > >          index_table->nb_ptses += s->index_duration;
> > > > > +        // If index_duration is substantially larger than
> > > > > nb_index_entries then this algorithm which
> > > > > +        // allocates index_duration elements is a bad idea.
> > > > > All
> > > > > files i tried have it equal
> > > > > +        if (s->index_duration > 10LL * s->nb_index_entries)
> > > > > +            return AVERROR_PATCHWELCOME;
> > > > 
> > > > I was going to say this can overflow but the 10LL ensures it
> > > > can't. So
> > > > looks OK.
> > > 
> > > will apply
> > 
> > Please don't, as far as I see this disallows the usage of partial
> > index
> > tables, so practically rejecting valid files, which is not OK.

Damn, not sure how I missed that. The use of the magic constant 10
should have tipped me off.

> 
> can you share a file that would break ?

mxfenc will produce files like that. It's actually a major deficiency
with the muxer since it produces excessive amounts of partitions.

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [FFmpeg-devel] [PATCH] avformat/mxfdec: Check index_duration
  2022-12-27 22:32         ` Marton Balint
@ 2022-12-28  2:26           ` Marton Balint
  2022-12-28  3:19             ` [FFmpeg-devel] [PATCH 1/3] avformat/mxfdec: check index entry array size Marton Balint
  2023-01-22  0:05             ` [FFmpeg-devel] [PATCH] avformat/mxfdec: Check index_duration Marton Balint
  0 siblings, 2 replies; 13+ messages in thread
From: Marton Balint @ 2022-12-28  2:26 UTC (permalink / raw)
  To: FFmpeg development discussions and patches



On Tue, 27 Dec 2022, Marton Balint wrote:

>
>
> On Tue, 27 Dec 2022, Michael Niedermayer wrote:
>
>>  On Tue, Dec 27, 2022 at 07:05:44PM +0100, Marton Balint wrote:
>>> 
>>>
>>>  On Mon, 26 Dec 2022, Michael Niedermayer wrote:
>>>
>>>>  On Mon, Dec 26, 2022 at 11:32:48AM +0100, Tomas Härdin wrote:
>>>>>  lör 2022-12-24 klockan 23:50 +0100 skrev Michael Niedermayer:
>>>>>>
>>>>>>           index_table->nb_ptses += s->index_duration;
>>>>>>  +        // If index_duration is substantially larger than
>>>>>>  nb_index_entries then this algorithm which
>>>>>>  +        // allocates index_duration elements is a bad idea. All
>>>>>>  files i tried have it equal
>>>>>>  +        if (s->index_duration > 10LL * s->nb_index_entries)
>>>>>>  +            return AVERROR_PATCHWELCOME;
>>>>>
>>>>>  I was going to say this can overflow but the 10LL ensures it can't. So
>>>>>  looks OK.
>>>>
>>>>  will apply
>>>
>>>  Please don't, as far as I see this disallows the usage of partial index
>>>  tables, so practically rejecting valid files, which is not OK.
>>
>>  can you share a file that would break ?
>
> I don't have such file. But the MXF specs (SMPTE 377-1-2009) explictly 
> defines the concept of partial index tables:
>
> "Where all Index Table segments are contiguous, or there is only one segment, 
> but not all Edit Units in the Essence Container are indexed, these tables are 
> called Partial Index Tables."
>
> As far as I see here nb_index_entries is corresponding to the number of 
> indexed edit units, and the number is allowed to be smaller than the index 
> duration, because not all edit units have to be indexed.

I read the specs again, and it seems that I misread it the first time, 
because partial index tables mean that the index segments have no gaps 
between them, but the index still not cover the whole essence. So it is 
not referring to the index entries in the segment.

So, in principal your patch *might* be OK. However, existing code simply 
ignores a corrupt index table, does not reject it. I kind of prefer we 
make the check more strict, but gracefully allow corrupted index by 
ignoring it fully.

I will post a follow up patch series.

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [FFmpeg-devel] [PATCH 1/3] avformat/mxfdec: check index entry array size
  2022-12-28  2:26           ` Marton Balint
@ 2022-12-28  3:19             ` Marton Balint
  2022-12-28  3:19               ` [FFmpeg-devel] [PATCH 2/3] avformat/mxfdec: support Avid files with an extra index entry Marton Balint
  2022-12-28  3:19               ` [FFmpeg-devel] [PATCH 3/3] avformat/mxfdec: check number of index table entires more strictly Marton Balint
  2023-01-22  0:05             ` [FFmpeg-devel] [PATCH] avformat/mxfdec: Check index_duration Marton Balint
  1 sibling, 2 replies; 13+ messages in thread
From: Marton Balint @ 2022-12-28  3:19 UTC (permalink / raw)
  To: ffmpeg-devel; +Cc: Marton Balint

Signed-off-by: Marton Balint <cus@passwd.hu>
---
 libavformat/mxfdec.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c
index e6118e141d..ad4ee15570 100644
--- a/libavformat/mxfdec.c
+++ b/libavformat/mxfdec.c
@@ -1198,11 +1198,15 @@ static int mxf_read_essence_container_data(void *arg, AVIOContext *pb, int tag,
 static int mxf_read_index_entry_array(AVIOContext *pb, MXFIndexTableSegment *segment)
 {
     int i, length;
+    uint32_t nb_index_entries;
 
     if (segment->temporal_offset_entries)
         return AVERROR_INVALIDDATA;
 
-    segment->nb_index_entries = avio_rb32(pb);
+    nb_index_entries = avio_rb32(pb);
+    if (nb_index_entries > INT_MAX)
+        return AVERROR_INVALIDDATA;
+    segment->nb_index_entries = nb_index_entries;
 
     length = avio_rb32(pb);
     if(segment->nb_index_entries && length < 11)
-- 
2.35.3

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [FFmpeg-devel] [PATCH 2/3] avformat/mxfdec: support Avid files with an extra index entry
  2022-12-28  3:19             ` [FFmpeg-devel] [PATCH 1/3] avformat/mxfdec: check index entry array size Marton Balint
@ 2022-12-28  3:19               ` Marton Balint
  2022-12-28  3:19               ` [FFmpeg-devel] [PATCH 3/3] avformat/mxfdec: check number of index table entires more strictly Marton Balint
  1 sibling, 0 replies; 13+ messages in thread
From: Marton Balint @ 2022-12-28  3:19 UTC (permalink / raw)
  To: ffmpeg-devel; +Cc: Marton Balint

Signed-off-by: Marton Balint <cus@passwd.hu>
---
 libavformat/mxfdec.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c
index ad4ee15570..2e61e77d67 100644
--- a/libavformat/mxfdec.c
+++ b/libavformat/mxfdec.c
@@ -1991,11 +1991,11 @@ static int mxf_compute_ptses_fake_index(MXFContext *mxf, MXFIndexTable *index_ta
         int index_delta = 1;
         int n = s->nb_index_entries;
 
-        if (s->nb_index_entries == 2 * s->index_duration + 1) {
+        if (s->nb_index_entries == 2 * s->index_duration + 1)
             index_delta = 2;    /* Avid index */
-            /* ignore the last entry - it's the size of the essence container */
+        if (s->nb_index_entries == index_delta * s->index_duration + 1)
+            /* ignore the last entry - it's the size of the essence container in Avid */
             n--;
-        }
 
         for (j = 0; j < n; j += index_delta, x++) {
             int offset = s->temporal_offset_entries[j] / index_delta;
-- 
2.35.3

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [FFmpeg-devel] [PATCH 3/3] avformat/mxfdec: check number of index table entires more strictly
  2022-12-28  3:19             ` [FFmpeg-devel] [PATCH 1/3] avformat/mxfdec: check index entry array size Marton Balint
  2022-12-28  3:19               ` [FFmpeg-devel] [PATCH 2/3] avformat/mxfdec: support Avid files with an extra index entry Marton Balint
@ 2022-12-28  3:19               ` Marton Balint
  1 sibling, 0 replies; 13+ messages in thread
From: Marton Balint @ 2022-12-28  3:19 UTC (permalink / raw)
  To: ffmpeg-devel; +Cc: Marton Balint

Let's ignore the index table if the number of index entries does not match the
index duration (or the special AVID index entry counts).

Fixes: OOM
Fixes: 50551/clusterfuzz-testcase-minimized-ffmpeg_dem_MXF_fuzzer-6607795234930688

Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
Signed-off-by: Marton Balint <cus@passwd.hu>
---
 libavformat/mxfdec.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c
index 2e61e77d67..0dc61648b1 100644
--- a/libavformat/mxfdec.c
+++ b/libavformat/mxfdec.c
@@ -1937,6 +1937,14 @@ static int mxf_compute_ptses_fake_index(MXFContext *mxf, MXFIndexTable *index_ta
             return 0;
         }
 
+        if (s->nb_index_entries != s->index_duration &&
+            s->nb_index_entries != s->index_duration + 1 &&
+            s->nb_index_entries != s->index_duration * 2 + 1) {
+            index_table->nb_ptses = 0;
+            av_log(mxf->fc, AV_LOG_ERROR, "ignoring IndexSID %d, duration does not match nb_index_entries\n", s->index_sid);
+            return 0;
+        }
+
         index_table->nb_ptses += s->index_duration;
     }
 
-- 
2.35.3

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [FFmpeg-devel] [PATCH] avformat/mxfdec: Check index_duration
  2022-12-28  2:26           ` Marton Balint
  2022-12-28  3:19             ` [FFmpeg-devel] [PATCH 1/3] avformat/mxfdec: check index entry array size Marton Balint
@ 2023-01-22  0:05             ` Marton Balint
  2023-01-29 11:15               ` Marton Balint
  1 sibling, 1 reply; 13+ messages in thread
From: Marton Balint @ 2023-01-22  0:05 UTC (permalink / raw)
  To: FFmpeg development discussions and patches



On Wed, 28 Dec 2022, Marton Balint wrote:

>
>
> On Tue, 27 Dec 2022, Marton Balint wrote:
>
>> 
>>
>>  On Tue, 27 Dec 2022, Michael Niedermayer wrote:
>>
>>>   On Tue, Dec 27, 2022 at 07:05:44PM +0100, Marton Balint wrote:
>>>> 
>>>>
>>>>   On Mon, 26 Dec 2022, Michael Niedermayer wrote:
>>>>
>>>>>   On Mon, Dec 26, 2022 at 11:32:48AM +0100, Tomas Härdin wrote:
>>>>>>   lör 2022-12-24 klockan 23:50 +0100 skrev Michael Niedermayer:
>>>>>>>
>>>>>>>            index_table->nb_ptses += s->index_duration;
>>>>>>>   +        // If index_duration is substantially larger than
>>>>>>>   nb_index_entries then this algorithm which
>>>>>>>   +        // allocates index_duration elements is a bad idea. All
>>>>>>>   files i tried have it equal
>>>>>>>   +        if (s->index_duration > 10LL * s->nb_index_entries)
>>>>>>>   +            return AVERROR_PATCHWELCOME;
>>>>>>
>>>>>>   I was going to say this can overflow but the 10LL ensures it can't.
>>>>>>   So
>>>>>>   looks OK.
>>>>>
>>>>>   will apply
>>>>
>>>>   Please don't, as far as I see this disallows the usage of partial index
>>>>   tables, so practically rejecting valid files, which is not OK.
>>>
>>>   can you share a file that would break ?
>>
>>  I don't have such file. But the MXF specs (SMPTE 377-1-2009) explictly
>>  defines the concept of partial index tables:
>>
>>  "Where all Index Table segments are contiguous, or there is only one
>>  segment, but not all Edit Units in the Essence Container are indexed,
>>  these tables are called Partial Index Tables."
>>
>>  As far as I see here nb_index_entries is corresponding to the number of
>>  indexed edit units, and the number is allowed to be smaller than the index
>>  duration, because not all edit units have to be indexed.
>
> I read the specs again, and it seems that I misread it the first time, 
> because partial index tables mean that the index segments have no gaps 
> between them, but the index still not cover the whole essence. So it is not 
> referring to the index entries in the segment.
>
> So, in principal your patch *might* be OK. However, existing code simply 
> ignores a corrupt index table, does not reject it. I kind of prefer we make 
> the check more strict, but gracefully allow corrupted index by ignoring it 
> fully.
>
> I will post a follow up patch series.

Ping for the series I posted.

Thanks,
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".

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [FFmpeg-devel] [PATCH] avformat/mxfdec: Check index_duration
  2023-01-22  0:05             ` [FFmpeg-devel] [PATCH] avformat/mxfdec: Check index_duration Marton Balint
@ 2023-01-29 11:15               ` Marton Balint
  0 siblings, 0 replies; 13+ messages in thread
From: Marton Balint @ 2023-01-29 11:15 UTC (permalink / raw)
  To: FFmpeg development discussions and patches



On Sun, 22 Jan 2023, Marton Balint wrote:

>
>
> On Wed, 28 Dec 2022, Marton Balint wrote:
>
>> 
>>
>>  On Tue, 27 Dec 2022, Marton Balint wrote:
>>
>>> 
>>>
>>>   On Tue, 27 Dec 2022, Michael Niedermayer wrote:
>>>
>>>>    On Tue, Dec 27, 2022 at 07:05:44PM +0100, Marton Balint wrote:
>>>>> 
>>>>>
>>>>>    On Mon, 26 Dec 2022, Michael Niedermayer wrote:
>>>>>
>>>>>>    On Mon, Dec 26, 2022 at 11:32:48AM +0100, Tomas Härdin wrote:
>>>>>>>    lör 2022-12-24 klockan 23:50 +0100 skrev Michael Niedermayer:
>>>>>>>>
>>>>>>>>             index_table->nb_ptses += s->index_duration;
>>>>>>>>    +        // If index_duration is substantially larger than
>>>>>>>>    nb_index_entries then this algorithm which
>>>>>>>>    +        // allocates index_duration elements is a bad idea. All
>>>>>>>>    files i tried have it equal
>>>>>>>>    +        if (s->index_duration > 10LL * s->nb_index_entries)
>>>>>>>>    +            return AVERROR_PATCHWELCOME;
>>>>>>>
>>>>>>>    I was going to say this can overflow but the 10LL ensures it can't.
>>>>>>>    So
>>>>>>>    looks OK.
>>>>>>
>>>>>>    will apply
>>>>>
>>>>>    Please don't, as far as I see this disallows the usage of partial
>>>>>    index
>>>>>    tables, so practically rejecting valid files, which is not OK.
>>>>
>>>>    can you share a file that would break ?
>>>
>>>   I don't have such file. But the MXF specs (SMPTE 377-1-2009) explictly
>>>   defines the concept of partial index tables:
>>>
>>>   "Where all Index Table segments are contiguous, or there is only one
>>>   segment, but not all Edit Units in the Essence Container are indexed,
>>>   these tables are called Partial Index Tables."
>>>
>>>   As far as I see here nb_index_entries is corresponding to the number of
>>>   indexed edit units, and the number is allowed to be smaller than the
>>>   index
>>>   duration, because not all edit units have to be indexed.
>>
>>  I read the specs again, and it seems that I misread it the first time,
>>  because partial index tables mean that the index segments have no gaps
>>  between them, but the index still not cover the whole essence. So it is
>>  not referring to the index entries in the segment.
>>
>>  So, in principal your patch *might* be OK. However, existing code simply
>>  ignores a corrupt index table, does not reject it. I kind of prefer we
>>  make the check more strict, but gracefully allow corrupted index by
>>  ignoring it fully.
>>
>>  I will post a follow up patch series.
>
> Ping for the series I posted.

Will apply.

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2023-01-29 11:15 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-12-24 22:50 [FFmpeg-devel] [PATCH] avformat/mxfdec: Check index_duration Michael Niedermayer
2022-12-26 10:32 ` Tomas Härdin
2022-12-26 21:54   ` Michael Niedermayer
2022-12-27 18:05     ` Marton Balint
2022-12-27 21:49       ` Michael Niedermayer
2022-12-27 22:32         ` Marton Balint
2022-12-28  2:26           ` Marton Balint
2022-12-28  3:19             ` [FFmpeg-devel] [PATCH 1/3] avformat/mxfdec: check index entry array size Marton Balint
2022-12-28  3:19               ` [FFmpeg-devel] [PATCH 2/3] avformat/mxfdec: support Avid files with an extra index entry Marton Balint
2022-12-28  3:19               ` [FFmpeg-devel] [PATCH 3/3] avformat/mxfdec: check number of index table entires more strictly Marton Balint
2023-01-22  0:05             ` [FFmpeg-devel] [PATCH] avformat/mxfdec: Check index_duration Marton Balint
2023-01-29 11:15               ` Marton Balint
2022-12-27 22:35         ` Tomas Härdin

Git Inbox Mirror of the ffmpeg-devel mailing list - see https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://master.gitmailbox.com/ffmpegdev/0 ffmpegdev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 ffmpegdev ffmpegdev/ https://master.gitmailbox.com/ffmpegdev \
		ffmpegdev@gitmailbox.com
	public-inbox-index ffmpegdev

Example config snippet for mirrors.


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git