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

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.

Regards,
 Jörg Höhle

[Maarten Lankhorst]

Hi Joerg,

Op 15-06-11 10:00, [e-mail] 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