[PATCH v4 1/2] ntdll/tests: Test user_shared_data timestamp updates.
Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- v4: * New way to test timestamps, still some little variations after the updates are implemented, as we may miss a server update. * Add checks for InterruptTime, including some weird offset on testbot w864. * Use a single page for the whole system, initialized when wineboot.exe completes its creation, using its USD initial values. * Use atomic writes if possible, or assume that reordering guarantees on volatile accesses will be enough. PROT_WRITE doesn't always implies PROT_READ and atomic exchange would probably need readable page. We could also be mmap RW on the server side. * Address a race condition when receiving the usd fd by including it in the server call uninterruptible section. * Use the same 0x10000 size in ntdll initial mapping, later remapping and for the server side page creation. dlls/ntdll/tests/time.c | 78 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 78 insertions(+) diff --git a/dlls/ntdll/tests/time.c b/dlls/ntdll/tests/time.c index 5382a952d8d8..fe71d4458c0f 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 BOOL (WINAPI *pRtlQueryUnbiasedInterruptTime)( ULONGLONG *time ); static const int MonthLengths[2][12] = { @@ -153,6 +156,79 @@ static void test_RtlQueryTimeZoneInformation(void) wine_dbgstr_w(tzinfo.DaylightName)); } +static ULONGLONG read_ksystem_time(volatile KSYSTEM_TIME *time) +{ + ULONGLONG high, low; + do + { + high = time->High1Time; + low = time->LowPart; + } + while (high != time->High2Time); + return high << 32 | low; +} + +static void test_user_shared_data_time(void) +{ + KSHARED_USER_DATA *user_shared_data = (void *)0x7ffe0000; + ULONGLONG t1, t2, t3; + int i = 0; + + i = 0; + do + { + t1 = GetTickCount(); + if (user_shared_data->NtMajorVersion <= 5 && user_shared_data->NtMinorVersion <= 1) + t2 = ((ULONG64)user_shared_data->TickCountLowDeprecated * user_shared_data->TickCountMultiplier) >> 24; + else + t2 = (read_ksystem_time(&user_shared_data->u.TickCount) * user_shared_data->TickCountMultiplier) >> 24; + t3 = GetTickCount(); + } while(t3 < t1 && i++ < 1); /* allow for wrap, but only once */ + + ok(t1 <= t2, "USD TickCount / GetTickCount are out of order: %s %s\n", + wine_dbgstr_longlong(t1), wine_dbgstr_longlong(t2)); + todo_wine + ok(t2 <= t3, "USD TickCount / GetTickCount are out of order: %s %s\n", + wine_dbgstr_longlong(t2), wine_dbgstr_longlong(t3)); + + i = 0; + do + { + LARGE_INTEGER system_time; + NtQuerySystemTime(&system_time); + t1 = system_time.QuadPart; + t2 = read_ksystem_time(&user_shared_data->SystemTime); + NtQuerySystemTime(&system_time); + t3 = system_time.QuadPart; + } while(t3 < t1 && i++ < 1); /* allow for wrap, but only once */ + + todo_wine + ok(t1 <= t2, "USD SystemTime / NtQuerySystemTime are out of order %s %s\n", + wine_dbgstr_longlong(t1), wine_dbgstr_longlong(t2)); + ok(t2 <= t3, "USD SystemTime / NtQuerySystemTime are out of order %s %s\n", + wine_dbgstr_longlong(t2), wine_dbgstr_longlong(t3)); + + if (!pRtlQueryUnbiasedInterruptTime) + win_skip("skipping RtlQueryUnbiasedInterruptTime tests\n"); + else + { + i = 0; + do + { + pRtlQueryUnbiasedInterruptTime(&t1); + t2 = read_ksystem_time(&user_shared_data->InterruptTime); + pRtlQueryUnbiasedInterruptTime(&t3); + } while(t3 < t1 && i++ < 1); /* allow for wrap, but only once */ + + todo_wine + ok(t1 <= t2, "USD InterruptTime / RtlQueryUnbiasedInterruptTime are out of order %s %s\n", + wine_dbgstr_longlong(t1), wine_dbgstr_longlong(t2)); + ok(t2 <= t3 || broken(t2 == t3 + 82410089070) /* w864 has some weird offset on testbot */, + "USD InterruptTime / RtlQueryUnbiasedInterruptTime are out of order %s %s\n", + wine_dbgstr_longlong(t2), wine_dbgstr_longlong(t3)); + } +} + START_TEST(time) { HMODULE mod = GetModuleHandleA("ntdll.dll"); @@ -163,6 +239,7 @@ START_TEST(time) (void *)GetProcAddress(mod, "RtlQueryTimeZoneInformation"); pRtlQueryDynamicTimeZoneInformation = (void *)GetProcAddress(mod, "RtlQueryDynamicTimeZoneInformation"); + pRtlQueryUnbiasedInterruptTime = (void *)GetProcAddress(mod, "RtlQueryUnbiasedInterruptTime"); if (pRtlTimeToTimeFields && pRtlTimeFieldsToTime) test_pRtlTimeToTimeFields(); @@ -170,4 +247,5 @@ START_TEST(time) win_skip("Required time conversion functions are not available\n"); test_NtQueryPerformanceCounter(); test_RtlQueryTimeZoneInformation(); + test_user_shared_data_time(); } -- 2.26.1
The USD page is created when the first process (wineboot.exe) completes its creation, using its provided user_shared_data for initialization. The server maps the page write-only and the clients map it read-only, then the server updates the timestamps every 16 ms. The tests todo_wine cannot be completely removed as the read may have missed a server timestamp update whereas the time functions are still directly calling native clocks to get their values. It may then be possible to implement time using USD but that would reduce the clocks accuracy down to the server update frequency. Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=29168 Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/ntdll/ntdll_misc.h | 1 + dlls/ntdll/server.c | 25 ++++++++++++-- dlls/ntdll/tests/time.c | 9 +++-- dlls/ntdll/thread.c | 2 ++ dlls/ntoskrnl.exe/instr.c | 12 ------- server/file.h | 2 ++ server/mapping.c | 69 +++++++++++++++++++++++++++++++++++++++ server/process.c | 8 +++++ server/protocol.def | 1 + 9 files changed, 112 insertions(+), 17 deletions(-) diff --git a/dlls/ntdll/ntdll_misc.h b/dlls/ntdll/ntdll_misc.h index 7dca506aad42..107ebaab16dd 100644 --- a/dlls/ntdll/ntdll_misc.h +++ b/dlls/ntdll/ntdll_misc.h @@ -209,6 +209,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 size_t user_shared_data_size DECLSPEC_HIDDEN; /* completion */ extern NTSTATUS NTDLL_AddCompletion( HANDLE hFile, ULONG_PTR CompletionValue, diff --git a/dlls/ntdll/server.c b/dlls/ntdll/server.c index c0dd0f35fc3d..4bf681af201a 100644 --- a/dlls/ntdll/server.c +++ b/dlls/ntdll/server.c @@ -81,6 +81,7 @@ #include "wine/server.h" #include "wine/debug.h" #include "ntdll_misc.h" +#include "ddk/wdm.h" WINE_DEFAULT_DEBUG_CHANNEL(server); @@ -1517,8 +1518,15 @@ void server_init_process_done(void) PEB *peb = NtCurrentTeb()->Peb; IMAGE_NT_HEADERS *nt = RtlImageNtHeader( peb->ImageBaseAddress ); void *entry = (char *)peb->ImageBaseAddress + nt->OptionalHeader.AddressOfEntryPoint; + obj_handle_t usd_handle; NTSTATUS status; - int suspend; + int suspend, usd_fd = -1; + sigset_t old_set; + SIZE_T size = user_shared_data_size; + void *addr = user_shared_data; + ULONG old_prot; + + NtProtectVirtualMemory( NtCurrentProcess(), &addr, &size, PAGE_READONLY, &old_prot ); #ifdef __APPLE__ send_server_task_port(); @@ -1533,6 +1541,7 @@ void server_init_process_done(void) signal_init_process(); /* Signal the parent process to continue */ + pthread_sigmask( SIG_BLOCK, &server_block_set, &old_set ); SERVER_START_REQ( init_process_done ) { req->module = wine_server_client_ptr( peb->ImageBaseAddress ); @@ -1541,10 +1550,22 @@ void server_init_process_done(void) #endif req->entry = wine_server_client_ptr( entry ); req->gui = (nt->OptionalHeader.Subsystem != IMAGE_SUBSYSTEM_WINDOWS_CUI); - status = wine_server_call( req ); + wine_server_add_data( req, user_shared_data, sizeof(*user_shared_data) ); + status = server_call_unlocked( req ); suspend = reply->suspend; } SERVER_END_REQ; + if (!status) usd_fd = receive_fd( &usd_handle ); + pthread_sigmask( SIG_SETMASK, &old_set, NULL ); + + if (usd_fd != -1) + { + munmap( user_shared_data, user_shared_data_size ); + if (user_shared_data != mmap( user_shared_data, user_shared_data_size, + PROT_READ, MAP_SHARED | MAP_FIXED, usd_fd, 0 )) + fatal_error( "failed to remap the process user shared data\n" ); + close( usd_fd ); + } assert( !status ); signal_start_process( entry, suspend ); diff --git a/dlls/ntdll/tests/time.c b/dlls/ntdll/tests/time.c index fe71d4458c0f..7f81c20f21ec 100644 --- a/dlls/ntdll/tests/time.c +++ b/dlls/ntdll/tests/time.c @@ -185,9 +185,10 @@ static void test_user_shared_data_time(void) t3 = GetTickCount(); } while(t3 < t1 && i++ < 1); /* allow for wrap, but only once */ + /* FIXME: not always in order, but should be close */ + todo_wine_if(t1 > t2 && t1 - t2 < 50) ok(t1 <= t2, "USD TickCount / GetTickCount are out of order: %s %s\n", wine_dbgstr_longlong(t1), wine_dbgstr_longlong(t2)); - todo_wine ok(t2 <= t3, "USD TickCount / GetTickCount are out of order: %s %s\n", wine_dbgstr_longlong(t2), wine_dbgstr_longlong(t3)); @@ -202,7 +203,8 @@ static void test_user_shared_data_time(void) t3 = system_time.QuadPart; } while(t3 < t1 && i++ < 1); /* allow for wrap, but only once */ - todo_wine + /* FIXME: not always in order, but should be close */ + todo_wine_if(t1 > t2 && t1 - t2 < 50 * TICKSPERMSEC) ok(t1 <= t2, "USD SystemTime / NtQuerySystemTime are out of order %s %s\n", wine_dbgstr_longlong(t1), wine_dbgstr_longlong(t2)); ok(t2 <= t3, "USD SystemTime / NtQuerySystemTime are out of order %s %s\n", @@ -220,7 +222,8 @@ static void test_user_shared_data_time(void) pRtlQueryUnbiasedInterruptTime(&t3); } while(t3 < t1 && i++ < 1); /* allow for wrap, but only once */ - todo_wine + /* FIXME: not always in order, but should be close */ + todo_wine_if(t1 > t2 && t1 - t2 < 50 * TICKSPERMSEC) ok(t1 <= t2, "USD InterruptTime / RtlQueryUnbiasedInterruptTime are out of order %s %s\n", wine_dbgstr_longlong(t1), wine_dbgstr_longlong(t2)); ok(t2 <= t3 || broken(t2 == t3 + 82410089070) /* w864 has some weird offset on testbot */, diff --git a/dlls/ntdll/thread.c b/dlls/ntdll/thread.c index e27b3a0c6dd3..ef3a94cf4828 100644 --- a/dlls/ntdll/thread.c +++ b/dlls/ntdll/thread.c @@ -56,6 +56,7 @@ WINE_DEFAULT_DEBUG_CHANNEL(thread); #endif struct _KUSER_SHARED_DATA *user_shared_data = NULL; +size_t user_shared_data_size = 0; static const WCHAR default_windirW[] = {'C',':','\\','w','i','n','d','o','w','s',0}; void (WINAPI *kernel32_start_process)(LPTHREAD_START_ROUTINE,void*) = NULL; @@ -244,6 +245,7 @@ TEB *thread_init(void) exit(1); } user_shared_data = addr; + user_shared_data_size = size; memcpy( user_shared_data->NtSystemRoot, default_windirW, sizeof(default_windirW) ); /* allocate and initialize the PEB */ diff --git a/dlls/ntoskrnl.exe/instr.c b/dlls/ntoskrnl.exe/instr.c index 77803f07d725..0973b3a80a07 100644 --- a/dlls/ntoskrnl.exe/instr.c +++ b/dlls/ntoskrnl.exe/instr.c @@ -593,15 +593,6 @@ static void fake_syscall_function(void) } -static void update_shared_data(void) -{ - struct _KUSER_SHARED_DATA *shared_data = (struct _KUSER_SHARED_DATA *)wine_user_shared_data; - - shared_data->u.TickCountQuad = GetTickCount64(); - shared_data->u.TickCount.High2Time = shared_data->u.TickCount.High1Time; -} - - /*********************************************************************** * emulate_instruction * @@ -802,7 +793,6 @@ static DWORD emulate_instruction( EXCEPTION_RECORD *rec, CONTEXT *context ) if (offset <= sizeof(KSHARED_USER_DATA) - data_size) { ULONGLONG temp = 0; - update_shared_data(); memcpy( &temp, wine_user_shared_data + offset, data_size ); store_reg_word( context, instr[2], (BYTE *)&temp, long_op, rex ); context->Rip += prefixlen + len + 2; @@ -823,7 +813,6 @@ static DWORD emulate_instruction( EXCEPTION_RECORD *rec, CONTEXT *context ) if (offset <= sizeof(KSHARED_USER_DATA) - data_size) { - update_shared_data(); switch (*instr) { case 0x8a: store_reg_byte( context, instr[1], wine_user_shared_data + offset, rex ); break; @@ -845,7 +834,6 @@ static DWORD emulate_instruction( EXCEPTION_RECORD *rec, CONTEXT *context ) if (offset <= sizeof(KSHARED_USER_DATA) - data_size) { - update_shared_data(); memcpy( &context->Rax, wine_user_shared_data + offset, data_size ); context->Rip += prefixlen + len + 1; return ExceptionContinueExecution; diff --git a/server/file.h b/server/file.h index 7395814dadd2..d577aaed3cfa 100644 --- a/server/file.h +++ b/server/file.h @@ -173,6 +173,8 @@ extern struct file *get_mapping_file( struct process *process, client_ptr_t base extern void free_mapped_views( struct process *process ); extern int get_page_size(void); +int get_user_shared_data_fd( const void *usd_init, data_size_t usd_size ); + /* device functions */ extern struct object *create_named_pipe_device( struct object *root, const struct unicode_str *name ); diff --git a/server/mapping.c b/server/mapping.c index 6990a1913d76..d73d8de9c283 100644 --- a/server/mapping.c +++ b/server/mapping.c @@ -35,6 +35,7 @@ #define WIN32_NO_STATUS #include "windef.h" #include "winternl.h" +#include "ddk/wdm.h" #include "file.h" #include "handle.h" @@ -943,6 +944,74 @@ int get_page_size(void) return page_mask + 1; } +static int kusd_fd; +static KSHARED_USER_DATA *kusd; +static const timeout_t kusd_timeout = 16 * -TICKS_PER_SEC / 1000; + +static void kusd_set_current_time( void *private ) +{ + ULONG system_time_high = current_time >> 32; + ULONG system_time_low = current_time & 0xffffffff; + ULONG interrupt_time_high = monotonic_time >> 32; + ULONG interrupt_time_low = monotonic_time & 0xffffffff; + ULONG tick_count_high = (monotonic_time * 1000 / TICKS_PER_SEC) >> 32; + ULONG tick_count_low = (monotonic_time * 1000 / TICKS_PER_SEC) & 0xffffffff; + KSHARED_USER_DATA *ptr = kusd; + + add_timeout_user( kusd_timeout, kusd_set_current_time, NULL ); + +#if defined(__GNUC__) && ((__GNUC__ > 4) || ((__GNUC__ == 4) && (__GNUC_MINOR__ >= 7))) + __atomic_store_n(&ptr->SystemTime.High2Time, system_time_high, __ATOMIC_SEQ_CST); + __atomic_store_n(&ptr->SystemTime.LowPart, system_time_low, __ATOMIC_SEQ_CST); + __atomic_store_n(&ptr->SystemTime.High1Time, system_time_high, __ATOMIC_SEQ_CST); + + __atomic_store_n(&ptr->InterruptTime.High2Time, interrupt_time_high, __ATOMIC_SEQ_CST); + __atomic_store_n(&ptr->InterruptTime.LowPart, interrupt_time_low, __ATOMIC_SEQ_CST); + __atomic_store_n(&ptr->InterruptTime.High1Time, interrupt_time_high, __ATOMIC_SEQ_CST); + + __atomic_store_n(&ptr->TickCount.High2Time, tick_count_high, __ATOMIC_SEQ_CST); + __atomic_store_n(&ptr->TickCount.LowPart, tick_count_low, __ATOMIC_SEQ_CST); + __atomic_store_n(&ptr->TickCount.High1Time, tick_count_high, __ATOMIC_SEQ_CST); + __atomic_store_n(&ptr->TickCountLowDeprecated, tick_count_low, __ATOMIC_SEQ_CST); +#else + ptr->SystemTime.High2Time = system_time_high; + ptr->SystemTime.LowPart = system_time_low; + ptr->SystemTime.High1Time = system_time_high; + + ptr->InterruptTime.High2Time = interrupt_time_high; + ptr->InterruptTime.LowPart = interrupt_time_low; + ptr->InterruptTime.High1Time = interrupt_time_high; + + ptr->TickCount.High2Time = tick_count_high; + ptr->TickCount.LowPart = tick_count_low; + ptr->TickCount.High1Time = tick_count_high; + ptr->TickCountLowDeprecated = tick_count_low; +#endif +} + +int get_user_shared_data_fd( const void *usd_init, data_size_t usd_size ) +{ + /* keep it the same as user_shared_data_size in ntdll */ + size_t size = 0x10000; + + if (sizeof(*kusd) != usd_size) return -1; + if (kusd) return kusd_fd; + + if ((kusd_fd = create_temp_file( size )) == -1) + return -1; + + if ((kusd = mmap( NULL, size, PROT_WRITE, MAP_SHARED, kusd_fd, 0 )) == MAP_FAILED) + { + close( kusd_fd ); + return -1; + } + + memcpy( kusd, usd_init, usd_size ); + + kusd_set_current_time( NULL ); + return kusd_fd; +} + /* create a file mapping */ DECL_HANDLER(create_mapping) { diff --git a/server/process.c b/server/process.c index 73984f363f59..80f9c5c6f97e 100644 --- a/server/process.c +++ b/server/process.c @@ -1356,6 +1356,7 @@ DECL_HANDLER(init_process_done) { struct process_dll *dll; struct process *process = current->process; + int usd_fd; if (is_process_init_done(process)) { @@ -1367,6 +1368,13 @@ DECL_HANDLER(init_process_done) set_error( STATUS_DLL_NOT_FOUND ); return; } + if ((usd_fd = get_user_shared_data_fd( get_req_data(), get_req_data_size() )) == -1) + { + set_error( STATUS_NO_MEMORY ); + return; + } + + send_client_fd( process, usd_fd, -1 ); /* main exe is the first in the dll list */ list_remove( &dll->entry ); diff --git a/server/protocol.def b/server/protocol.def index 06a29b153ea0..b1fe198ad043 100644 --- a/server/protocol.def +++ b/server/protocol.def @@ -854,6 +854,7 @@ struct rawinput_device mod_handle_t module; /* main module base address */ client_ptr_t ldt_copy; /* address of LDT copy (in thread address space) */ client_ptr_t entry; /* process entry point */ + VARARG(usd,bytes); /* USD initialization data */ @REPLY int suspend; /* is process suspended? */ @END -- 2.26.1
On 4/30/20 11:58 AM, Rémi Bernon wrote:
The USD page is created when the first process (wineboot.exe) completes its creation, using its provided user_shared_data for initialization.
The server maps the page write-only and the clients map it read-only, then the server updates the timestamps every 16 ms.
The tests todo_wine cannot be completely removed as the read may have missed a server timestamp update whereas the time functions are still directly calling native clocks to get their values.
Ech, sorry, I didn't consider that when giving that advice. I guess it'll work out in the end, if we implement GetTickCount() this way...
It may then be possible to implement time using USD but that would reduce the clocks accuracy down to the server update frequency.
If Windows has updates that slow, I think this makes sense. We'd probably want to use a different channel for +timestamp, though [QueryPerformanceCounter(), I guess?] There's probably other consequences I'm not considering...
On 4/30/20 21:21, Zebediah Figura wrote:
It may then be possible to implement time using USD but that would reduce the clocks accuracy down to the server update frequency. If Windows has updates that slow, I think this makes sense. We'd probably want to use a different channel for +timestamp, though [QueryPerformanceCounter(), I guess?]
There's probably other consequences I'm not considering...
It might be potentially usable for GetTickCount() which has resolution around 10-15ms (though dangerous probably), but QueryPerformanceCounter() is designed as higher resolution timer, every game is using it and expects better resolution. 15-16ms is a long time. But proper Unix functions for getting high resolution time should already be quick, they also end up getting time from shared memory pages maintained by kernel.
On 4/30/20 1:28 PM, Paul Gofman wrote:
On 4/30/20 21:21, Zebediah Figura wrote:
It may then be possible to implement time using USD but that would reduce the clocks accuracy down to the server update frequency. If Windows has updates that slow, I think this makes sense. We'd probably want to use a different channel for +timestamp, though [QueryPerformanceCounter(), I guess?]
There's probably other consequences I'm not considering...
It might be potentially usable for GetTickCount() which has resolution around 10-15ms (though dangerous probably), but QueryPerformanceCounter() is designed as higher resolution timer, every game is using it and expects better resolution. 15-16ms is a long time. But proper Unix functions for getting high resolution time should already be quick, they also end up getting time from shared memory pages maintained by kernel.
Er, right, I wasn't saying that we should implement QueryPerformanceCounter() using the shared data, but rather we should use it to implement WINEDEBUG=+timestamp. Sorry if that came out ambiguous.
Rémi Bernon <rbernon(a)codeweavers.com> writes:
@@ -1533,6 +1541,7 @@ void server_init_process_done(void) signal_init_process();
/* Signal the parent process to continue */ + pthread_sigmask( SIG_BLOCK, &server_block_set, &old_set ); SERVER_START_REQ( init_process_done ) { req->module = wine_server_client_ptr( peb->ImageBaseAddress ); @@ -1541,10 +1550,22 @@ void server_init_process_done(void) #endif req->entry = wine_server_client_ptr( entry ); req->gui = (nt->OptionalHeader.Subsystem != IMAGE_SUBSYSTEM_WINDOWS_CUI); - status = wine_server_call( req ); + wine_server_add_data( req, user_shared_data, sizeof(*user_shared_data) ); + status = server_call_unlocked( req ); suspend = reply->suspend; } SERVER_END_REQ; + if (!status) usd_fd = receive_fd( &usd_handle ); + pthread_sigmask( SIG_SETMASK, &old_set, NULL );
It should be possible to use standard file mapping functions (NtOpenSection, NtMapViewOfSection etc.) instead of adding custom handling to the init_process_done request. -- Alexandre Julliard julliard(a)winehq.org
On 5/4/20 5:49 PM, Alexandre Julliard wrote:
Rémi Bernon <rbernon(a)codeweavers.com> writes:
@@ -1533,6 +1541,7 @@ void server_init_process_done(void) signal_init_process();
/* Signal the parent process to continue */ + pthread_sigmask( SIG_BLOCK, &server_block_set, &old_set ); SERVER_START_REQ( init_process_done ) { req->module = wine_server_client_ptr( peb->ImageBaseAddress ); @@ -1541,10 +1550,22 @@ void server_init_process_done(void) #endif req->entry = wine_server_client_ptr( entry ); req->gui = (nt->OptionalHeader.Subsystem != IMAGE_SUBSYSTEM_WINDOWS_CUI); - status = wine_server_call( req ); + wine_server_add_data( req, user_shared_data, sizeof(*user_shared_data) ); + status = server_call_unlocked( req ); suspend = reply->suspend; } SERVER_END_REQ; + if (!status) usd_fd = receive_fd( &usd_handle ); + pthread_sigmask( SIG_SETMASK, &old_set, NULL );
It should be possible to use standard file mapping functions (NtOpenSection, NtMapViewOfSection etc.) instead of adding custom handling to the init_process_done request.
Well, there was actually an issue with the service implementation because it used standard section function to map the USD. The handle was allocated early and under some circumstances could collide with a console handle copied from a parent process. The details are a bit convoluted but it's also the reason behind Paul's recent mapping patches. With these patches, NtWriteFile should not be able to write to the section handle anymore, but the idea was also that maybe the USD should not use standard mapping functions to avoid such issues. Also, on Windows NtQueryVirtualMemory returns MEM_PRIVATE for the USD, where it would return MEM_MAPPED if a section is used (although unlikely that any application cares). -- Rémi Bernon <rbernon(a)codeweavers.com>
Rémi Bernon <rbernon(a)codeweavers.com> writes:
On 5/4/20 5:49 PM, Alexandre Julliard wrote:
Rémi Bernon <rbernon(a)codeweavers.com> writes:
@@ -1533,6 +1541,7 @@ void server_init_process_done(void) signal_init_process(); /* Signal the parent process to continue */ + pthread_sigmask( SIG_BLOCK, &server_block_set, &old_set ); SERVER_START_REQ( init_process_done ) { req->module = wine_server_client_ptr( peb->ImageBaseAddress ); @@ -1541,10 +1550,22 @@ void server_init_process_done(void) #endif req->entry = wine_server_client_ptr( entry ); req->gui = (nt->OptionalHeader.Subsystem != IMAGE_SUBSYSTEM_WINDOWS_CUI); - status = wine_server_call( req ); + wine_server_add_data( req, user_shared_data, sizeof(*user_shared_data) ); + status = server_call_unlocked( req ); suspend = reply->suspend; } SERVER_END_REQ; + if (!status) usd_fd = receive_fd( &usd_handle ); + pthread_sigmask( SIG_SETMASK, &old_set, NULL );
It should be possible to use standard file mapping functions (NtOpenSection, NtMapViewOfSection etc.) instead of adding custom handling to the init_process_done request.
Well, there was actually an issue with the service implementation because it used standard section function to map the USD. The handle was allocated early and under some circumstances could collide with a console handle copied from a parent process. The details are a bit convoluted but it's also the reason behind Paul's recent mapping patches.
I see no reason that this would be an issue, unless you are leaking the handle.
With these patches, NtWriteFile should not be able to write to the section handle anymore, but the idea was also that maybe the USD should not use standard mapping functions to avoid such issues. Also, on Windows NtQueryVirtualMemory returns MEM_PRIVATE for the USD, where it would return MEM_MAPPED if a section is used (although unlikely that any application cares).
If it has to be MEM_PRIVATE we can use mmap(MAP_FIXED) to map it, but I think it can still be a standard object. -- Alexandre Julliard julliard(a)winehq.org
On 5/5/20 12:31, Alexandre Julliard wrote:
Rémi Bernon <rbernon(a)codeweavers.com> writes:
On 5/4/20 5:49 PM, Alexandre Julliard wrote:
Rémi Bernon <rbernon(a)codeweavers.com> writes:
@@ -1533,6 +1541,7 @@ void server_init_process_done(void) signal_init_process(); /* Signal the parent process to continue */ + pthread_sigmask( SIG_BLOCK, &server_block_set, &old_set ); SERVER_START_REQ( init_process_done ) { req->module = wine_server_client_ptr( peb->ImageBaseAddress ); @@ -1541,10 +1550,22 @@ void server_init_process_done(void) #endif req->entry = wine_server_client_ptr( entry ); req->gui = (nt->OptionalHeader.Subsystem != IMAGE_SUBSYSTEM_WINDOWS_CUI); - status = wine_server_call( req ); + wine_server_add_data( req, user_shared_data, sizeof(*user_shared_data) ); + status = server_call_unlocked( req ); suspend = reply->suspend; } SERVER_END_REQ; + if (!status) usd_fd = receive_fd( &usd_handle ); + pthread_sigmask( SIG_SETMASK, &old_set, NULL ); It should be possible to use standard file mapping functions (NtOpenSection, NtMapViewOfSection etc.) instead of adding custom handling to the init_process_done request.
Well, there was actually an issue with the service implementation because it used standard section function to map the USD. The handle was allocated early and under some circumstances could collide with a console handle copied from a parent process. The details are a bit convoluted but it's also the reason behind Paul's recent mapping patches. I see no reason that this would be an issue, unless you are leaking the handle.
Wine currently allows invalid handle values (copied from parent process without duplicating) for std handles (at least before kernel32 and msvcrt process attach) if process is created without handle inheritance and CREATE_NEW_CONSOLE flag. That can also happen on Windows before Windows 10, but (AFAIK) only when std handles are given explicitly in STARTUPINFO with no inheritance. The scenario behind that problem was: - parent process (Rockstar Launcher) creates a child (game) process without handle inheritance and CREATE_NEW_CONSOLE flag; hStdErr value gets to the child process PEB hStdErr; - initializtion in ntdll creates Nt section, and the value for handle happens to be identical to the one currently in hStdErr; - msvcrt initialization would normally zero hStdErr handle if it was invalid, but it is valid and GetFileType() returns FILE_TYPE_DISK for it, so stderr gets fully initialized with Nt section; - DXVK starts writing its logs to stderr (which succeeds as Wine currently allows working with Nt sections as files) and those logs end up in user shared data area. I've sent the patches for review which do not allow using Nt section headers as files (fix read / write to them succeding) and making GetFileType() return FILE_TYPE_UNKNOWN as on Windows. I am not sure, maybe we should really go forward and completely exclude the possibility to get invalid handle values in PEB std handles on early process creation? I did some testing (using a naked DLL which is loaded before msvcrt to save hStdErr value), it looks like Windows clears the invalid handle before msvcrt. It looked to me that the right thing would be to just always zero std handles in server new_process handler if handles are not inherited. But currently it duplicates the handles if CREATE_NEW_CONSOLE flag is not set. I could not find the scenario on Windows when it would duplicate the handle, but I am not sure yet why that handle duplication was added in the first place and maybe it will break some console handling, while I could not find a broken test with zeroing handles so far. Another concern is, those launchers often use PROC_THREAD_ATTRIBUTE_HANDLE_LIST process creation attribute these days which we ignore now without known issues. But maybe we should implement that first before touching anything related to handles inheritance.
On 5/5/20 12:00 PM, Paul Gofman wrote:
On 5/5/20 12:31, Alexandre Julliard wrote:
Rémi Bernon <rbernon(a)codeweavers.com> writes:
On 5/4/20 5:49 PM, Alexandre Julliard wrote:
Rémi Bernon <rbernon(a)codeweavers.com> writes:
@@ -1533,6 +1541,7 @@ void server_init_process_done(void) signal_init_process(); /* Signal the parent process to continue */ + pthread_sigmask( SIG_BLOCK, &server_block_set, &old_set ); SERVER_START_REQ( init_process_done ) { req->module = wine_server_client_ptr( peb->ImageBaseAddress ); @@ -1541,10 +1550,22 @@ void server_init_process_done(void) #endif req->entry = wine_server_client_ptr( entry ); req->gui = (nt->OptionalHeader.Subsystem != IMAGE_SUBSYSTEM_WINDOWS_CUI); - status = wine_server_call( req ); + wine_server_add_data( req, user_shared_data, sizeof(*user_shared_data) ); + status = server_call_unlocked( req ); suspend = reply->suspend; } SERVER_END_REQ; + if (!status) usd_fd = receive_fd( &usd_handle ); + pthread_sigmask( SIG_SETMASK, &old_set, NULL ); It should be possible to use standard file mapping functions (NtOpenSection, NtMapViewOfSection etc.) instead of adding custom handling to the init_process_done request.
Well, there was actually an issue with the service implementation because it used standard section function to map the USD. The handle was allocated early and under some circumstances could collide with a console handle copied from a parent process. The details are a bit convoluted but it's also the reason behind Paul's recent mapping patches. I see no reason that this would be an issue, unless you are leaking the handle.
Wine currently allows invalid handle values (copied from parent process without duplicating) for std handles (at least before kernel32 and msvcrt process attach) if process is created without handle inheritance and CREATE_NEW_CONSOLE flag. That can also happen on Windows before Windows 10, but (AFAIK) only when std handles are given explicitly in STARTUPINFO with no inheritance.
The scenario behind that problem was:
- parent process (Rockstar Launcher) creates a child (game) process without handle inheritance and CREATE_NEW_CONSOLE flag; hStdErr value gets to the child process PEB hStdErr;
- initializtion in ntdll creates Nt section, and the value for handle happens to be identical to the one currently in hStdErr;
- msvcrt initialization would normally zero hStdErr handle if it was invalid, but it is valid and GetFileType() returns FILE_TYPE_DISK for it, so stderr gets fully initialized with Nt section;
- DXVK starts writing its logs to stderr (which succeeds as Wine currently allows working with Nt sections as files) and those logs end up in user shared data area.
I've sent the patches for review which do not allow using Nt section headers as files (fix read / write to them succeding) and making GetFileType() return FILE_TYPE_UNKNOWN as on Windows.
I am not sure, maybe we should really go forward and completely exclude the possibility to get invalid handle values in PEB std handles on early process creation? I did some testing (using a naked DLL which is loaded before msvcrt to save hStdErr value), it looks like Windows clears the invalid handle before msvcrt. It looked to me that the right thing would be to just always zero std handles in server new_process handler if handles are not inherited. But currently it duplicates the handles if CREATE_NEW_CONSOLE flag is not set. I could not find the scenario on Windows when it would duplicate the handle, but I am not sure yet why that handle duplication was added in the first place and maybe it will break some console handling, while I could not find a broken test with zeroing handles so far. Another concern is, those launchers often use PROC_THREAD_ATTRIBUTE_HANDLE_LIST process creation attribute these days which we ignore now without known issues. But maybe we should implement that first before touching anything related to handles inheritance.
IIUC Alexandre point is that if the section handle is closed after being mapped, then although its value could collide with stderr, it will never be possible to write to it later on. I'm not sure to remember all the details but I think I kept it open because I wasn't sure if the mapping would stay valid or not after the section is closed and also possibly because I had to keep the section alive until the service was started. If it is, like in the Unix world, and as in this case the mapping will be kept alive on the server-side anyway, it is then probably fine to use a section here. -- Rémi Bernon <rbernon(a)codeweavers.com>
participants (4)
-
Alexandre Julliard -
Paul Gofman -
Rémi Bernon -
Zebediah Figura