[FFmpeg-devel] [PATCH] matroskadec: execute seekheads recursively

[Rodger Combs]

> On Oct 19, 2014, at 23:34, Rodger Combs <rodger.combs at gmail.com> wrote:
> 
> 
>> On Oct 17, 2014, at 17:40, Michael Niedermayer <michaelni at gmx.at <mailto:michaelni at gmx.at>> wrote:
>> 
>> On Fri, Oct 17, 2014 at 03:27:49PM +0200, Michael Niedermayer wrote:
>>> On Fri, Oct 17, 2014 at 01:55:35AM -0500, Rodger Combs wrote:
>>>> 
>>>>> On Oct 17, 2014, at 01:52, Rodger Combs <rodger.combs at gmail.com <mailto:rodger.combs at gmail.com>> wrote:
>>>>> 
>>>>> 
>>>>>> On Oct 17, 2014, at 01:16, Rodger Combs <rodger.combs at gmail.com <mailto:rodger.combs at gmail.com> <mailto:rodger.combs at gmail.com <mailto:rodger.combs at gmail.com>>> wrote:
>>>>>> 
>>>>>> This fixes https://trac.ffmpeg.org/ticket/3934 <https://trac.ffmpeg.org/ticket/3934> <https://trac.ffmpeg.org/ticket/3934 <https://trac.ffmpeg.org/ticket/3934>>, but I'm not sure if there was a good reason for this to be here to begin with. Perhaps a protection against infinite recursion (though I believe EBML_MAX_DEPTH serves that purpose to some degree)?
>>>>>> <0001-matroskadec-execute-seekheads-recursively.patch>
>>>>> 
>>>>> Evidently either I or my mail client screwed up and the patch didn't get attached. Whoops.
>>>>> <0001-matroskadec-execute-seekheads-recursively.patch>
>>>>> Let's try that again...
>>>> 
>>>> Welp, apparently my email client's borked badly in some way. Here's a gist link instead: https://gist.github.com/08f111e72b8b5ddba078 <https://gist.github.com/08f111e72b8b5ddba078>
>>> 
>>> copy and pasted so our archives dont depend on external links as well
>>> as for easy revieweing
>>> 
>>> From 4cf14a9d117da69b64c267e6f982931cfa60a300 Mon Sep 17 00:00:00 2001
>>> From: Rodger Combs <rodger.combs at gmail.com <mailto:rodger.combs at gmail.com>>
>>> Date: Fri, 17 Oct 2014 00:35:12 -0500
>>> Subject: [PATCH] matroskadec: execute seekheads recursively
>>> 
>>> Fixes https://trac.ffmpeg.org/ticket/3934 <https://trac.ffmpeg.org/ticket/3934>
>>> ---
>>> libavformat/matroskadec.c | 1 -
>>> 1 file changed, 1 deletion(-)
>>> 
>>> diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
>>> index b742319..b437e74 100644
>>> --- a/libavformat/matroskadec.c
>>> +++ b/libavformat/matroskadec.c
>>> @@ -1368,7 +1368,6 @@ static int matroska_parse_seekhead_entry(MatroskaDemuxContext *matroska,
>>>     int ret = 0;
>>> 
>>>     if (idx >= seekhead_list->nb_elem            ||
>>> -        seekhead[idx].id == MATROSKA_ID_SEEKHEAD ||
>>>         seekhead[idx].id == MATROSKA_ID_CLUSTER)
>>>         return 0;
>> 
>> ebml_parse() that gets called as a result of this change does not
>> succeed and causes the one and only seekhead entry to return failure
>> so i think this doesnt execute seekheads recursively
>> 
>> [...]
>> 
>> -- 
>> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>> 
>> It is dangerous to be right in matters on which the established authorities
>> are wrong. -- Voltaire
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel at ffmpeg.org <mailto:ffmpeg-devel at ffmpeg.org>
>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel <http://ffmpeg.org/mailman/listinfo/ffmpeg-devel>
> Just stepped through this; I also see ebml_parse() failing with the clip I originally uploaded, but not with the original source file, which I've uploaded to Dropbox: https://www.dropbox.com/s/32ve4hp567ukikt/Maken-Ki%21%20-%20S01E02.mkv?dl=0 <https://www.dropbox.com/s/32ve4hp567ukikt/Maken-Ki%21%20-%20S01E02.mkv?dl=0>
> 
> 

On IRC, the consensus seems to be:
1. This works as expected
2. We're not sure if it could lead to an infinite loop if a SeekHead pointed at itself, or at another SeekHead pointing to it; could someone with more experience with matroskadec.c confirm?
3. matroska_read_seek currently returns 0 and fails to seek if the file has no Cues; it should return -1 and let libavformat fallback on legacy behavior (as it does if it encounters an invalid SeekHead or Cues). This is fixed by this patch:

From acd730174bbbc2f95be1e697ed4fb6bf33c30f05 Mon Sep 17 00:00:00 2001
From: Rodger Combs <rodger.combs at gmail.com>
Date: Tue, 21 Oct 2014 01:33:08 -0500
Subject: [PATCH] matroskadec: fail matroska_read_seek if no index was parsed

---
 libavformat/matroskadec.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
index b742319..a50e9ea 100644
--- a/libavformat/matroskadec.c
+++ b/libavformat/matroskadec.c
@@ -2917,7 +2917,7 @@ static int matroska_read_seek(AVFormatContext *s, int stream_index,
         matroska_parse_cues(matroska);
     }
 
-    if (!st->nb_index_entries)
+    if (!st->nb_index_entries || !matroska->index.nb_elem)
         goto err;
     timestamp = FFMAX(timestamp, st->index_entries[0].timestamp);
 
-- 
1.9.1