| Message ID | 20190207001251.23599-1-chcunningham@chromium.org |
|---|---|
| State | Accepted |
| Commit | 3ea87e5d9ea075d5b3c0f4f8c6c48e514b454cbe |
| Headers | show |
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
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 >
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
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 --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];