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

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