[vlc-devel] [PATCH 1/2] libvlc: deactivate the playlist before destroying the preparser

Rémi Denis-Courmont remi at remlab.net
Thu Aug 25 15:50:58 CEST 2016


Le torstaina 25. elokuuta 2016, 15.33.03 EEST Filip Roséen a écrit :
> Hi again,
> 
> On 16/08/25 16:19, Rémi Denis-Courmont wrote:
> > That´s what I don´t understand. How is deactivating the playlist,
> > i.e. shutting down the playlist thread, making a difference here?
> 
> If the playlist is not deactivated prior to destroying the preparser,
> the playlist can indirectly cause the preparser to be invoked again at
> a time where the preparser is either about the be shut down, or has
> already been deleted.

In other words, deactivating the playlist removes one source of preparser 
events. Not the other ones (interfaces, LibVLC media players, VLM(?) 
SDs(?)...). I still don´t understand the point.

> > I don´t actually think that there is anything wrong with the
> > preparser in the first place. The playlist fails to deregister its
> > callbacks; the root cause of the bug lies in the playlist, not the
> > preparser.

> There are plenty of bugs in the current preparser implementation,
> among other things:
> 
>   - the timeout mechanism is broken,
>   - requests for preparsing is blocking at times where it should not,

Preparsing is serialized to avoid stressing system resources (CPU, memory, I/O 
bandwith, network...). This is a feature, not a bug.

>   - there is really no reason for the preparser thread to be in
>     *DETACH* state,

The point is to free the thread resources even though no other thread is 
joining or likely to join any time soon. Though the extent of the "resources" 
depends on the threading implementation.

>   - there are races due to weird locking mechanisms.

> I plan on creating tickets for my findings as soon as possible (prior
> to me submitting new implemenations in order to fix them).
> 
> ---------------------------------------------------------------------
> 
> The Preparser + Playlist
> ---------------------------------------------------------------------
> 
> In terms of talking about the preparser and its relationship with the
> playlist it is broken because the playlist does not own the preparser,
> and the preparser does not own the playlist.

No. The preparser has no dependency on the playlist. It is a shared resource 
by design. The playlist depends on the preparser, and the preparser should be 
created before and destroyed after the playlist.

Where are the chicken and the egg?!

The problem is the playlist leaving dangling callbacks on input items when it 
terminates, which leads to use-after-free if any of the dangling callbacks 
fires.

> The above leads to a *chicken-and-egg* problem, and circumventing it
> is the purpose of the step we are discussing. Though as already stated
> I hope to solve it in a different manner all together (which includes
> a new preparsing implementation).

If the playlist deregistered its callbacks correctly (admittedly not exactly 
easy), there would be no bug. The preparser would fire events after the 
playlist is gone, and yet, there would be no use-after-free.

The problem is the playlist assuming it owns the input items, or more 
specifically, when it does not. Or more specifically, input items can outlive 
the playlist; the playlist wrongly assumes otherwise.

-- 
Rémi Denis-Courmont
http://www.remlab.net/



More information about the vlc-devel mailing list