[FFmpeg-devel] [PATCH 4/5] avformat/mxfdec: Check index_edit_rate

[Tomas Härdin]

sön 2024-04-14 klockan 22:55 +0200 skrev 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.

No. We should not encourage proliferation of broken muxers. This leads
to compounding headaches down the line.

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

These users can sign an SLA if they want support for these cameras.

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

I mean, at the very least we need such samples in FATE or we can't
refactor mxfdec with any level of confidence. Restricting hacks to
specific vendors as identified by the Identification set would help
bring some order to them.

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

This is in contradiction with your desire to support broken muxers (:

If IndexEditRate doesn't match the IndexRate of the Essence Track then
we can't in general know what to do. We can know what to do for files
coming out of *specific muxers*, assuming they actually identify
themselves. Without knowing this, and without samples, we're just
engaging in cargo cult programming, or fear driven development.

/Tomas