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

[Rémi Bernon]

On 5/5/20 12:00 PM, Paul Gofman wrote:
> 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.
> 

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 at codeweavers.com>