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

[Johan Gunnarsson]

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,
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;
+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