Re: Maarten Lankhorst : winmm: Fix midi deadlock by not holding lock on release .

[Ken Thomases]

This has a whiff of the double-checked locking anti-pattern.  I can't  
see a way where it would really bite us in this case, but that anti- 
pattern has several subtleties.  So, I just wanted to prompt people to  
double-check it (pun acknowledged).

Cheers,
Ken

On Mar 18, 2008, at 7:44 AM, Alexandre Julliard wrote:
> Module: wine
> Branch: master
> Commit: 15907b5035ea7f03b369a0463ab1f6ac2b24704e
> URL:    http://source.winehq.org/git/wine.git/?a=commit;h=15907b5035ea7f03b369a0463a...
>
> Author: Maarten Lankhorst <m.b.lankhorst(a)gmail.com>
> Date:   Mon Mar 17 13:15:42 2008 -0700
>
> winmm: Fix midi deadlock by not holding lock on release.
>
> ---
>
> dlls/winmm/mci.c |   22 ++++++++++++++--------
> 1 files changed, 14 insertions(+), 8 deletions(-)
>
> diff --git a/dlls/winmm/mci.c b/dlls/winmm/mci.c
> index b7b5ac2..c792d18 100644
> --- a/dlls/winmm/mci.c
> +++ b/dlls/winmm/mci.c
> @@ -1767,19 +1767,25 @@ static	DWORD MCI_Close(UINT16 wDevID, DWORD  
> dwParam, LPMCI_GENERIC_PARMS lpParms
>     TRACE("(%04x, %08X, %p)\n", wDevID, dwParam, lpParms);
>
> if (wDevID == MCI_ALL_DEVICE_ID) {
> -	LPWINE_MCIDRIVER	next;
> -
> -	EnterCriticalSection(&WINMM_cs);
>   /* FIXME: shall I notify once after all is done, or for
>    * each of the open drivers ? if the latest, which notif
>    * to return when only one fails ?
>    */
> -	for (wmd = MciDrivers; wmd; ) {
> -	    next = wmd->lpNext;
> -	    MCI_Close(wmd->wDeviceID, dwParam, lpParms);
> -	    wmd = next;
> +	while (MciDrivers) {
> +            /* Retrieve the device ID under lock, but send the  
> message without,
> +             * the driver might be calling some winmm functions  
> from another
> +             * thread before being fully stopped.
> +             */
> +            EnterCriticalSection(&WINMM_cs);
> +            if (!MciDrivers)
> +            {
> +                LeaveCriticalSection(&WINMM_cs);
> +                break;
> +            }
> +            wDevID = MciDrivers->wDeviceID;
> +            LeaveCriticalSection(&WINMM_cs);
> +            MCI_Close(wDevID, dwParam, lpParms);
>   }
> -	LeaveCriticalSection(&WINMM_cs);
>   return 0;
>     }