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

[Andrew Eikum]

On Wed, Oct 23, 2019 at 10:09:43PM +0200, Sven Baars wrote:
> 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.
> 

Before, the listen_cs lock was held during both of the break
conditions in the while(1) loop, and only released after destroying
the event. So, the destruction is synchronized with the loop break
condition checks. After your patch, they're no longer synchronized,
because you release the lock before breaking out of the loop and then
reacquire it before destroying the event. So I think your patch
introduces a potential new issue.

> 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.
> 

Yeah that looks like yet another separate problem (not re-checking the
initial condition after reacquiring the lock). And, there's no reason
to hold the lock just to call SetEvent. That whole file's thread
synchronization looks like a mess.

Note that listen_count is also manipulated in RPCRT4_stop_listen while
holding the lock, so it's important that its state be synchronized
with listen_done_event.

Andrew