[PATCH 3/4] ntdll: Validate job handles at process creation.

[Paul Gofman]

On 5/20/21 11:56, Alexandre Julliard wrote:
> Paul Gofman <pgofman at codeweavers.com> writes:
>
>> On 5/20/21 10:56, Alexandre Julliard wrote:
>>> Paul Gofman <pgofman at codeweavers.com> writes:
>>>
>>>> +    job_handle_count = req->jobs_size / sizeof(*handles);
>>>> +    for (i = 0; i < job_handle_count; ++i)
>>>> +    {
>>>> +        if (!(job = get_job_obj( current->process, job_handles[i], JOB_OBJECT_ASSIGN_PROCESS )))
>>>> +        {
>>>> +            close( socket_fd );
>>>> +            goto done;
>>>> +        }
>>>> +        release_object( job );
>>>> +    }
>>> It doesn't seem useful to pre-check the handles, you'll need to handle
>>> failures when assigning the process anyway.
>>>
>> The reason I did it this way is that invalid handle aborts the process
>> creation without changing any job's state, while error adding job to
>> process leaves the state of the jobs earlier in the list altered if
>> aborting on adding next job. So I probably need to precheck (get) the
>> handles before assigning the process anyway. Should I move this check to
>> that place (and squash the patches)?
> I'd expect that the process cleanup would undo what was done to the
> previous jobs. Otherwise you can't allow add_job_process() to fail in
> patch 4.
>
But my tests in patch 4 show that this is not the case. When the process
can't be assigned to the second job in list, the first one still gets
the parent and even sends completions for adding and removing
nonexistent process. My test, besides querying process counts from that
unexpectedly altered job, checks that it got the parent by attempting
another process create to be sure.