[vlc-devel] [PATCH v5 3/7] upnp: Split s_lock into two locks to solve deadlock issue

[Steve Lhomme]

👇🏼

On 2019-11-24 23:33, Johan Gunnarsson wrote:
> If libupnp executes a callback while vlc runs UpnpInstanceWrapper dtor, vlc will
> deadlock. This happens because the callback handler and UpnpInstanceWrapper dtor
> both takes s_lock, and Upnp_Finish waits for all callbacks to finish.
> UpnpUnRegisterRootDevice casues libupnp to broadcast "byebye" advertisements,

"causes"

> which libupnp picks up again and delivers as callbacks.
> 
> This patch separates the locks into one that protects the UpnpInstanceWrapper
> refcounts and one that protects the list of listeners.
> ---
>   modules/services_discovery/upnp-wrapper.cpp | 17 +++++++++--------
>   modules/services_discovery/upnp-wrapper.hpp |  3 ++-
>   2 files changed, 11 insertions(+), 9 deletions(-)
> 
> diff --git a/modules/services_discovery/upnp-wrapper.cpp b/modules/services_discovery/upnp-wrapper.cpp
> index 2e59061437..b3ea9b6716 100644
> --- a/modules/services_discovery/upnp-wrapper.cpp
> +++ b/modules/services_discovery/upnp-wrapper.cpp
> @@ -79,7 +79,8 @@ static const char *mediarenderer_desc =
>   
>   UpnpInstanceWrapper* UpnpInstanceWrapper::s_instance;
>   UpnpInstanceWrapper::Listeners UpnpInstanceWrapper::s_listeners;
> -vlc_mutex_t UpnpInstanceWrapper::s_lock = VLC_STATIC_MUTEX;
> +vlc_mutex_t UpnpInstanceWrapper::s_lock_refcount = VLC_STATIC_MUTEX;

s_lock_instance might be a better name.

> +vlc_mutex_t UpnpInstanceWrapper::s_lock_listeners = VLC_STATIC_MUTEX;
>   
>   UpnpInstanceWrapper::UpnpInstanceWrapper()
>       : m_client_handle( -1 )
> @@ -100,7 +101,7 @@ UpnpInstanceWrapper::~UpnpInstanceWrapper()
>   
>   UpnpInstanceWrapper *UpnpInstanceWrapper::get(vlc_object_t *p_obj)
>   {
> -    vlc_mutex_locker lock( &s_lock );
> +    vlc_mutex_locker lock( &s_lock_refcount );
>       if ( s_instance == NULL )
>       {
>           UpnpInstanceWrapper* instance = new(std::nothrow) UpnpInstanceWrapper;
> @@ -168,7 +169,7 @@ UpnpInstanceWrapper *UpnpInstanceWrapper::get(vlc_object_t *p_obj)
>   void UpnpInstanceWrapper::release()
>   {
>       UpnpInstanceWrapper *p_delete = NULL;
> -    vlc::threads::mutex_locker lock( &s_lock );
> +    vlc::threads::mutex_locker lock( &s_lock_refcount );
>       if (--s_instance->m_refcount == 0)
>       {
>           p_delete = s_instance;
> @@ -189,7 +190,7 @@ UpnpDevice_Handle UpnpInstanceWrapper::device_handle() const
>   
>   int UpnpInstanceWrapper::Callback(Upnp_EventType event_type, UpnpEventPtr p_event, void *p_user_data)
>   {
> -    vlc::threads::mutex_locker lock( &s_lock );
> +    vlc::threads::mutex_locker lock( &s_lock_listeners );
>       for (Listeners::iterator iter = s_listeners.begin(); iter != s_listeners.end(); ++iter)
>       {
>           (*iter)->onEvent(event_type, p_event, p_user_data);
> @@ -200,14 +201,14 @@ int UpnpInstanceWrapper::Callback(Upnp_EventType event_type, UpnpEventPtr p_even
>   
>   void UpnpInstanceWrapper::addListener(ListenerPtr listener)
>   {
> -    vlc::threads::mutex_locker lock( &s_lock );
> +    vlc::threads::mutex_locker lock( &s_lock_listeners );
>       if ( std::find( s_listeners.begin(), s_listeners.end(), listener) == s_listeners.end() )
>           s_listeners.push_back( std::move(listener) );
>   }
>   
>   void UpnpInstanceWrapper::removeListener(ListenerPtr listener)
>   {
> -    vlc::threads::mutex_locker lock( &s_lock );
> +    vlc::threads::mutex_locker lock( &s_lock_listeners );
>       Listeners::iterator iter = std::find( s_listeners.begin(), s_listeners.end(), listener );
>       if ( iter != s_listeners.end() )
>           s_listeners.erase( iter );
> @@ -215,7 +216,7 @@ void UpnpInstanceWrapper::removeListener(ListenerPtr listener)
>   
>   void UpnpInstanceWrapper::startMediaRenderer( vlc_object_t *p_obj )
>   {
> -    vlc::threads::mutex_locker lock( &s_lock );
> +    vlc::threads::mutex_locker lock( &s_lock_refcount );
>       if( m_mediarenderer_refcount == 0 )
>       {
>           int i_res;
> @@ -241,7 +242,7 @@ void UpnpInstanceWrapper::startMediaRenderer( vlc_object_t *p_obj )
>   
>   void UpnpInstanceWrapper::stopMediaRenderer( vlc_object_t *p_obj )
>   {
> -    vlc::threads::mutex_locker lock( &s_lock );
> +    vlc::threads::mutex_locker lock( &s_lock_refcount );
>       m_mediarenderer_refcount--;
>       if( m_mediarenderer_refcount == 0 )
>       {
> diff --git a/modules/services_discovery/upnp-wrapper.hpp b/modules/services_discovery/upnp-wrapper.hpp
> index 665edbf9ac..59fd1f0f5f 100644
> --- a/modules/services_discovery/upnp-wrapper.hpp
> +++ b/modules/services_discovery/upnp-wrapper.hpp
> @@ -71,7 +71,8 @@ public:
>   
>   private:
>       static UpnpInstanceWrapper* s_instance;
> -    static vlc_mutex_t s_lock;
> +    static vlc_mutex_t s_lock_listeners;
> +    static vlc_mutex_t s_lock_refcount;
>       UpnpClient_Handle m_client_handle;
>       UpnpDevice_Handle m_device_handle;
>       int m_refcount;
> -- 
> 2.17.1
> 
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel
>