[Bug 58082] New: race condition in GlobalMemoryStatusEx() implementation

[WineHQ Bugzilla]

http://bugs.winehq.org/show_bug.cgi?id=58082

Bug ID: 58082
           Summary: race condition in GlobalMemoryStatusEx()
                    implementation
           Product: Wine
           Version: unspecified
          Hardware: x86-64
                OS: Linux
            Status: UNCONFIRMED
          Severity: normal
          Priority: P2
         Component: kernel32
          Assignee: [e-mail]
          Reporter: [e-mail]
      Distribution: ---

I suspect that there is a race condition implementation of Win32 API function
GlobalMemoryStatusEx().

This function has a local static variable cached_status of type MEMORYSTATUSEX.
The content of this structure is returned by GlobalMemoryStatusEx() if tick
count since last call is less then 1000.

https://gitlab.winehq.org/wine/wine/-/blob/6b04bdf25796c9c76815588bf7bdc36ff...

However when there are multiple threads calling GlobalMemoryStatusEx() at
nearly the same time, then it can happen that:

1) Thread 1 checks (NtGetTickCount() - last_check) < 1000 and finds that it is
false (e.g. because long time elapsed since system start and last_check is 0
because it is static). It sets last_check = NtGetTickCount() and proceeds with
retrieving the information.

2) Thread 2 checks (NtGetTickCount() - last_check) < 1000 and finds that it is
true because last_check was set by Thread 1 just a few ticks ago. And returns
content of cached_status which is not yet filled.

3) Thread 1 proceeds with filling the structure retrieved via parameter and
after filling it in, it saves it into cached_status.

The Thread 2 received content of cached_status that not yet populated with real
information.

As a solution I suggest to move 
last_check = NtGetTickCount();
after 
cached_status = *status;
assignment which means that  (NtGetTickCount() - last_check) < 1000 condition
will be satisfied only after cached_status is actually filled in.

[Bug 58082] race condition in GlobalMemoryStatusEx() implementation

[WineHQ Bugzilla]

http://bugs.winehq.org/show_bug.cgi?id=58082

--- Comment #1 from Rastislav Stanik  ---
Created attachment 78405
  --> http://bugs.winehq.org/attachment.cgi?id=78405
program that exhibits the problem

The .zip file contains source code file, Visual Studio 2017 project file and
resulting .exe. When the .exe is started without any arguments, it creates a
thread and both the thread and main() call GlobalMemoryStatusEx() at the same
time. On my system this results in following output:

C:\tmp>winebug58082.exe
main found total memory 0
thread found total memory 270026698752

C:\tmp>winebug58082.exe
thread found total memory 0
main found total memory 270026698752

There one of the calls returns incorrectly 0.

When started with an argument the argument is passed to Sleep() and causes the
thread to wait a bit before calling GlobalMemoryStatusEx(). This on my system
results in correct output:

C:\tmp>winebug58082.exe 1
main found total memory 270026698752
thread found total memory 270026698752

[Bug 58082] race condition in GlobalMemoryStatusEx() implementation

[WineHQ Bugzilla]

http://bugs.winehq.org/show_bug.cgi?id=58082

Austin English  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |download, source, testcase

[Bug 58082] race condition in GlobalMemoryStatusEx() implementation

[WineHQ Bugzilla]

http://bugs.winehq.org/show_bug.cgi?id=58082

Austin English  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
            Version|unspecified                 |10.5

[Bug 58082] race condition in GlobalMemoryStatusEx() implementation

[WineHQ Bugzilla]

http://bugs.winehq.org/show_bug.cgi?id=58082

--- Comment #2 from Nikolay Sivov  ---
This was merged as
https://gitlab.winehq.org/wine/wine/-/commit/db190a0d80ee559afcd483e19543bb9....
Should we mark it fixed?

[Bug 58082] race condition in GlobalMemoryStatusEx() implementation

[WineHQ Bugzilla]

http://bugs.winehq.org/show_bug.cgi?id=58082

Rastislav Stanik  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |RESOLVED
         Resolution|---                         |FIXED

--- Comment #3 from Rastislav Stanik  ---
Yes.

[Bug 58082] Race condition in GlobalMemoryStatusEx() implementation

[WineHQ Bugzilla]

http://bugs.winehq.org/show_bug.cgi?id=58082

Nikolay Sivov  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
            Summary|race condition in           |Race condition in
                   |GlobalMemoryStatusEx()      |GlobalMemoryStatusEx()
                   |implementation              |implementation
      Fixed by SHA1|                            |db190a0d80ee559afcd483e1954
                   |                            |3bb9e5db6975a

[Bug 58082] Race condition in GlobalMemoryStatusEx() implementation

[WineHQ Bugzilla]

http://bugs.winehq.org/show_bug.cgi?id=58082

Alexandre Julliard  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|RESOLVED                    |CLOSED

--- Comment #4 from Alexandre Julliard  ---
Closing bugs fixed in 10.7.