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

[Alexandre Julliard]

Paul Gofman <pgofman at codeweavers.com> writes:

> 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.

Ah, I see. Sounds very much like a Windows bug, but I guess we have to
do the same then. Thanks!

-- 
Alexandre Julliard
julliard at winehq.org