Re: [PATCH v2 2/4] ntdll: Support creating processes with specified parent.

[Alexandre Julliard]

Paul Gofman  writes:
> On 12/12/19 11:35, Alexandre Julliard wrote:
> > Paul Gofman  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.