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