[vlc-devel] [PATCH] input: Fix race condition on input item titles

[Rémi Denis-Courmont]

Le 28 août 2017 11:01:38 GMT+03:00, "Hugo Beauzée-Luyssen" <hugo at beauzee.fr> a écrit :
>On Sun, Aug 27, 2017, at 10:39 AM, Rémi Denis-Courmont wrote:
>> Le sunnuntaina 27. elokuuta 2017, 11.02.50 EEST Rémi Denis-Courmont a
>> écrit :
>> > Le perjantaina 25. elokuuta 2017, 15.42.59 EEST Hugo
>Beauzée-Luyssen a écrit 
>> :
>> > > Fix #18727
>> > > ---
>> > > 
>> > >  src/input/input.c | 6 +++++-
>> > >  1 file changed, 5 insertions(+), 1 deletion(-)
>> > > 
>> > > diff --git a/src/input/input.c b/src/input/input.c
>> > > index ae5fe03f81..af416b4b54 100644
>> > > --- a/src/input/input.c
>> > > +++ b/src/input/input.c
>> > > @@ -2269,7 +2269,10 @@ static void UpdateGenericFromDemux(
>input_thread_t
>> > > *p_input )
>> > > 
>> > >  static void UpdateTitleListfromDemux( input_thread_t *p_input )
>> > >  {
>> > > 
>> > > -    input_source_t *in = input_priv(p_input)->master;
>> > > +    input_thread_private_t *priv = input_priv(p_input);
>> > > +    input_source_t *in = priv->master;
>> > > +
>> > > +    vlc_mutex_lock( &priv->p_item->lock );
>> > > 
>> > >      /* Delete the preexisting titles */
>> > >      if( in->i_title > 0 )
>> > > 
>> > > @@ -2288,6 +2291,7 @@ static void UpdateTitleListfromDemux(
>input_thread_t
>> > > *p_input ) else
>> > > 
>> > >          in->b_title_demux = true;
>> > > 
>> > > +    vlc_mutex_unlock( &priv->p_item->lock );
>> > > 
>> > >      InitTitle( p_input );
>> > >  
>> > >  }
>> > 
>> > I don´t see where the item is used, so then there is no point
>taking its
>> > lock.
>> 
>
>As pointed out by #18727, it's being used by (at least)
>modules/gui/qt/adapters/seekpoints.cpp:43
>
>> There is in fact a not very good design decision to use the master
>input
>> item 
>> lock to protect some properties of the input thread. But that does
>not
>> seem to 
>> be the case for ->titles.
>> 
>> Also, the lock cannot be taken while calling the demuxer: The demuxer
>is 
>> allowed to acquire that lock - potentially leading to undefined
>recursive 
>> locking or lock inversion.
>
>Oh, my bad then. I assumed it wouldn't.
>I'll do it another way then.
>
>> 
>> -- 
>> 雷米‧德尼-库尔蒙
>> https://www.remlab.net/
>> 
>> _______________________________________________
>> vlc-devel mailing list
>> To unsubscribe or modify your subscription options:
>> https://mailman.videolan.org/listinfo/vlc-devel
>
>
>-- 
>  Hugo Beauzée-Luyssen
>  hugo at beauzee.fr
>_______________________________________________
>vlc-devel mailing list
>To unsubscribe or modify your subscription options:
>https://mailman.videolan.org/listinfo/vlc-devel

I can see code sites where the item lock is taken. But I don't see that being either an actual consistent policy, nor a documented policy, when it comes to titles of the input.

Some of the input code seems to assume that titles are private data of the input thread.
-- 
Envoyé de mon appareil Android avec Courriel K-9 Mail. Veuillez excuser ma brièveté.