[PATCH 0/4] Update USER_SHARED_DATA timestamps more often.
This is a rewrite of the ntdll-User_Shared_Data staging patch series, with a simpler approach that still solves the issue. Although it is less correct in term of timestamp update than the original series, it should also have less impact and may be more acceptable. We already update the timestamps from time to time in ntoskrnl before accessing the memory, and this adds another update after each sleep. I checked that it allows the games reported in [2] to start, and some others that also required the original patch series: * Overwatch (with some additional staging patches) * World of Warcraft (with some additional staging patches) * Star Wars: The Old Republic * Batman: Arkham Knight * Just Cause 3 [1] https://www.winehq.org/pipermail/wine-devel/2012-March/094788.html [2] https://bugs.winehq.org/show_bug.cgi?id=29168 Andrew Wesie (1): ntdll/tests: Test updating TickCount in user_shared_data. Rémi Bernon (3): ntdll: Add user_shared_data_update internal timestamp update function. ntdll: Update user_shared_data InterruptTime fields when updating time. ntdll: Update user_shared_data timestamps after NtDelayExecution. dlls/ntdll/ntdll_misc.h | 1 + dlls/ntdll/sync.c | 16 ++++++++++------ dlls/ntdll/tests/time.c | 26 ++++++++++++++++++++++++++ dlls/ntdll/thread.c | 37 +++++++++++++++++++++++++++++-------- 4 files changed, 66 insertions(+), 14 deletions(-) -- 2.24.0
From: Andrew Wesie <awesie(a)gmail.com> Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/ntdll/tests/time.c | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/dlls/ntdll/tests/time.c b/dlls/ntdll/tests/time.c index 5382a952d8d..eb7b5bf8890 100644 --- a/dlls/ntdll/tests/time.c +++ b/dlls/ntdll/tests/time.c @@ -18,7 +18,9 @@ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA */ +#define NONAMELESSUNION #include "ntdll_test.h" +#include "ddk/wdm.h" #define TICKSPERSEC 10000000 #define TICKSPERMSEC 10000 @@ -29,6 +31,7 @@ static VOID (WINAPI *pRtlTimeFieldsToTime)( PTIME_FIELDS TimeFields, PLARGE_IN static NTSTATUS (WINAPI *pNtQueryPerformanceCounter)( LARGE_INTEGER *counter, LARGE_INTEGER *frequency ); static NTSTATUS (WINAPI *pRtlQueryTimeZoneInformation)( RTL_TIME_ZONE_INFORMATION *); static NTSTATUS (WINAPI *pRtlQueryDynamicTimeZoneInformation)( RTL_DYNAMIC_TIME_ZONE_INFORMATION *); +static ULONG (WINAPI *pNtGetTickCount)(void); static const int MonthLengths[2][12] = { @@ -153,12 +156,35 @@ static void test_RtlQueryTimeZoneInformation(void) wine_dbgstr_w(tzinfo.DaylightName)); } +static void test_NtGetTickCount(void) +{ + KSHARED_USER_DATA *user_shared_data = (void *)0x7ffe0000; + LONG diff; + int i; + + if (!pNtGetTickCount) + { + win_skip("NtGetTickCount is not available\n"); + return; + } + + for (i = 0; i < 5; ++i) + { + diff = (user_shared_data->u.TickCountQuad * user_shared_data->TickCountMultiplier) >> 24; + diff = pNtGetTickCount() - diff; + todo_wine + ok(diff < 32, "NtGetTickCount - TickCountQuad too high, expected < 32 got %d\n", diff); + Sleep(50); + } +} + START_TEST(time) { HMODULE mod = GetModuleHandleA("ntdll.dll"); pRtlTimeToTimeFields = (void *)GetProcAddress(mod,"RtlTimeToTimeFields"); pRtlTimeFieldsToTime = (void *)GetProcAddress(mod,"RtlTimeFieldsToTime"); pNtQueryPerformanceCounter = (void *)GetProcAddress(mod, "NtQueryPerformanceCounter"); + pNtGetTickCount = (void *)GetProcAddress(mod,"NtGetTickCount"); pRtlQueryTimeZoneInformation = (void *)GetProcAddress(mod, "RtlQueryTimeZoneInformation"); pRtlQueryDynamicTimeZoneInformation = @@ -169,5 +195,6 @@ START_TEST(time) else win_skip("Required time conversion functions are not available\n"); test_NtQueryPerformanceCounter(); + test_NtGetTickCount(); test_RtlQueryTimeZoneInformation(); } -- 2.24.0
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=61090 Your paranoid android. === w2008s64 (32 bit report) === ntdll: time.c:176: Test failed: NtGetTickCount - TickCountQuad too high, expected < 32 got 63
Based on a staging patch from Sebastian Lackner <sebastian(a)fds-team.de> Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/ntdll/ntdll_misc.h | 1 + dlls/ntdll/thread.c | 19 +++++++++++++++---- 2 files changed, 16 insertions(+), 4 deletions(-) diff --git a/dlls/ntdll/ntdll_misc.h b/dlls/ntdll/ntdll_misc.h index ac146941236..8767d9a3ecd 100644 --- a/dlls/ntdll/ntdll_misc.h +++ b/dlls/ntdll/ntdll_misc.h @@ -195,6 +195,7 @@ extern void virtual_set_large_address_space(void) DECLSPEC_HIDDEN; extern void virtual_fill_image_information( const pe_image_info_t *pe_info, SECTION_IMAGE_INFORMATION *info ) DECLSPEC_HIDDEN; extern struct _KUSER_SHARED_DATA *user_shared_data DECLSPEC_HIDDEN; +extern void user_shared_data_update(void); /* completion */ extern NTSTATUS NTDLL_AddCompletion( HANDLE hFile, ULONG_PTR CompletionValue, diff --git a/dlls/ntdll/thread.c b/dlls/ntdll/thread.c index 621aaddfe34..6f700e3e9d7 100644 --- a/dlls/ntdll/thread.c +++ b/dlls/ntdll/thread.c @@ -225,7 +225,6 @@ TEB *thread_init(void) TEB *teb; void *addr; SIZE_T size; - LARGE_INTEGER now; NTSTATUS status; struct ntdll_thread_data *thread_data; @@ -301,6 +300,21 @@ TEB *thread_init(void) set_process_name( __wine_main_argc, __wine_main_argv ); /* initialize time values in user_shared_data */ + user_shared_data_update(); + + fill_cpu_info(); + + return teb; +} + + +/*********************************************************************** + * user_shared_data_update + */ +void user_shared_data_update() +{ + LARGE_INTEGER now; + NtQuerySystemTime( &now ); user_shared_data->SystemTime.LowPart = now.u.LowPart; user_shared_data->SystemTime.High1Time = user_shared_data->SystemTime.High2Time = now.u.HighPart; @@ -308,9 +322,6 @@ TEB *thread_init(void) user_shared_data->u.TickCount.High2Time = user_shared_data->u.TickCount.High1Time; user_shared_data->TickCountLowDeprecated = user_shared_data->u.TickCount.LowPart; user_shared_data->TickCountMultiplier = 1 << 24; - fill_cpu_info(); - - return teb; } -- 2.24.0
Based on a staging patch from Michael Müller <michael(a)fds-team.de> Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/ntdll/thread.c | 22 ++++++++++++++++------ 1 file changed, 16 insertions(+), 6 deletions(-) diff --git a/dlls/ntdll/thread.c b/dlls/ntdll/thread.c index 6f700e3e9d7..995088be36b 100644 --- a/dlls/ntdll/thread.c +++ b/dlls/ntdll/thread.c @@ -313,15 +313,25 @@ TEB *thread_init(void) */ void user_shared_data_update() { + ULARGE_INTEGER interrupt; LARGE_INTEGER now; NtQuerySystemTime( &now ); - user_shared_data->SystemTime.LowPart = now.u.LowPart; - user_shared_data->SystemTime.High1Time = user_shared_data->SystemTime.High2Time = now.u.HighPart; - user_shared_data->u.TickCountQuad = (now.QuadPart - server_start_time) / 10000; - user_shared_data->u.TickCount.High2Time = user_shared_data->u.TickCount.High1Time; - user_shared_data->TickCountLowDeprecated = user_shared_data->u.TickCount.LowPart; - user_shared_data->TickCountMultiplier = 1 << 24; + user_shared_data->SystemTime.High2Time = now.u.HighPart; + user_shared_data->SystemTime.LowPart = now.u.LowPart; + user_shared_data->SystemTime.High1Time = now.u.HighPart; + + RtlQueryUnbiasedInterruptTime( &interrupt.QuadPart ); + user_shared_data->InterruptTime.High2Time = interrupt.HighPart; + user_shared_data->InterruptTime.LowPart = interrupt.LowPart; + user_shared_data->InterruptTime.High1Time = interrupt.HighPart; + + interrupt.QuadPart /= 10000; + user_shared_data->u.TickCount.High2Time = interrupt.HighPart; + user_shared_data->u.TickCount.LowPart = interrupt.LowPart; + user_shared_data->u.TickCount.High1Time = interrupt.HighPart; + user_shared_data->TickCountLowDeprecated = interrupt.LowPart; + user_shared_data->TickCountMultiplier = 1 << 24; } -- 2.24.0
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=61092 Your paranoid android. === debian10 (32 bit report) === ntdll: time.c:176: Test succeeded inside todo block: NtGetTickCount - TickCountQuad too high, expected < 32 got 31 === debian10 (32 bit Chinese:China report) === ntdll: time.c:176: Test succeeded inside todo block: NtGetTickCount - TickCountQuad too high, expected < 32 got 29 === debian10 (32 bit WoW report) === ntdll: time.c:176: Test succeeded inside todo block: NtGetTickCount - TickCountQuad too high, expected < 32 got 30 === debian10 (64 bit WoW report) === ntdll: time.c:176: Test succeeded inside todo block: NtGetTickCount - TickCountQuad too high, expected < 32 got 23
Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=29168 Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/ntdll/sync.c | 16 ++++++++++------ dlls/ntdll/tests/time.c | 1 - 2 files changed, 10 insertions(+), 7 deletions(-) diff --git a/dlls/ntdll/sync.c b/dlls/ntdll/sync.c index 35b89df52ed..f241b9c2669 100644 --- a/dlls/ntdll/sync.c +++ b/dlls/ntdll/sync.c @@ -1146,11 +1146,14 @@ NTSTATUS WINAPI NtYieldExecution(void) */ NTSTATUS WINAPI NtDelayExecution( BOOLEAN alertable, const LARGE_INTEGER *timeout ) { + NTSTATUS status = STATUS_SUCCESS; + /* if alertable, we need to query the server */ if (alertable) - return server_select( NULL, 0, SELECT_INTERRUPTIBLE | SELECT_ALERTABLE, timeout ); - - if (!timeout || timeout->QuadPart == TIMEOUT_INFINITE) /* sleep forever */ + { + status = server_select( NULL, 0, SELECT_INTERRUPTIBLE | SELECT_ALERTABLE, timeout ); + } + else if (!timeout || timeout->QuadPart == TIMEOUT_INFINITE) /* sleep forever */ { for (;;) select( 0, NULL, NULL, NULL, NULL ); } @@ -1167,11 +1170,10 @@ NTSTATUS WINAPI NtDelayExecution( BOOLEAN alertable, const LARGE_INTEGER *timeou /* Note that we yield after establishing the desired timeout */ NtYieldExecution(); - if (!when) return STATUS_SUCCESS; - for (;;) { struct timeval tv; + if (!when) break; NtQuerySystemTime( &now ); diff = (when - now.QuadPart + 9) / 10; if (diff <= 0) break; @@ -1180,7 +1182,9 @@ NTSTATUS WINAPI NtDelayExecution( BOOLEAN alertable, const LARGE_INTEGER *timeou if (select( 0, NULL, NULL, NULL, &tv ) != -1) break; } } - return STATUS_SUCCESS; + + user_shared_data_update(); + return status; } diff --git a/dlls/ntdll/tests/time.c b/dlls/ntdll/tests/time.c index eb7b5bf8890..251e3936fb6 100644 --- a/dlls/ntdll/tests/time.c +++ b/dlls/ntdll/tests/time.c @@ -172,7 +172,6 @@ static void test_NtGetTickCount(void) { diff = (user_shared_data->u.TickCountQuad * user_shared_data->TickCountMultiplier) >> 24; diff = pNtGetTickCount() - diff; - todo_wine ok(diff < 32, "NtGetTickCount - TickCountQuad too high, expected < 32 got %d\n", diff); Sleep(50); } -- 2.24.0
And of course, this would probably need to be done in a thread-safe way to be correct. But I'll let it be for a while, for further discussion on the general idea. -- Rémi Bernon <rbernon(a)codeweavers.com>
On 11/29/19 12:23, Rémi Bernon wrote:
This is a rewrite of the ntdll-User_Shared_Data staging patch series, with a simpler approach that still solves the issue. Although it is less correct in term of timestamp update than the original series, it should also have less impact and may be more acceptable. We already update the timestamps from time to time in ntoskrnl before accessing the memory, and this adds another update after each sleep.
While this might allow the games to start, won't such approach break (some of) game mechanics or sync with servers? The games might be using this instead of GetTickCount(), having the time updated at some rare and irregular intervals looks scary. I thought that somewhat potentially better approach than that currently in Staging would be to map user shared area to wineserver and always create a single thread in server to update the shared area. That would be somewhat similar to what Windows does if wineserver stands for kernel. I also heard that sharing memory with server is considered a no go, but I could not find any discussion why is it so.
On 11/29/19 11:03 AM, Paul Gofman wrote:
On 11/29/19 12:23, Rémi Bernon wrote:
This is a rewrite of the ntdll-User_Shared_Data staging patch series, with a simpler approach that still solves the issue. Although it is less correct in term of timestamp update than the original series, it should also have less impact and may be more acceptable. We already update the timestamps from time to time in ntoskrnl before accessing the memory, and this adds another update after each sleep.
While this might allow the games to start, won't such approach break (some of) game mechanics or sync with servers? The games might be using this instead of GetTickCount(), having the time updated at some rare and irregular intervals looks scary.
AFAICS it's working fine although I didn't test it really deep to be honest.
I thought that somewhat potentially better approach than that currently in Staging would be to map user shared area to wineserver and always create a single thread in server to update the shared area. That would be somewhat similar to what Windows does if wineserver stands for kernel. I also heard that sharing memory with server is considered a no go, but I could not find any discussion why is it so.
Yes, that was one of the approaches discussed in the original thread [1] and probably a more accurate way to do it. It has however some drawbacks, in addition to being much more complicated and probably having some unforeseen implications: * Creating a thread in wineserver is probably not going to be OK, as it is single threaded and should stay as such whatever the purpose is. * Regarding memory sharing, I understood from a WineConf discussion that sharing read-only memory from the server to applications would be OK generally speaking. But in this case, KUSER_SHARED_MEMORY contains some information about the emulated OS from an application perspective, and that can vary from on application to another. This would require sharing different memory regions depending on the application and updating all of them, complicating the whole thing. There are also some other fields currently written by applications and that would need to be updated by the server to keep the shared memory read-only. * Then according to [1], updating the timestamps continuously could apparently break some copy-protection schemes (SecuROM for instance) that measures the time taken by some syscalls. As Wine is not exactly doing as Windows, the syscall timings could be larger and having the timestamps not increase in this particular case allows us to pass these checks. [1] https://www.winehq.org/pipermail/wine-devel/2012-March/094788.html -- Rémi Bernon <rbernon(a)codeweavers.com>
On 11/29/19 11:40 AM, Rémi Bernon wrote:
* Then according to [1], updating the timestamps continuously could apparently break some copy-protection schemes (SecuROM for instance) that measures the time taken by some syscalls. As Wine is not exactly doing as Windows, the syscall timings could be larger and having the timestamps not increase in this particular case allows us to pass these checks.
[1] https://www.winehq.org/pipermail/wine-devel/2012-March/094788.html
My bad, it was SafeDisk, not SecuROM, as mentioned here: https://www.winehq.org/pipermail/wine-devel/2012-March/094810.html -- Rémi Bernon <rbernon(a)codeweavers.com>
On 11/29/19 13:40, Rémi Bernon wrote:
While this might allow the games to start, won't such approach break (some of) game mechanics or sync with servers? The games might be using this instead of GetTickCount(), having the time updated at some rare and irregular intervals looks scary.
AFAICS it's working fine although I didn't test it really deep to be honest.
It is extremely hard to test. I've got SWTOR for testing purposes, but I can't test this aspect really as you have to be a hardcore gamer to do that. The impact might be some game characters ability working a bit slower (which is critical for those playing the game), or maybe some more apparent issue but revealing in group contents only. That sort of problems would be very hard to debug.
I thought that somewhat potentially better approach than that currently in Staging would be to map user shared area to wineserver and always create a single thread in server to update the shared area. That would be somewhat similar to what Windows does if wineserver stands for kernel. I also heard that sharing memory with server is considered a no go, but I could not find any discussion why is it so.
* Then according to [1], updating the timestamps continuously could apparently break some copy-protection schemes (SecuROM for instance) that measures the time taken by some syscalls. As Wine is not exactly doing as Windows, the syscall timings could be larger and having the timestamps not increase in this particular case allows us to pass these checks.
I am not sure I quite understand this argument. There are other ways how copy-protection schemes can measure time. Do we want to effectively reduce the update frequency for, e. g., GetTickCount() to workaround this potential issue? Or even if some protection schemes are known to depend on precise timing and use user shared data for that measurements, is it an acceptable workaround to tweak update frequency to help that? IMO the better solution would be to look first if the system calls in question can be optimized. I am not sure if it is a real problem. I. e., copy protectors can of course measure the time for system calls, but if they are going to do that with the accuracy enough to distinguish between Wine and Winodws call times, that should be extremely fragile on Windows too, I doubt it can work. I suppose the protectors should check that with threshold big enough, it looks like anti debugging trick rather than a way to detect API spoofing. I've read the comment about Safedisk. Is it broken in Wine Staging now which does a sort of continuous update?
participants (3)
-
Marvin -
Paul Gofman -
Rémi Bernon