[PATCH v4 2/2] server: Add USD support with timestamp updates.

[Paul Gofman]

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