[FFmpeg-devel] [PATCH] avformat/mxfdec: Check index_duration

[Marton Balint]

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