On 5/5/20 12:31, Alexandre Julliard wrote:
Rémi Bernon rbernon@codeweavers.com writes:
On 5/4/20 5:49 PM, Alexandre Julliard wrote:
Rémi Bernon rbernon@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.