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

[Filip Roséen]

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?

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"*.

> 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). 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20161102/c0cefbec/attachment.html>