[PATCH 1/2] rpcrt4: Don't nest critical sections to prevent a deadlock (Coverity).
Signed-off-by: Sven Baars <sven.wine(a)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; + } + 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; -- 2.17.1
Signed-off-by: Sven Baars <sven.wine(a)gmail.com> --- dlls/winmm/waveform.c | 27 ++++++++++++++++----------- 1 file changed, 16 insertions(+), 11 deletions(-) diff --git a/dlls/winmm/waveform.c b/dlls/winmm/waveform.c index 11a5cad601..b08768577b 100644 --- a/dlls/winmm/waveform.c +++ b/dlls/winmm/waveform.c @@ -1966,27 +1966,20 @@ static MMRESULT WINMM_BeginPlaying(WINMM_Device *device) return MMSYSERR_NOERROR; } -static LRESULT WINMM_Pause(HWAVE hwave) +static LRESULT WINMM_Pause(WINMM_Device *device) { - WINMM_Device *device = WINMM_GetDeviceFromHWAVE(hwave); HRESULT hr; - TRACE("(%p)\n", hwave); - - if(!WINMM_ValidateAndLock(device)) - return MMSYSERR_INVALHANDLE; + TRACE("(%p)\n", device->handle); hr = IAudioClient_Stop(device->client); if(FAILED(hr)){ - LeaveCriticalSection(&device->lock); WARN("Stop failed: %08x\n", hr); return MMSYSERR_ERROR; } device->stopped = FALSE; - LeaveCriticalSection(&device->lock); - return MMSYSERR_NOERROR; } @@ -2939,9 +2932,21 @@ UINT WINAPI waveOutBreakLoop(HWAVEOUT hWaveOut) */ UINT WINAPI waveOutPause(HWAVEOUT hWaveOut) { + WINMM_Device *device; + MMRESULT mr; + TRACE("(%p)\n", hWaveOut); - return WINMM_Pause((HWAVE)hWaveOut); + device = WINMM_GetDeviceFromHWAVE((HWAVE)hWaveOut); + + if(!WINMM_ValidateAndLock(device)) + return MMSYSERR_INVALHANDLE; + + mr = WINMM_Pause(device); + + LeaveCriticalSection(&device->lock); + + return mr; } /************************************************************************** @@ -3573,7 +3578,7 @@ UINT WINAPI waveInStop(HWAVEIN hWaveIn) if(!WINMM_ValidateAndLock(device)) return MMSYSERR_INVALHANDLE; - hr = WINMM_Pause((HWAVE)hWaveIn); + hr = WINMM_Pause(device); if(FAILED(hr)){ LeaveCriticalSection(&device->lock); return MMSYSERR_ERROR; -- 2.17.1
These two patches of course don't depend on each other. I just forgot to add the -N flag when creating the patches. Sorry for that. Sven
On Mon, Oct 21, 2019 at 11:49:49PM +0200, Sven Baars wrote:
Signed-off-by: Sven Baars <sven.wine(a)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;
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(a)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
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(a)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
participants (2)
-
Andrew Eikum -
Sven Baars