[PATCH 2/3] user32: Fix a race condition in update_monitor_cache().

[Zhiyi Zhang]

Access to video_key and last_query_monitors_time should have been
protected by monitors_section.

Signed-off-by: Zhiyi Zhang <zzhang(a)codeweavers.com>
---
 dlls/user32/sysparams.c | 30 +++++++++++++++++-------------
 1 file changed, 17 insertions(+), 13 deletions(-)

diff --git a/dlls/user32/sysparams.c b/dlls/user32/sysparams.c
index d9c7779edb..8568a5a73a 100644
--- a/dlls/user32/sysparams.c
+++ b/dlls/user32/sysparams.c
@@ -732,6 +732,7 @@ static HANDLE get_display_device_init_mutex( void )
 
 static void release_display_device_init_mutex( HANDLE mutex )
 {
+    if (!mutex) return;
     ReleaseMutex( mutex );
     CloseHandle( mutex );
 }
@@ -3755,15 +3756,18 @@ static BOOL update_monitor_cache(void)
     DWORD type;
 
     /* Update monitor cache from SetupAPI if it's outdated */
+    EnterCriticalSection( &monitors_section );
     if (!video_key && RegOpenKeyW( HKEY_LOCAL_MACHINE, VIDEO_KEY, &video_key ))
-        return FALSE;
+        goto done;
     if (RegQueryInfoKeyW( video_key, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, &filetime ))
-        return FALSE;
+        goto done;
     if (CompareFileTime( &filetime, &last_query_monitors_time ) < 1)
-        return TRUE;
+    {
+        ret = TRUE;
+        goto done;
+    }
 
     mutex = get_display_device_init_mutex();
-    EnterCriticalSection( &monitors_section );
     devinfo = SetupDiGetClassDevsW( &GUID_DEVCLASS_MONITOR, DISPLAY, NULL, DIGCF_PRESENT );
 
     while (SetupDiEnumDeviceInfo( devinfo, i++, &device_data ))
@@ -3771,7 +3775,7 @@ static BOOL update_monitor_cache(void)
         /* Inactive monitors don't get enumerated */
         if (!SetupDiGetDevicePropertyW( devinfo, &device_data, &WINE_DEVPROPKEY_MONITOR_STATEFLAGS, &type,
                                         (BYTE *)&state_flags, sizeof(DWORD), NULL, 0 ))
-            goto fail;
+            goto done;
         if (state_flags & DISPLAY_DEVICE_ACTIVE)
             device_count++;
     }
@@ -3780,7 +3784,7 @@ static BOOL update_monitor_cache(void)
     {
         monitor_array = heap_realloc( monitors, device_count * sizeof(*monitor_array) );
         if (!monitor_array)
-            goto fail;
+            goto done;
         monitors = monitor_array;
     }
 
@@ -3788,18 +3792,18 @@ static BOOL update_monitor_cache(void)
     {
         if (!SetupDiGetDevicePropertyW( devinfo, &device_data, &WINE_DEVPROPKEY_MONITOR_STATEFLAGS, &type,
                                         (BYTE *)&state_flags, sizeof(DWORD), NULL, 0 ))
-            goto fail;
+            goto done;
         if (!(state_flags & DISPLAY_DEVICE_ACTIVE))
             continue;
         if (!SetupDiGetDevicePropertyW( devinfo, &device_data, &WINE_DEVPROPKEY_MONITOR_RCMONITOR, &type,
                                         (BYTE *)&monitors[monitor_count].rcMonitor, sizeof(RECT), NULL, 0 ))
-            goto fail;
+            goto done;
 
         /* Mirrored slave monitors also don't get enumerated */
         mirrored_slave = FALSE;
         for (j = 0; j < monitor_count; j++)
         {
-            if (EqualRect(&monitors[j].rcMonitor, &monitors[monitor_count].rcMonitor))
+            if (EqualRect( &monitors[j].rcMonitor, &monitors[monitor_count].rcMonitor ))
             {
                 mirrored_slave = TRUE;
                 break;
@@ -3810,10 +3814,10 @@ static BOOL update_monitor_cache(void)
 
         if (!SetupDiGetDevicePropertyW( devinfo, &device_data, &WINE_DEVPROPKEY_MONITOR_RCWORK, &type,
                                         (BYTE *)&monitors[monitor_count].rcWork, sizeof(RECT), NULL, 0 ))
-            goto fail;
+            goto done;
         if (!SetupDiGetDevicePropertyW( devinfo, &device_data, &WINE_DEVPROPKEY_MONITOR_ADAPTERNAME, &type,
                                         (BYTE *)monitors[monitor_count].szDevice, CCHDEVICENAME * sizeof(WCHAR), NULL, 0))
-            goto fail;
+            goto done;
         monitors[monitor_count].dwFlags =
             !lstrcmpW( DEFAULT_ADAPTER_NAME, monitors[monitor_count].szDevice ) ? MONITORINFOF_PRIMARY : 0;
 
@@ -3822,10 +3826,10 @@ static BOOL update_monitor_cache(void)
 
     last_query_monitors_time = filetime;
     ret = TRUE;
-fail:
+done:
     SetupDiDestroyDeviceInfoList( devinfo );
-    LeaveCriticalSection( &monitors_section );
     release_display_device_init_mutex( mutex );
+    LeaveCriticalSection( &monitors_section );
     if (!ret)
         ERR("Failed to update monitor cache.\n");
     return ret;
-- 
2.23.0

[Marvin]

Hi,

While running your changed tests, I think I found new failures.
Being a bot and all I'm not very good at pattern recognition, so I might be
wrong, but could you please double-check?

Full results can be found at:
https://testbot.winehq.org/JobDetails.pl?Key=58191

Your paranoid android.


=== debian10 (32 bit report) ===

user32:
msg.c:8774: Test failed: WaitForSingleObject failed 102
msg.c:8780: Test failed: destroy child on thread exit: 0: the msg 0x0082 was expected, but got msg 0x000f instead
msg.c:8780: Test failed: destroy child on thread exit: 1: the msg 0x000f was expected, but got msg 0x0014 instead
msg.c:8780: Test failed: destroy child on thread exit: 2: the msg sequence is not complete: expected 0014 - actual 0000

=== debian10 (64 bit WoW report) ===

user32:
msg.c:8774: Test failed: WaitForSingleObject failed 102
msg.c:8780: Test failed: destroy child on thread exit: 0: the msg 0x0082 was expected, but got msg 0x000f instead
msg.c:8780: Test failed: destroy child on thread exit: 1: the msg 0x000f was expected, but got msg 0x0014 instead
msg.c:8780: Test failed: destroy child on thread exit: 2: the msg sequence is not complete: expected 0014 - actual 0000