From: Paul Gofman <gofmanp@gmail.com>
Subject: Re: [PATCH v2 2/4] ntdll: Support creating processes with specified parent.
Message-Id: <f0b0ee7d-e936-0b7f-f6df-b6818562eeab@gmail.com>
Date: Thu, 12 Dec 2019 13:45:35 +0300
In-Reply-To: <87tv65hllp.fsf@wine>
References: <20191209215114.210057-1-gofmanp@gmail.com> <20191209215114.210057-2-gofmanp@gmail.com> <87zhfygcvo.fsf@wine> <c0c2b089-f879-5956-a57f-96d815ec2333@gmail.com> <87tv65hllp.fsf@wine>

On 12/12/19 13:41, Alexandre Julliard wrote:
> Paul Gofman <gofmanp@gmail.com> writes:
>
>> On 12/12/19 11:35, Alexandre Julliard wrote:
>>> Paul Gofman <gofmanp@gmail.com> writes:
>>>
>>>> @@ -1667,8 +1667,14 @@ NTSTATUS WINAPI RtlCreateUserProcess( UNICODE_STRING *path, ULONG attributes,
>>>>  
>>>>      RtlNormalizeProcessParams( params );
>>>>  
>>>> -    TRACE( "%s image %s cmdline %s\n", debugstr_us( path ),
>>>> -           debugstr_us( &params->ImagePathName ), debugstr_us( &params->CommandLine ));
>>>> +    TRACE( "%s image %s cmdline %s, parent %p.\n", debugstr_us( path ),
>>>> +           debugstr_us( &params->ImagePathName ), debugstr_us( &params->CommandLine ), parent);
>>>> +
>>>> +    if (parent == INVALID_HANDLE_VALUE)
>>>> +    {
>>>> +        memset(info, 0, sizeof(*info));
>>>> +        return STATUS_INVALID_HANDLE;
>>>> +    }
>>> You are still using INVALID_HANDLE_VALUE here, I don't think that makes
>>> sense. The check for an invalid handle should use a truly invalid value
>>> like 0xdeadbeef.
>>>
>>> It may be interesting to test what happens with GetCurrentProcess(), and
>>> also some other handle pointing to the current process, but that should
>>> be separate from the generic invalid handle test.
>>>
>>     The check for invalid handle here is not for something explicitly
>> assigned in kernelbase, CreateProcessInternalW() in my v2 patch doesn't
>> encode any error conditions in INVALID_HANDLE_VALUE anymore. The truly
>> invalid handles (like 0xdeadbeef) get error from server call. The same
>> is for the other pseudo handles like that for current thread as they
>> have wrong type. Unlike INVALID_HANDLE_VALUE which is taken as current
>> process there. I have a test with 0xdeadbeef in the next patch which
>> returns an error on process creation on all Windows versions as well as
>> with these patches without any explicit checks before server call. There
>> are also tests with INVALID_HANDLE_VALUE and GetCurrentProcess() present
>> in the next patch which fail to create the process on Vista, w7 and w10
>> (and creates a process on w7u / w8), so I added the explicit check to
>> satisfy the "mainstream" behaviour. Now I briefly checked what happens
>> if I set a duplicated GetCurrentProcess() handle or handle from
>> OpenProcess(..., GetCurrentProcessId()) for parent handle, and the
>> process creation succeeds with that valid non-pseudo handle for the
>> current process. Should I add this test?
>>
>>     So should I maybe move this check for 0xffffffff handle to
>> CreateProcessInternalW(), where it checks for 0 handle already?
> I'm not sure we need the check at all, since it works on some Windows
> versions, and it works for other handles to the current
> process. Accepting GetCurrentProcess() doesn't strike me as something
> that needs to be considered broken.
>
> More to the point, INVALID_HANDLE_VALUE is meaningless with process
> handles. It's a value that's used with file handle APIs, but for
> processes, the error value is 0, and 0xffffffff is the current process
> pseudo-handle.
>
So I will remove the check then and change what is 'broken' in the tests.