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

[Sven Baars]

On 23-10-19 18:47, Andrew Eikum wrote:
> 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

Hi Andrew,

I thought that this was the reason that the "listen_done_event == event"
statement is there in the first place. Note that before, during the
while loop, listen_cs was already unlocked anyway, so in that case this
issue also existed there.

But I indeed was also a bit confused because I suppose something like
this could happen:

RPCRT4_stop_listen locks listen_cs
RPCRT4_stop_listen checks if listen_done_event is NULL, it is not
RPCRT4_stop_listen unlocks listen_cs
RpcMgmtWaitServerListen locks listen_cs
RpcMgmtWaitServerListen sets listen_done_event to NULL
RpcMgmtWaitServerListen unlocks listen_cs
RPCRT4_stop_listen locks listen_cs
RPCRT4_stop_listen calls SetEvent with listen_done_event now being NULL
RPCRT4_stop_listen unlocks listen_cs

and other similar things. But I'm not sure if these are actual issues
that should be fixed. For the same reason I also wasn't really sure if I
had to add that lock around listen_count.

Sven