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

[Michael Niedermayer]

On Tue, Oct 21, 2014 at 01:34:16AM -0500, Rodger Combs wrote:
> 
> > 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);

posted a different patch to fix this, please review/comment

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

The greatest way to live with honor in this world is to be what we pretend
to be. -- Socrates
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20141022/03b346ed/attachment.asc>