[PATCH v3 0/1] MR7870: kernelbase: fix racing condition in GlobalMemoryStatusEx()

[Rastislav Stanik (＠rastos)]

use a critical section to guard access to local static variable with cached information

Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=58082

--
  v3: kernelbase: fix racing condition in GlobalMemoryStatusEx()
 
https://gitlab.winehq.org/wine/wine/-/merge_requests/7870

[PATCH v3 1/1] kernelbase: fix racing condition in GlobalMemoryStatusEx()

[Rastislav Stanik]

From: Rastislav Stanik <git(a)rastos.org>

use a critical section to guard access to local static variable with cached information

Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=58082
---
 dlls/kernelbase/memory.c | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/dlls/kernelbase/memory.c b/dlls/kernelbase/memory.c
index 0be178f6ab7..39018012199 100644
--- a/dlls/kernelbase/memory.c
+++ b/dlls/kernelbase/memory.c
@@ -42,6 +42,14 @@ WINE_DECLARE_DEBUG_CHANNEL(virtual);
 WINE_DECLARE_DEBUG_CHANNEL(globalmem);
 
 
+static CRITICAL_SECTION memstatus_section;
+static CRITICAL_SECTION_DEBUG critsect_debug =
+{
+    0, 0, &memstatus_section,
+    { &critsect_debug.ProcessLocksList, &critsect_debug.ProcessLocksList },
+      0, 0, { (DWORD_PTR)(__FILE__ ": memstatus_section") }
+};
+static CRITICAL_SECTION memstatus_section = { &critsect_debug, -1, 0, 0, 0, 0 };
 
 static CROSS_PROCESS_WORK_LIST *open_cross_process_connection( HANDLE process )
 {
@@ -1333,7 +1341,7 @@ BOOL WINAPI DECLSPEC_HOTPATCH GetPhysicallyInstalledSystemMemory( ULONGLONG *mem
 BOOL WINAPI DECLSPEC_HOTPATCH GlobalMemoryStatusEx( MEMORYSTATUSEX *status )
 {
     static MEMORYSTATUSEX cached_status;
-    static DWORD last_check;
+    static ULONGLONG last_check;
     SYSTEM_BASIC_INFORMATION basic_info;
     SYSTEM_PERFORMANCE_INFORMATION perf_info;
     VM_COUNTERS_EX vmc;
@@ -1343,12 +1351,13 @@ BOOL WINAPI DECLSPEC_HOTPATCH GlobalMemoryStatusEx( MEMORYSTATUSEX *status )
         SetLastError( ERROR_INVALID_PARAMETER );
         return FALSE;
     }
-    if ((NtGetTickCount() - last_check) < 1000)
+    RtlEnterCriticalSection(&memstatus_section);
+    if ((GetTickCount64() - last_check) < 1000 && cached_status.dwLength > 0)
     {
 	*status = cached_status;
+	RtlLeaveCriticalSection(&memstatus_section);
 	return TRUE;
     }
-    last_check = NtGetTickCount();
 
     if (!set_ntstatus( NtQuerySystemInformation( SystemBasicInformation,
                                                  &basic_info, sizeof(basic_info), NULL )) ||
@@ -1356,7 +1365,10 @@ BOOL WINAPI DECLSPEC_HOTPATCH GlobalMemoryStatusEx( MEMORYSTATUSEX *status )
                                                  &perf_info, sizeof(perf_info), NULL)) ||
         !set_ntstatus( NtQueryInformationProcess( GetCurrentProcess(), ProcessVmCounters,
                                                   &vmc, sizeof(vmc), NULL )))
+    {
+        RtlLeaveCriticalSection(&memstatus_section);
         return FALSE;
+    }
 
     status->dwMemoryLoad     = 0;
     status->ullTotalPhys     = basic_info.MmNumberOfPhysicalPages;
@@ -1381,6 +1393,8 @@ BOOL WINAPI DECLSPEC_HOTPATCH GlobalMemoryStatusEx( MEMORYSTATUSEX *status )
                      status->ullAvailPageFile, status->ullTotalVirtual, status->ullAvailVirtual );
 
     cached_status = *status;
+    last_check = GetTickCount64();
+    RtlLeaveCriticalSection(&memstatus_section);
     return TRUE;
 }
 
-- 
GitLab

https://gitlab.winehq.org/wine/wine/-/merge_requests/7870

[Jinoh Kang (＠iamahuman)]

Don't post a new MR, continue your work at !7789. Thanks in advance!

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/7870#note_101571

[Rastislav Stanik (＠rastos)]

On Mon Apr 21 19:11:50 2025 +0000, Jinoh Kang wrote:
> Don't post a new MR, continue your work at !7789. Thanks in advance!
> **EDIT**: I notice you requested MR from the master branch and want to
> fix it, in that case, shall we close !7789?
> Yes, merge request !7789 can be closed.

Based on the discussion in IRC channel #winehackers the approach suggested in !7789 (moving the assignment, that decides whether to use cache, to different place) was rejected. It could reduce the possibility of race condition but it was deemed not good enough. We are now trying a different approach by using a critical section.

I apologize for multiple MR, I'm not well experienced with using gitlab (but I'm trying to improve).

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/7870#note_101573