deadlock in mmdevapi AudioClient_Stop (winealsa.drv/mmdevdrv.c)

[Maarten Lankhorst]

Hi Joerg,

Op 15-06-11 10:00, Joerg-Cyril.Hoehle at t-systems.com schreef:
> Hi,
>
> please try and convince me that a deadlock cannot happen.
>
> http://source.winehq.org/source//dlls/winealsa.drv/mmdevdrv.c
>
> AudioClient_Start calls
>  CreateTimerQueueTimer
>    which creates a new thread periodically invoking the timer callback.
>
> The callback invokes
> alsa_push_buffer_data()
>  which ceases the audio client object lock (This->lock).
>
>  AudioClient_Stop performs
>   EnterCriticalSection(&This->lock); then
>   DeleteTimerQueueTimer(g_timer_q, This->timer, INVALID_HANDLE_VALUE);
>
> Now imagine Stop is invoked, EnterCriticalSection executed then the thread is preempted.
> Suppose the timer callback kicks in: alsa_push_buffer_data will block on This->lock.
> When the thread is resumed, DeleteTimerQueue will hang forever waiting for all timer callbacks to come to an end.
>
> My opinion is that critical sections are great to prevent concurrent execution while
> things are running, but they are hell to tear down properly.
>
> IMHO, LeaveCriticalSection + Re-EnterCriticalSection around DeleteTimerQueue is not a solution.
> I consider using Leave+EnterCS around anything that can wait/block an anti-pattern
> (there are a few places like that in Wine). I believe a "restart op" is needed in general
> (like a "restart transaction if it failed" in databases) as anything could have happened while waiting.
You're right, This->timer should be set to NULL inside the lock,
and DeleteTimerQueueTimer should be run afterwards.

~Maarten