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

[Marton Balint]

On Mon, 15 Apr 2024, Tomas Härdin wrote:

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

You only want to support non-compliant files if the users pay for it?

Regards,
Marton