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

[Steve Lhomme]

On Wed, Nov 2, 2016 at 10:43 AM, Filip Roséen <filip at atch.se> wrote:
> Hi,
>
> On 2016-11-02 10:29, Steve Lhomme wrote:
>
>  To summarize the issue, is STREAM_CAN_FASTSEEK_FORWARD needed ?
>
> No, what one could argue is necessary is a straight-forward way to know
> whether a stream has a finite size or not (which would lead to problems in
> terms of implementation, since there’s nothing that really guarantees this
> for all/most accessors).
>
> Also, it is not really about “seeking forward” given that we need to exhaust
> (somewhat byte-wise) every cluster stumbled upon in order to find the next
> one (since we cannot read the cluster header, and then simply jump to the
> end of it). I am not sure how libmatroska/libebml handles this internally.
>
> STREAM_CAN_FASTSEEK_FORWARD sounds more like STREAM_CAN_FASTSKIP, but I
> really don’t think that we need more controls in that regard; this
> particular issue boils down to semantics, not the current state of stream_t.
>
>  As you say, it's a way to know the duration even if it's not written
>  specifically. So that's already a fringe case. If the file is
>  particularly badly muxed, then too bad it's normal that it doesn't
>  play quickly. But we shouldn't remove the feature of knowing the
>  duration of a fully written file just to avoid to read bad files too
>  much. The same applies for a file with no duration and no Cues. All
>  Clusters and the whole last Cluster will need to be parsed. And that
>  has nothing to do with the finite size. It has everything to do with
>  how fast we can seek through all these data.
>
> Yes, we already try to compensate for a badly muxed stream, should we then
> take another step in the “let’s save the muxer” direction, and forgive it
> for generating clusters with a non-finite size (where the decision to not
> specifying the size might actually be a valid one)?
>
> In my opinion the theory behind my argumentation holds ground; if a cluster
> has no size, expecting it to end might be practically possible, but in
> theory it is not.

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

>  So yes, f2756634ed3601fa7c00e974533f68fcbbb12d06 is wrong IMO too. I
>  overlooked it.
>
> If anything I think it should be an opt-in option if we decide to query
> non-finite clusters for their duration (in order to get the duration of the
> entire file).

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.

>
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel
>