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