diff mbox

[FFmpeg-devel] avformat/mov.c: require tfhd to begin parsing trun

Message ID 20190207001251.23599-1-chcunningham@chromium.org
State Accepted
Commit 3ea87e5d9ea075d5b3c0f4f8c6c48e514b454cbe
Headers show

Commit Message

chcunningham@chromium.org Feb. 7, 2019, 12:12 a.m. UTC
Detecting missing tfhd avoids re-using tfhd track info from the previous
moof. For files with multiple tracks, this may make a mess of the
avindex and fragindex, which can later trigger av_assert0 in
mov_read_trun().
---
 libavformat/isom.h |  1 +
 libavformat/mov.c  | 10 ++++++++++
 2 files changed, 11 insertions(+)

Comments

Derek Buitenhuis Feb. 7, 2019, 3:50 p.m. UTC | #1
On 07/02/2019 00:12, chcunningham wrote:
> Detecting missing tfhd avoids re-using tfhd track info from the previous
> moof. For files with multiple tracks, this may make a mess of the
> avindex and fragindex, which can later trigger av_assert0 in
> mov_read_trun().
> ---
>  libavformat/isom.h |  1 +
>  libavformat/mov.c  | 10 ++++++++++
>  2 files changed, 11 insertions(+)

I think this seems reasonable.

Is the intent to entirely reject these files, or only the broken parts?
(lack of patch context for how it cascades for me)

- Derek
chcunningham@chromium.org Feb. 7, 2019, 7:30 p.m. UTC | #2
This will reject the file entirely. The testcase I have (can share once
chromium bug is fixed) was previously hitting an av_assert0 in
mov_read_trun, arguably also a total rejection ;). Recovery for this could
be pretty tricky since the dropped trun will break decode dependencies. I
would be surprised to find files with missing tfhd's in the wild (outside
of fuzzer / maliciously crafted sorts).

On Thu, Feb 7, 2019 at 7:58 AM Derek Buitenhuis <derek.buitenhuis@gmail.com>
wrote:

> On 07/02/2019 00:12, chcunningham wrote:
> > Detecting missing tfhd avoids re-using tfhd track info from the previous
> > moof. For files with multiple tracks, this may make a mess of the
> > avindex and fragindex, which can later trigger av_assert0 in
> > mov_read_trun().
> > ---
> >  libavformat/isom.h |  1 +
> >  libavformat/mov.c  | 10 ++++++++++
> >  2 files changed, 11 insertions(+)
>
> I think this seems reasonable.
>
> Is the intent to entirely reject these files, or only the broken parts?
> (lack of patch context for how it cascades for me)
>
> - Derek
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
Derek Buitenhuis Feb. 7, 2019, 8:03 p.m. UTC | #3
On 07/02/2019 19:30, Chris Cunningham wrote:
> This will reject the file entirely. The testcase I have (can share once
> chromium bug is fixed) was previously hitting an av_assert0 in
> mov_read_trun, arguably also a total rejection ;). Recovery for this could
> be pretty tricky since the dropped trun will break decode dependencies. I
> would be surprised to find files with missing tfhd's in the wild (outside
> of fuzzer / maliciously crafted sorts).

You would be surprised what kind of garbage things can produce...

In any case, sounds OK to me.

- Derek
Michael Niedermayer Feb. 8, 2019, 11 a.m. UTC | #4
On Thu, Feb 07, 2019 at 08:03:01PM +0000, Derek Buitenhuis wrote:
> On 07/02/2019 19:30, Chris Cunningham wrote:
> > This will reject the file entirely. The testcase I have (can share once
> > chromium bug is fixed) was previously hitting an av_assert0 in
> > mov_read_trun, arguably also a total rejection ;). Recovery for this could
> > be pretty tricky since the dropped trun will break decode dependencies. I
> > would be surprised to find files with missing tfhd's in the wild (outside
> > of fuzzer / maliciously crafted sorts).
> 
> You would be surprised what kind of garbage things can produce...
> 
> In any case, sounds OK to me.

will apply

thanks

[...]
diff mbox

Patch

diff --git a/libavformat/isom.h b/libavformat/isom.h
index e629663949..69452cae8e 100644
--- a/libavformat/isom.h
+++ b/libavformat/isom.h
@@ -87,6 +87,7 @@  typedef struct MOVAtom {
 struct MOVParseTableEntry;
 
 typedef struct MOVFragment {
+    int found_tfhd;
     unsigned track_id;
     uint64_t base_data_offset;
     uint64_t moof_offset;
diff --git a/libavformat/mov.c b/libavformat/mov.c
index 9b9739f788..6afb656dae 100644
--- a/libavformat/mov.c
+++ b/libavformat/mov.c
@@ -1366,6 +1366,9 @@  static void fix_frag_index_entries(MOVFragmentIndex *frag_index, int index,
 
 static int mov_read_moof(MOVContext *c, AVIOContext *pb, MOVAtom atom)
 {
+    // Set by mov_read_tfhd(). mov_read_trun() will reject files missing tfhd.
+    c->fragment.found_tfhd = 0;
+
     if (!c->has_looked_for_mfra && c->use_mfra_for > 0) {
         c->has_looked_for_mfra = 1;
         if (pb->seekable & AVIO_SEEKABLE_NORMAL) {
@@ -4544,6 +4547,8 @@  static int mov_read_tfhd(MOVContext *c, AVIOContext *pb, MOVAtom atom)
     MOVTrackExt *trex = NULL;
     int flags, track_id, i;
 
+    c->fragment.found_tfhd = 1;
+
     avio_r8(pb); /* version */
     flags = avio_rb24(pb);
 
@@ -4679,6 +4684,11 @@  static int mov_read_trun(MOVContext *c, AVIOContext *pb, MOVAtom atom)
     AVIndexEntry *new_entries;
     MOVFragmentStreamInfo * frag_stream_info;
 
+    if (!frag->found_tfhd) {
+        av_log(c->fc, AV_LOG_ERROR, "trun track id unknown, no tfhd was found\n");
+        return AVERROR_INVALIDDATA;
+    }
+
     for (i = 0; i < c->fc->nb_streams; i++) {
         if (c->fc->streams[i]->id == frag->track_id) {
             st = c->fc->streams[i];