[PATCH 1/2] rpcrt4: Don't nest critical sections to prevent a deadlock (Coverity).

[Andrew Eikum]

On Mon, Oct 21, 2019 at 11:49:49PM +0200, Sven Baars wrote:
> Signed-off-by: Sven Baars <sven.wine at gmail.com>
> ---
>  dlls/rpcrt4/rpc_server.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/dlls/rpcrt4/rpc_server.c b/dlls/rpcrt4/rpc_server.c
> index 12260b7298..b5110c879c 100644
> --- a/dlls/rpcrt4/rpc_server.c
> +++ b/dlls/rpcrt4/rpc_server.c
> @@ -1558,12 +1558,16 @@ RPC_STATUS WINAPI RpcMgmtWaitServerListen( void )
>    WaitForSingleObject( event, INFINITE );
>    TRACE( "done waiting\n" );
>  
> -  EnterCriticalSection(&listen_cs);
>    /* wait for server threads to finish */
>    while(1)
>    {
> +      EnterCriticalSection(&listen_cs);
>        if (listen_count)
> +      {
> +          LeaveCriticalSection(&listen_cs);
>            break;

I agree there's a potential deadlock here (RpcMgmtWaitServerListen
locks listen_cs and then server_cs, while RPCRT4_start_listen locks
server_cs and then listen_cs), but this change unsynchronizes the
listen_count check from the listen_done_event assignment below. That
doesn't look right to me.

Andrew

> +      }
> +      LeaveCriticalSection(&listen_cs);
>  
>        wait_thread = NULL;
>        EnterCriticalSection(&server_cs);
> @@ -1577,10 +1581,9 @@ RPC_STATUS WINAPI RpcMgmtWaitServerListen( void )
>            break;
>  
>        TRACE("waiting for thread %u\n", GetThreadId(wait_thread));
> -      LeaveCriticalSection(&listen_cs);
>        WaitForSingleObject(wait_thread, INFINITE);
> -      EnterCriticalSection(&listen_cs);
>    }
> +  EnterCriticalSection(&listen_cs);
>    if (listen_done_event == event)
>    {
>        listen_done_event = NULL;