[vlc-devel] [PATCH 1/4] mkv/demux: fix 17567: null-pointer dereference in EnsureDuration

[Steve Lhomme]

On Wed, Nov 2, 2016 at 11:16 AM, Filip Roséen <filip at atch.se> wrote:
> Hi Steve,
>
> Could you provide samples, where the decision to have non-finite clusters is
> justifiable, that break with the proposed (and already merged) changes?

As I said, the already merge change is wrong. Non-finite streams
should be treated as finite streams when it comes to the duration, so
long as we know the file end (I suppose STREAM_CAN_FASTSEEK implies
that).

> On 2016-11-02 11:08, Steve Lhomme wrote:
>
>  Again, you're setting aside half of the other cases: files with finite
>  size and no Cues. The same piece of code to try to find the duration
>  will be applied. So at the very least if you don't want to bother
>  looking for the duration for these files then forbid it for these too.
>
> I am not setting that aside, not at all.
>
> If the file has no cues, not metadata that express the duration of the file,
> and has non-finite clusters that we need to walk through (all of them) in
> order to find the duration I think it makes more sense to simply say “hey,
> this file has no duration which I feel like looking for - you can play it,
> but you will not have a duration”.

But why is it OK with a file with missing/bogus Cues then ? This is
the exact same code that will try to find the duration.

Now I'm beginning to see a difference though. If one (and probably
all) Cluster has no known size we need to parse all elements/Blocks of
that Cluster. Whereas for a finite size Cluster we just skip over that
Cluster until we find no Cluster. That's a lot less skipping and
header reading.

So I agree that for non-finite Clusters we should not try too hard.
But that decision should be made in EnsureDuration(). Because if the
Cues is present we don't have to do that huge data parsing.

It makes sense than a live Matroska recorder would keep track of where
it wrote each Cluster and write a Cues at the end, making the file a
lot easier to seek.

How about the attached patch ?

>  And what about a file that would have one Cluster, a finite size, Cues
>  (1 Cluster is easy) and 1GB of Blocks in that Cluster ? It will also
>  take forever to parse just to find the last duration of the unique
>  Cluster. We might as well remove EnsureDuration().
>
> That is not at all the same as semantically saying that we do not probe
> non-finite clusters for their duration.
>
> Having a cluster with N gigabytes blocks is b0rked and has no true rationale
> behind it, but having a stream with clusters of unknown size has valid
> applications (and these valid applications do not play well with “finite
> duration” in either case).
>
>  Looking at the code, the Cluster duration is used only for
>  VirtualSegments, ie for chaining segments. But that duration, is also
>  used for seeking.
>
>     f_duration = p_current_vsegment->Duration();
>
>     if( p_sys->f_duration < 0 )
>      {
>          msg_Warn( p_demux, "cannot seek without duration!");
>          return;
>      }
>
>  We won't be able to seek in these files at all. It seems harsh to
>  forbid seeking for these files by default (maybe not the one with a
>  single huge Cluster). An option to disable the duration check is some
>  case might be useful though. There's already the `b_preparsing` flag
>  we could use to skip this. It may be renamed to something more generic
>  like b_skip_long_unnecessary_operations.
>
> You are disregarding the fact that it will only happen for files where
> non-finite clusters are used, and most files have finite clusters (as
> non-finite clusters are generally only used when content is streamed).
>
>
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-mkv-demux-do-not-parse-the-whole-infinite-Clusters-t.patch
Type: application/octet-stream
Size: 1117 bytes
Desc: not available
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20161102/6b92bf1b/attachment.obj>