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

[Marton Balint]

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