[vlc-devel] [PATCH] upnp: Fix potential race during tear down.

[Hugo Beauzée-Luyssen]

On Mon, Aug 19, 2019, at 5:46 PM, Johan Gunnarsson wrote:
> This patch can lead to deadlock if release() is called when there are 
> pending callbacks. UpnpFinish() will wait for all pending callbacks to 
> finish, but they can't finish since release() holds s_lock.
> 

Hi,

As far as I understand, no callback will come after UpnpUnRegisterClient returns, so I don't think there's a problem with UpnpFinish here, however I agree that there's an issue if a callback gets invoked before UpnpUnRegisterClient returns, and I'm  not sure there's a proper fix that doesn't involve to patch libupnp.

> On Wed, 7 Aug 2019, 16:46 Hugo Beauzée-Luyssen, <hugo at beauzee.fr> wrote:
> > If two threads call UpnpFinish at the same time (or more precisely, if a
> >  2nd thread calls UpnpFinish before the first one sets UpnpSdkInit to 0)
> >  we can end up double releasing most libupnp resources
> >  ---
> >  modules/services_discovery/upnp-wrapper.cpp | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> > 
> >  diff --git a/modules/services_discovery/upnp-wrapper.cpp b/modules/services_discovery/upnp-wrapper.cpp
> >  index c001492f37..3b8037b3f1 100644
> >  --- a/modules/services_discovery/upnp-wrapper.cpp
> >  +++ b/modules/services_discovery/upnp-wrapper.cpp
> >  @@ -113,13 +113,12 @@ UpnpInstanceWrapper *UpnpInstanceWrapper::get(vlc_object_t *p_obj)
> >  void UpnpInstanceWrapper::release()
> >  {
> >  UpnpInstanceWrapper *p_delete = NULL;
> >  - vlc_mutex_lock( &s_lock );
> >  + vlc::threads::mutex_locker lock( &s_lock );
> >  if (--s_instance->m_refcount == 0)
> >  {
> >  p_delete = s_instance;
> >  s_instance = NULL;
> >  }
> >  - vlc_mutex_unlock( &s_lock );
> >  delete p_delete;
> >  }
> > 

-- 
  Hugo Beauzée-Luyssen
  hugo at beauzee.fr