diff mbox

[FFmpeg-devel] lavf/mov: ensure only one tkhd per trak

Message ID 20181213215840.214490-1-chcunningham@chromium.org
State Accepted
Commit c9f7b6f7a9fdffa0ab8f3aa84a1f701cf5b3a6e9
Headers show

Commit Message

chcunningham@chromium.org Dec. 13, 2018, 9:58 p.m. UTC 
Chromium fuzzing produced a whacky file with extra tkhds. This caused
an AVStream that was already in use to be corrupted by assigning it a
new id, which blows up later in mov_read_trun because the
MOVFragmentStreamInfo.index_entry now points OOB.
---
 libavformat/mov.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

Comments

Michael Niedermayer Dec. 14, 2018, 5:18 p.m. UTC | #1 
On Thu, Dec 13, 2018 at 01:58:40PM -0800, chcunningham wrote:
> Chromium fuzzing produced a whacky file with extra tkhds. This caused
> an AVStream that was already in use to be corrupted by assigning it a
> new id, which blows up later in mov_read_trun because the
> MOVFragmentStreamInfo.index_entry now points OOB.
> ---
>  libavformat/mov.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)

from a quick look, i did not find a file this breaks

[...]
chcunningham@chromium.org Dec. 14, 2018, 11:12 p.m. UTC | #2 
>
> from a quick look, i did not find a file this breaks
>

Woot.  Baptiste, I'm happy with this last patch if you are.
Baptiste Coudurier Dec. 15, 2018, 7:48 p.m. UTC | #3 
Hi Chris,

> On Dec 14, 2018, at 3:12 PM, Chris Cunningham <chcunningham@chromium.org> wrote:
> 
>> 
>> from a quick look, i did not find a file this breaks
>> 
> 
> Woot.  Baptiste, I'm happy with this last patch if you are.

Yes, looks great.

Thanks a lot!

— 
Baptiste Coudurier
Michael Niedermayer Dec. 16, 2018, 9:18 a.m. UTC | #4 
On Sat, Dec 15, 2018 at 11:48:27AM -0800, Baptiste Coudurier wrote:
> Hi Chris,
> 
> > On Dec 14, 2018, at 3:12 PM, Chris Cunningham <chcunningham@chromium.org> wrote:
> > 
> >> 
> >> from a quick look, i did not find a file this breaks
> >> 
> > 
> > Woot.  Baptiste, I'm happy with this last patch if you are.
> 
> Yes, looks great.

ok, will apply

thx

> 
> Thanks a lot!
> 
> — 
> Baptiste Coudurier
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
diff mbox

Patch

diff --git a/libavformat/mov.c b/libavformat/mov.c
index ec57a05803..6f92742e23 100644
--- a/libavformat/mov.c
+++ b/libavformat/mov.c
@@ -1326,6 +1326,10 @@  static int update_frag_index(MOVContext *c, int64_t offset)
         return -1;
 
     for (i = 0; i < c->fc->nb_streams; i++) {
+        // Avoid building frag index if streams lack track id.
+        if (c->fc->streams[i]->id < 0)
+            return AVERROR_INVALIDDATA;
+
         frag_stream_info[i].id = c->fc->streams[i]->id;
         frag_stream_info[i].sidx_pts = AV_NOPTS_VALUE;
         frag_stream_info[i].tfdt_dts = AV_NOPTS_VALUE;
@@ -4154,7 +4158,7 @@  static int mov_read_trak(MOVContext *c, AVIOContext *pb, MOVAtom atom)
 
     st = avformat_new_stream(c->fc, NULL);
     if (!st) return AVERROR(ENOMEM);
-    st->id = c->fc->nb_streams;
+    st->id = -1;
     sc = av_mallocz(sizeof(MOVStreamContext));
     if (!sc) return AVERROR(ENOMEM);
 
@@ -4438,6 +4442,11 @@  static int mov_read_tkhd(MOVContext *c, AVIOContext *pb, MOVAtom atom)
     st = c->fc->streams[c->fc->nb_streams-1];
     sc = st->priv_data;
 
+    // Each stream (trak) should have exactly 1 tkhd. This catches bad files and
+    // avoids corrupting AVStreams mapped to an earlier tkhd.
+    if (st->id != -1)
+        return AVERROR_INVALIDDATA;
+
     version = avio_r8(pb);
     flags = avio_rb24(pb);
     st->disposition |= (flags & MOV_TKHD_FLAG_ENABLED) ? AV_DISPOSITION_DEFAULT : 0;
@@ -4704,6 +4713,7 @@  static int mov_read_trun(MOVContext *c, AVIOContext *pb, MOVAtom atom)
             break;
         }
     }
+    av_assert0(index_entry_pos <= st->nb_index_entries);
 
     avio_r8(pb); /* version */
     flags = avio_rb24(pb);