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

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?