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

[Filip Roséen]

Hi Steve,

On 2016-11-02 11:39, Steve Lhomme wrote:

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

I do not agree to this, which you know.

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

Precisely my point from the beginning (though not where to have the
decision take place - that is something I (now) agree with), there is
a huge difference between knowing the cluster size (meaning that we
can jump to the next one directly), and walking through the entire
thing (scanning for the end).

> 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 ?

 - If one really want to make sure that things behave as I have been
   trying to explain, one should add a check for non-finite cluster in
   the while-predicate, inside the if-block modified by your patch,
   and abort computation if such is discovered.

 - The branch to locate the last cluster should be taken even if we
   have cues that have populated `_seeker._cluster_positions` (because
   there is nothing saying that the last entry in the `_seeker`
   corresponds to the last cluster in the file (broken or missing
   cues)).

Other than the above, LGTM.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20161102/f7c6d48f/attachment.html>