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

[Michael Niedermayer]

On Mon, Dec 04, 2017 at 03:24:18AM +0100, Carl Eugen Hoyos wrote:
> 2017-12-04 1:22 GMT+01:00 Michael Niedermayer :
> > 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 
> >> > >  >> > > > wrote:
> >> > >
> >> > > > On Mon, Oct 23, 2017 at 04:18:28PM -0700, Sasi Inguva wrote:
> >> > > > > Signed-off-by: Sasi Inguva 
> >> > > > > ---
> >> > > > >  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 = >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@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(-)
> >> > > 

Re: [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 :
> 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 
>> > > > > > > wrote:
>> > >
>> > > > On Mon, Oct 23, 2017 at 04:18:28PM -0700, Sasi Inguva wrote:
>> > > > > Signed-off-by: Sasi Inguva 
>> > > > > ---
>> > > > >  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 = >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@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 
>> > > Date: Wed, 18 Oct 2017 20:11:16 -0700
>> > > Subject: [PATCH] lavf/mov.c: Fix parsing of edit list atoms 

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

[Michael Niedermayer]

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 
> > >  > > > wrote:
> > > 
> > > > On Mon, Oct 23, 2017 at 04:18:28PM -0700, Sasi Inguva wrote:
> > > > > Signed-off-by: Sasi Inguva 
> > > > > ---
> > > > >  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 = >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@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 
> > > 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
> 
> TESTmov-invalid-elst-entry-count
> --- 

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

[Carl Eugen Hoyos]

2017-10-29 13:39 GMT+01:00 Michael Niedermayer :
> 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:

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

Fails differently here on ppc64be:

--- ./tests/ref/fate/mov-invalid-elst-entry-count   2017-10-28
22:32:57.766024470 +
+++ tests/data/fate/mov-invalid-elst-entry-count2017-10-29
14:04:35.227325635 +
@@ -7,51 +7,51 @@
 #dimensions 0: 640x480
 #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
-0,  4,  4,1,   460800, eb28174acfceb868b9058757bed049c5
-0,  5,  5,1,   460800, 9732bd4a58884dbf2be48d819433130f
-0,  6,  6,1,   460800, 0c553fb530cf042eb84f5b13817a96a6
-0,  7,  7,1,   460800, 621f02aded5e35fa1f373afd3ed283bd
-0,  8,  8,1,   460800, c76167c6bda91f657708c88252ea315d
-0,  9,  9,1,   460800, 872df2d8c522e2440ddd04bca7dce497
-0, 10, 10,1,   460800, 6ee9154e48c5132ad4ba122b255bd2bb
-0, 11, 11,1,   460800, 362e61629795702ebe9183ce3786d7f2
-0, 12, 12,1,   460800, f3ec59e6fc4e3c2e75f42bef34ca73b5
-0, 13, 13,1,   460800, 68d9caea8697736dd716cba43b614919
-0, 14, 14,1,   460800, 4a4efb0201a64236db4330725758c139
-0, 15, 15,1,   460800, f32f8997dcdd87ad910dea886a0de17d
-0, 16, 16,1,   460800, 51a8549d7b4aaacaf6050bc07a82b440
-0, 17, 17,1,   460800, 5145aa05bbb0c3faab40fc8fa233af1d
-0, 18, 18,1,   460800, bbfcbe3c9600b2a0f413057d7959e9e7
-0, 19, 19,1,   460800, 02cfd4a350fa274e12fce8352001bf21
-0, 20, 20,1,   460800, 20dd372da9e656add433f31e3e9c1fb8
-0, 21, 21,1,   460800, 3b885593f8b42676ce40c329a63f62bf
-0, 22, 22,1,   460800, c38b453b56c3ea14f7d8691d83752486
-0, 23, 23,1,   460800, 79643132988dabc9dc1ba3af0aeaebc5
-0, 24, 24,1,   460800, 60a099be31244b2f69ca6107cdbd7e06
-0, 25, 25,1,   460800, 1de6ff4e0aa81216e4b7b1c8e74fb143
-0, 26, 26,1,   460800, 5223a81e6964c28cf42593f259397aa1
-0, 27, 27,1,   460800, 2dfcf01c86aa712cd6f1c7656eeb17db
-0, 28, 28,1,   460800, 8c86ee0f02fabccaed8d8fc8babd031e
-0, 29, 29,1,   460800, b3ea1983f7efeec11306445d9ae5d477
-0, 30, 30,1,   460800, 86a4cc9fa7db5ff5ca2be69ad191179f
-0, 31, 31,1,   460800, 8194715afe23ae34a019797a53a6ee2c
-0, 32, 32,1,   460800, 447a153f1c6bb703eff62edfd14e08e0
-0, 33, 33,1,   460800, 092257082789b898dbb14d1f19e79347
-0, 34, 34,1,   460800, d6320d204a119cfeef5645a4118bc600
-0, 35, 35,1,   460800, 2ee710deae4bb0d156528797ad1c4981
-0, 36, 36,1,   460800, 1256eac030985c04c4501ad5a72e9d66
-0, 37, 37,1,   460800, f16ad8c1aa572be7666c7907ce4aebbc
-0, 38, 38,1,   460800, 865088cbd47d0151b62a45d5426c8216
-0, 39, 39,1,   460800, 26c78ca43d93c6da153f3dea5d945e0e
-0, 40, 40,1,   460800, d775d6705c965401ccc143d5ae432938
-0, 41, 41,1,   460800, f9837d514753c59e6776452d9043524f
-0, 42, 42,1,   460800, 8463f5172914828abcc770f888bfd183
-0, 43, 43,1,   460800, 3108557748cfb7965b33b16b35359de0
-0, 44, 44,1,   460800, 477d596944e028dd72c207bb6e6b22de
-0, 45, 45,1,   460800, 69e4ffbd600c8d8bc070d7d7324ee2b1
-0, 46, 

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

[Michael Niedermayer]

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  > > wrote:
> > 
> > > On Mon, Oct 23, 2017 at 04:18:28PM -0700, Sasi Inguva wrote:
> > > > Signed-off-by: Sasi Inguva 
> > > > ---
> > > >  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 = >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@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 
> > 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

TESTmov-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-count2017-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,

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

[Michael Niedermayer]

On Thu, Oct 26, 2017 at 08:51:50PM -0700, Sasi Inguva wrote:
> On Tue, Oct 24, 2017 at 2:48 PM, Michael Niedermayer  > wrote:
> 
> > On Mon, Oct 23, 2017 at 04:18:28PM -0700, Sasi Inguva wrote:
> > > Signed-off-by: Sasi Inguva 
> > > ---
> > >  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 = >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@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 
> 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

thanks

[...]

-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Let us carefully observe those good qualities wherein our enemies excel us
and endeavor to excel them, by avoiding what is faulty, and imitating what
is excellent in them. -- Plutarch


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Sasi Inguva]

On Tue, Oct 24, 2017 at 2:48 PM, Michael Niedermayer  wrote:

> On Mon, Oct 23, 2017 at 04:18:28PM -0700, Sasi Inguva wrote:
> > Signed-off-by: Sasi Inguva 
> > ---
> >  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 = >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@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
>
From 188602e662868a79c7f85e9193e9aedc9ba1a170 Mon Sep 17 00:00:00 2001
From: Sasi Inguva 
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.

Signed-off-by: Sasi Inguva 
---
 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(-)
 create mode 100644 tests/ref/fate/mov-invalid-elst-entry-count

diff --git a/libavformat/mov.c b/libavformat/mov.c
index b22a116140..bd019317bf 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,21 @@ 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) {
+if (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;
+  

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

[Michael Niedermayer]

On Mon, Oct 23, 2017 at 04:18:28PM -0700, Sasi Inguva wrote:
> Signed-off-by: Sasi Inguva 
> ---
>  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 = >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


[...]

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


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Sasi Inguva]

On Fri, Oct 20, 2017 at 4:26 PM, Michael Niedermayer  wrote:

> On Wed, Oct 18, 2017 at 08:12:50PM -0700, Sasi Inguva wrote:
> > Signed-off-by: Sasi Inguva 
> > ---
> >  libavformat/mov.c   | 16 +++-
> >  tests/fate/mov.mak  |  6 ++-
> >  tests/ref/fate/mov-invalid-elst-entry-count | 57
> +
> >  3 files changed, 76 insertions(+), 3 deletions(-)
> >  create mode 100644 tests/ref/fate/mov-invalid-elst-entry-count
> >
> > diff --git a/libavformat/mov.c b/libavformat/mov.c
> > index b22a116140..8d2602c739 100644
> > --- a/libavformat/mov.c
> > +++ b/libavformat/mov.c
> > @@ -4596,7 +4596,7 @@ free_and_return:
> >  static int mov_read_elst(MOVContext *c, AVIOContext *pb, MOVAtom atom)
> >  {
> >  MOVStreamContext *sc;
> > -int i, edit_count, version;
> > +int i, edit_count, version, elst_entry_size;
> >
> >  if (c->fc->nb_streams < 1 || c->ignore_editlist)
> >  return 0;
> > @@ -4605,6 +4605,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 &&
>
> the edit_count * elst_entry_size can overflow
>
> Thanks. Changed elst_entry_size to int64

>
> > +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 +4626,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 = >elst_data[i];
>
> isnt atom.size < 0 an error condition ?
> this could would not error out in that case
> is that intended ?
>
> we are comparing that atom.size is exactly equal to edit_count *
elst_entry_size . The for loop will decrease elst_entry_size bytes from
atom.size in each iteration, so it can only be < 0 iff  it wasn't exactly
equal to edit_count * elst_entry_size .


> [...]
> --
> Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> Complexity theory is the science of finding the exact solution to an
> approximation. Benchmarking OTOH is finding an approximation of the exact
>
> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
>
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Sasi Inguva]

Signed-off-by: Sasi Inguva 
---
 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 = >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);
 
diff --git a/tests/fate/mov.mak b/tests/fate/mov.mak
index cfdada7a2e..ef250a12d8 100644
--- a/tests/fate/mov.mak
+++ b/tests/fate/mov.mak
@@ -6,6 +6,7 @@ FATE_MOV = fate-mov-3elist \
fate-mov-1elist-ends-last-bframe \
fate-mov-2elist-elist1-ends-bframe \
fate-mov-3elist-encrypted \
+   fate-mov-invalid-elst-entry-count \
fate-mov-gpmf-remux \
 
 FATE_MOV_FFPROBE = fate-mov-aac-2048-priming \
@@ -39,6 +40,9 @@ fate-mov-1elist-ends-last-bframe: CMD = framemd5 -i 
$(TARGET_SAMPLES)/mov/mov-1e
 # Makes sure that we handle timestamps of packets in case of multiple edit 
lists with one of them ending on a B-frame correctly.
 fate-mov-2elist-elist1-ends-bframe: CMD = framemd5 -i 
$(TARGET_SAMPLES)/mov/mov-2elist-elist1-ends-bframe.mov
 
+# Makes sure that we handle invalid edit list entry count correctly.
+fate-mov-invalid-elst-entry-count: CMD = framemd5 -i 
$(TARGET_SAMPLES)/mov/invalid_elst_entry_count.mov
+
 fate-mov-aac-2048-priming: CMD = run ffprobe$(PROGSSUF)$(EXESUF) -show_packets 
-print_format compact $(TARGET_SAMPLES)/mov/aac-2048-priming.mov
 
 fate-mov-zombie: CMD = run ffprobe$(PROGSSUF)$(EXESUF) -show_streams 
-show_packets -show_frames -bitexact -print_format compact 
$(TARGET_SAMPLES)/mov/white_zombie_scrunch-part.mov
diff --git a/tests/ref/fate/mov-invalid-elst-entry-count 
b/tests/ref/fate/mov-invalid-elst-entry-count
new file mode 100644
index 00..13b575816b
--- /dev/null
+++ b/tests/ref/fate/mov-invalid-elst-entry-count
@@ -0,0 +1,57 @@
+#format: frame checksums
+#version: 2
+#hash: MD5
+#tb 0: 1/24
+#media_type 0: video
+#codec_id 0: rawvideo
+#dimensions 0: 640x480
+#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
+0,  4,  4,1,   460800, eb28174acfceb868b9058757bed049c5
+0,  5,  5,1,   460800, 9732bd4a58884dbf2be48d819433130f
+0,  6,  6,1,   460800, 0c553fb530cf042eb84f5b13817a96a6
+0,  7,  7,1,   460800, 621f02aded5e35fa1f373afd3ed283bd
+0,  8,  8,1,   460800, c76167c6bda91f657708c88252ea315d
+0,  9,  9,1,   460800, 872df2d8c522e2440ddd04bca7dce497
+0, 10, 10,1,   460800, 6ee9154e48c5132ad4ba122b255bd2bb
+0, 

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

[Carl Eugen Hoyos]

2017-10-21 1:28 GMT+02:00 James Almer :

> Could you look at ticket #6714 while at it? (And others also potentially
> related to edit lists).

https://trac.ffmpeg.org/query?status=new=open=~edts

Carl Eugen
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[James Almer]

On 10/19/2017 12:12 AM, Sasi Inguva wrote:
> Signed-off-by: Sasi Inguva 
> ---
>  libavformat/mov.c   | 16 +++-
>  tests/fate/mov.mak  |  6 ++-
>  tests/ref/fate/mov-invalid-elst-entry-count | 57 
> +
>  3 files changed, 76 insertions(+), 3 deletions(-)
>  create mode 100644 tests/ref/fate/mov-invalid-elst-entry-count
> 
> diff --git a/libavformat/mov.c b/libavformat/mov.c
> index b22a116140..8d2602c739 100644
> --- a/libavformat/mov.c
> +++ b/libavformat/mov.c
> @@ -4596,7 +4596,7 @@ free_and_return:
>  static int mov_read_elst(MOVContext *c, AVIOContext *pb, MOVAtom atom)
>  {
>  MOVStreamContext *sc;
> -int i, edit_count, version;
> +int i, edit_count, version, elst_entry_size;
>  
>  if (c->fc->nb_streams < 1 || c->ignore_editlist)
>  return 0;
> @@ -4605,6 +4605,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 +4626,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 = >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);
>  
> diff --git a/tests/fate/mov.mak b/tests/fate/mov.mak
> index cfdada7a2e..012e6f61b7 100644
> --- a/tests/fate/mov.mak
> +++ b/tests/fate/mov.mak
> @@ -6,7 +6,8 @@ FATE_MOV = fate-mov-3elist \
> fate-mov-1elist-ends-last-bframe \
> fate-mov-2elist-elist1-ends-bframe \
> fate-mov-3elist-encrypted \
> -   fate-mov-gpmf-remux \
> +fate-mov-invalid-elst-entry-count \
> +fate-mov-gpmf-remux \

Don't use tabs.

Could you look at ticket #6714 while at it? (And others also potentially
related to edit lists).
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Michael Niedermayer]

On Wed, Oct 18, 2017 at 08:12:50PM -0700, Sasi Inguva wrote:
> Signed-off-by: Sasi Inguva 
> ---
>  libavformat/mov.c   | 16 +++-
>  tests/fate/mov.mak  |  6 ++-
>  tests/ref/fate/mov-invalid-elst-entry-count | 57 
> +
>  3 files changed, 76 insertions(+), 3 deletions(-)
>  create mode 100644 tests/ref/fate/mov-invalid-elst-entry-count
> 
> diff --git a/libavformat/mov.c b/libavformat/mov.c
> index b22a116140..8d2602c739 100644
> --- a/libavformat/mov.c
> +++ b/libavformat/mov.c
> @@ -4596,7 +4596,7 @@ free_and_return:
>  static int mov_read_elst(MOVContext *c, AVIOContext *pb, MOVAtom atom)
>  {
>  MOVStreamContext *sc;
> -int i, edit_count, version;
> +int i, edit_count, version, elst_entry_size;
>  
>  if (c->fc->nb_streams < 1 || c->ignore_editlist)
>  return 0;
> @@ -4605,6 +4605,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 &&

the edit_count * elst_entry_size can overflow


> +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 +4626,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 = >elst_data[i];

isnt atom.size < 0 an error condition ?
this could would not error out in that case
is that intended ?

[...]
-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Complexity theory is the science of finding the exact solution to an
approximation. Benchmarking OTOH is finding an approximation of the exact


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Michael Niedermayer]

On Wed, Oct 18, 2017 at 08:18:27PM -0700, Sasi Inguva wrote:
> Attaching fate sample,

uploaded

thx

[...]

-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Those who are too smart to engage in politics are punished by being
governed by those who are dumber. -- Plato 


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Sasi Inguva]

Signed-off-by: Sasi Inguva 
---
 libavformat/mov.c   | 16 +++-
 tests/fate/mov.mak  |  6 ++-
 tests/ref/fate/mov-invalid-elst-entry-count | 57 +
 3 files changed, 76 insertions(+), 3 deletions(-)
 create mode 100644 tests/ref/fate/mov-invalid-elst-entry-count

diff --git a/libavformat/mov.c b/libavformat/mov.c
index b22a116140..8d2602c739 100644
--- a/libavformat/mov.c
+++ b/libavformat/mov.c
@@ -4596,7 +4596,7 @@ free_and_return:
 static int mov_read_elst(MOVContext *c, AVIOContext *pb, MOVAtom atom)
 {
 MOVStreamContext *sc;
-int i, edit_count, version;
+int i, edit_count, version, elst_entry_size;
 
 if (c->fc->nb_streams < 1 || c->ignore_editlist)
 return 0;
@@ -4605,6 +4605,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 +4626,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 = >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);
 
diff --git a/tests/fate/mov.mak b/tests/fate/mov.mak
index cfdada7a2e..012e6f61b7 100644
--- a/tests/fate/mov.mak
+++ b/tests/fate/mov.mak
@@ -6,7 +6,8 @@ FATE_MOV = fate-mov-3elist \
fate-mov-1elist-ends-last-bframe \
fate-mov-2elist-elist1-ends-bframe \
fate-mov-3elist-encrypted \
-   fate-mov-gpmf-remux \
+  fate-mov-invalid-elst-entry-count \
+  fate-mov-gpmf-remux \
 
 FATE_MOV_FFPROBE = fate-mov-aac-2048-priming \
fate-mov-zombie \
@@ -39,6 +40,9 @@ fate-mov-1elist-ends-last-bframe: CMD = framemd5 -i 
$(TARGET_SAMPLES)/mov/mov-1e
 # Makes sure that we handle timestamps of packets in case of multiple edit 
lists with one of them ending on a B-frame correctly.
 fate-mov-2elist-elist1-ends-bframe: CMD = framemd5 -i 
$(TARGET_SAMPLES)/mov/mov-2elist-elist1-ends-bframe.mov
 
+# Makes sure that we handle invalid edit list entry count correctly.
+fate-mov-invalid-elst-entry-count: CMD = framemd5 -i 
$(TARGET_SAMPLES)/mov/invalid_elst_entry_count.mov
+
 fate-mov-aac-2048-priming: CMD = run ffprobe$(PROGSSUF)$(EXESUF) -show_packets 
-print_format compact $(TARGET_SAMPLES)/mov/aac-2048-priming.mov
 
 fate-mov-zombie: CMD = run ffprobe$(PROGSSUF)$(EXESUF) -show_streams 
-show_packets -show_frames -bitexact -print_format compact 
$(TARGET_SAMPLES)/mov/white_zombie_scrunch-part.mov
diff --git a/tests/ref/fate/mov-invalid-elst-entry-count 
b/tests/ref/fate/mov-invalid-elst-entry-count
new file mode 100644
index 00..13b575816b
--- /dev/null
+++ b/tests/ref/fate/mov-invalid-elst-entry-count
@@ -0,0 +1,57 @@
+#format: frame checksums
+#version: 2
+#hash: MD5
+#tb 0: 1/24
+#media_type 0: video
+#codec_id 0: rawvideo
+#dimensions 0: 640x480
+#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
+0,  4,  4,1,   460800, eb28174acfceb868b9058757bed049c5
+0,  5,  5,1,   460800, 9732bd4a58884dbf2be48d819433130f
+0,  6,  6,1,   460800, 0c553fb530cf042eb84f5b13817a96a6
+0,  7,  7,1,   460800, 621f02aded5e35fa1f373afd3ed283bd
+0,  8,  8,1,   460800, c76167c6bda91f657708c88252ea315d
+0,  9,  9,1,   460800,