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

[Hugo Beauzée-Luyssen]

On Tue, Aug 6, 2019, at 3:24 PM, Hugo Beauzée-Luyssen 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 | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/modules/services_discovery/upnp-wrapper.cpp 
> b/modules/services_discovery/upnp-wrapper.cpp
> index c001492f37..76ceb89a1b 100644
> --- a/modules/services_discovery/upnp-wrapper.cpp
> +++ b/modules/services_discovery/upnp-wrapper.cpp
> @@ -119,8 +119,8 @@ void UpnpInstanceWrapper::release()
>          p_delete = s_instance;
>          s_instance = NULL;
>      }
> -    vlc_mutex_unlock( &s_lock );
>      delete p_delete;
> +    vlc_mutex_unlock( &s_lock );
>  }
>  
>  UpnpClient_Handle UpnpInstanceWrapper::handle() const

A bit more context that might be useful: the libupnp resources I refer to in the commit log are global variables, not protected by any lock. When calling UpnpFinish, they will be released if UpnpSdkInit is 1, which is the case until UpnpFinish returns.
If another thread is to call UpnpInit before UpnpFinish returns (which is possible currently since the refcount is set to 0, s_instance to NULL, and the lock released), UpnpInit will return UPNP_E_INIT causing us to release the newly created instance. Now we have 2 threads calling UpnpFinish with a UpnpSdkInit set to 1, causing the 2nd thread to double release all the global libupnp resources.

I hope that makes more sense!

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