[vlc-devel] [PATCH v6 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 causes 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 acdddd9b46..1ad3b3eb21 100644
--- a/modules/services_discovery/upnp-wrapper.cpp
+++ b/modules/services_discovery/upnp-wrapper.cpp
@@ -80,7 +80,8 @@ static const char *mediarenderer_desc_template   =
 
 UpnpInstanceWrapper* UpnpInstanceWrapper::s_instance;
 UpnpInstanceWrapper::Listeners UpnpInstanceWrapper::s_listeners;
-vlc_mutex_t UpnpInstanceWrapper::s_lock = VLC_STATIC_MUTEX;
+vlc_mutex_t UpnpInstanceWrapper::s_lock_instance = VLC_STATIC_MUTEX;
+vlc_mutex_t UpnpInstanceWrapper::s_lock_listeners = VLC_STATIC_MUTEX;
 
 std::string generateUDN()
 {
@@ -134,7 +135,7 @@ UpnpInstanceWrapper::~UpnpInstanceWrapper()
 
 UpnpInstanceWrapper *UpnpInstanceWrapper::get(vlc_object_t *p_obj)
 {
-    vlc_mutex_locker lock( &s_lock );
+    vlc_mutex_locker lock( &s_lock_instance );
     if ( s_instance == NULL )
     {
         UpnpInstanceWrapper* instance = new(std::nothrow) UpnpInstanceWrapper;
@@ -209,7 +210,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_instance );
     if (--s_instance->m_refcount == 0)
     {
         p_delete = s_instance;
@@ -235,7 +236,7 @@ std::string UpnpInstanceWrapper::udn() 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);
@@ -246,14 +247,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 );
@@ -261,7 +262,7 @@ void UpnpInstanceWrapper::removeListener(ListenerPtr listener)
 
 bool UpnpInstanceWrapper::startMediaRenderer( vlc_object_t *p_obj )
 {
-    vlc::threads::mutex_locker lock( &s_lock );
+    vlc::threads::mutex_locker lock( &s_lock_instance );
     if( m_mediarenderer_refcount == 0 )
     {
         std::string friendly_name( "VLC media player" );
@@ -311,7 +312,7 @@ bool UpnpInstanceWrapper::startMediaRenderer( vlc_object_t *p_obj )
 
 bool UpnpInstanceWrapper::stopMediaRenderer( vlc_object_t *p_obj )
 {
-    vlc::threads::mutex_locker lock( &s_lock_lock );
+    vlc::threads::mutex_locker lock( &s_lock_instance );
     if( m_mediarenderer_refcount == 1 )
     {
         int i_res;
diff --git a/modules/services_discovery/upnp-wrapper.hpp b/modules/services_discovery/upnp-wrapper.hpp
index a831a93632..1a3bd9c2ee 100644
--- a/modules/services_discovery/upnp-wrapper.hpp
+++ b/modules/services_discovery/upnp-wrapper.hpp
@@ -69,7 +69,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_instance;
     UpnpClient_Handle m_client_handle;
     UpnpDevice_Handle m_device_handle;
     std::string m_udn;
-- 
2.17.1