[FFmpeg-devel] [PATCH] lavf/mov.c: Fix parsing of edit list atoms with invalid elst entry count.

[Carl Eugen Hoyos]

2017-12-04 1:22 GMT+01:00 Michael Niedermayer <michael at niedermayer.cc>:
> On Sun, Oct 29, 2017 at 01:39:06PM +0100, Michael Niedermayer wrote:
>> On Sat, Oct 28, 2017 at 08:26:16PM +0200, Michael Niedermayer wrote:
>> > On Thu, Oct 26, 2017 at 08:51:50PM -0700, Sasi Inguva wrote:
>> > > On Tue, Oct 24, 2017 at 2:48 PM, Michael Niedermayer <michael at niedermayer.cc
>> > > > wrote:
>> > >
>> > > > On Mon, Oct 23, 2017 at 04:18:28PM -0700, Sasi Inguva wrote:
>> > > > > Signed-off-by: Sasi Inguva <isasi at google.com>
>> > > > > ---
>> > > > >  libavformat/mov.c                           | 15 +++++++-
>> > > > >  tests/fate/mov.mak                          |  4 ++
>> > > > >  tests/ref/fate/mov-invalid-elst-entry-count | 57
>> > > > +++++++++++++++++++++++++++++
>> > > > >  3 files changed, 75 insertions(+), 1 deletion(-)
>> > > > >  create mode 100644 tests/ref/fate/mov-invalid-elst-entry-count
>> > > > >
>> > > > > diff --git a/libavformat/mov.c b/libavformat/mov.c
>> > > > > index b22a116140..424293ad93 100644
>> > > > > --- a/libavformat/mov.c
>> > > > > +++ b/libavformat/mov.c
>> > > > > @@ -4597,6 +4597,7 @@ static int mov_read_elst(MOVContext *c,
>> > > > AVIOContext *pb, MOVAtom atom)
>> > > > >  {
>> > > > >      MOVStreamContext *sc;
>> > > > >      int i, edit_count, version;
>> > > > > +    int64_t elst_entry_size;
>> > > > >
>> > > > >      if (c->fc->nb_streams < 1 || c->ignore_editlist)
>> > > > >          return 0;
>> > > > > @@ -4605,6 +4606,15 @@ static int mov_read_elst(MOVContext *c,
>> > > > AVIOContext *pb, MOVAtom atom)
>> > > > >      version = avio_r8(pb); /* version */
>> > > > >      avio_rb24(pb); /* flags */
>> > > > >      edit_count = avio_rb32(pb); /* entries */
>> > > > > +    atom.size -= 8;
>> > > > > +
>> > > > > +    elst_entry_size = version == 1 ? 20 : 12;
>> > > > > +    if (atom.size != edit_count * elst_entry_size &&
>> > > > > +        c->fc->strict_std_compliance >= FF_COMPLIANCE_STRICT) {
>> > > > > +        av_log(c->fc, AV_LOG_ERROR, "Invalid edit list entry_count: %d
>> > > > for elst atom of size: %"PRId64" bytes.\n",
>> > > > > +               edit_count, atom.size + 8);
>> > > > > +        return AVERROR_INVALIDDATA;
>> > > > > +    }
>> > > > >
>> > > > >      if (!edit_count)
>> > > > >          return 0;
>> > > > > @@ -4617,17 +4627,20 @@ static int mov_read_elst(MOVContext *c,
>> > > > AVIOContext *pb, MOVAtom atom)
>> > > > >          return AVERROR(ENOMEM);
>> > > > >
>> > > > >      av_log(c->fc, AV_LOG_TRACE, "track[%u].edit_count = %i\n",
>> > > > c->fc->nb_streams - 1, edit_count);
>> > > > > -    for (i = 0; i < edit_count && !pb->eof_reached; i++) {
>> > > > > +    for (i = 0; i < edit_count && atom.size > 0 && !pb->eof_reached;
>> > > > i++) {
>> > > > >          MOVElst *e = &sc->elst_data[i];
>> > > > >
>> > > > >          if (version == 1) {
>> > > > >              e->duration = avio_rb64(pb);
>> > > > >              e->time     = avio_rb64(pb);
>> > > > > +            atom.size -= 16;
>> > > > >          } else {
>> > > > >              e->duration = avio_rb32(pb); /* segment duration */
>> > > > >              e->time     = (int32_t)avio_rb32(pb); /* media time */
>> > > > > +            atom.size -= 8;
>> > > > >          }
>> > > > >          e->rate = avio_rb32(pb) / 65536.0;
>> > > > > +        atom.size -= 4;
>> > > > >          av_log(c->fc, AV_LOG_TRACE, "duration=%"PRId64" time=%"PRId64"
>> > > > rate=%f\n",
>> > > > >                 e->duration, e->time, e->rate);
>> > > >
>> > > > it would be simpler to adjust edit_count in case of ineqality.
>> > > > you already compute the elst_entry_size
>> > > > this would also avoid allocating a larger than needed array
>> > > >
>> > > > Done. Attaching the corrected patch.
>> > >
>> > > >
>> > > > [...]
>> > > >
>> > > > --
>> > > > Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>> > > >
>> > > > Many things microsoft did are stupid, but not doing something just because
>> > > > microsoft did it is even more stupid. If everything ms did were stupid they
>> > > > would be bankrupt already.
>> > > >
>> > > > _______________________________________________
>> > > > ffmpeg-devel mailing list
>> > > > ffmpeg-devel at ffmpeg.org
>> > > > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>> > > >
>> > > >
>> >
>> > >  libavformat/mov.c                           |   21 +++++++++-
>> > >  tests/fate/mov.mak                          |    4 +
>> > >  tests/ref/fate/mov-invalid-elst-entry-count |   57 ++++++++++++++++++++++++++++
>> > >  3 files changed, 81 insertions(+), 1 deletion(-)
>> > > c553340f66797876d039f408f83574a40c54d17b  0001-lavf-mov.c-Fix-parsing-of-edit-list-atoms-with-inval.patch
>> > > From 188602e662868a79c7f85e9193e9aedc9ba1a170 Mon Sep 17 00:00:00 2001
>> > > From: Sasi Inguva <isasi at google.com>
>> > > Date: Wed, 18 Oct 2017 20:11:16 -0700
>> > > Subject: [PATCH] lavf/mov.c: Fix parsing of edit list atoms with invalid elst
>> > >  entry count.
>> >
>> > applied
>>
>> It seems this fails on ARM qemu
>>
>> TEST    mov-invalid-elst-entry-count
>> --- tests/ref/fate/mov-invalid-elst-entry-count 2017-10-29 13:26:29.180224127 +0100
>> +++ tests/data/fate/mov-invalid-elst-entry-count        2017-10-29 13:38:18.536239072 +0100
>> @@ -8,50 +8,50 @@
>>  #sar 0: 1/1
>>  #stream#, dts,        pts, duration,     size, hash
>>  0,          0,          0,        1,   460800, 549730883a0b56e6accaf021903daecf
>> -0,          1,          1,        1,   460800, 783389b4342d4be925fc5244791e760a
>> -0,          2,          2,        1,   460800, 8384af6426d94a2077930c93013e09ad
>> -0,          3,          3,        1,   460800, 9380a1d9ecacf5b3105383c1c8083188
>
> should i disable this test or is someone working on fixing this ?

I don't know what the issue is and I am not working on a fix.

Carl Eugen