Re: [i-g-t,1/2] lib/igt_core: Check if helper proc was alive before kill()

[Gustavo Sousa <gustavo.sousa@intel.com>]

Daniel Charles <daniel.charles@intel.com> writes:

> On 10/13/2026 10:56 AM, Gustavo Sousa wrote:
>> Daniel Charles <daniel.charles@intel.com> writes:
>>
>>> On 10/7/2026 7:16 AM, Gustavo Sousa wrote:
>>>> The function igt_stop_helper() does a sanity check by verifying if the
>>>> helper process died "too early", however it does that by checking if
>>>> it was terminated due to not handling the signal used in the kill()
>>>> call.
>>>>
>>>> That logic has a flaw though: it the helper process gracefully handles
>>>> the signal sent by kill() and exits normally, igt_stop_helper() will
>>>> mistakenly take that as dying too early, because WIFSIGNALED(status)
>>>> will be false.
>>>>
>>>> That's the case with the pw-reserve process: in an upcoming change we
>>>> will change pipewire_reserve_wait() to call it with execlp().  Because
>>>> it handles SIGTERM and exits normally, a warning and assertion failure
>>>> would happen with the current logic.
>>>>
>>>> Fix that by checking if the process was alive before calling kill().
>>>>
>>>> Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
>>>> ---
>>>>    lib/igt_core.c | 20 +++++++++-----------
>>>>    1 file changed, 9 insertions(+), 11 deletions(-)
>>>>
>>>> diff --git a/lib/igt_core.c b/lib/igt_core.c
>>>> index 8ee9432923e6..d9949ea131f8 100644
>>>> --- a/lib/igt_core.c
>>>> +++ b/lib/igt_core.c
>>>> @@ -2652,13 +2652,6 @@ int igt_wait_helper(struct igt_helper_process *proc)
>>>>    	return status;
>>>>    }
>>>>    
>>>> -static bool helper_was_alive(struct igt_helper_process *proc,
>>>> -			     int status)
>>>> -{
>>>> -	return (WIFSIGNALED(status) &&
>>>> -		WTERMSIG(status) == (proc->use_SIGKILL ? SIGKILL : SIGTERM));
>>>> -}
>>>> -
>>>>    /**
>>>>     * igt_stop_helper:
>>>>     * @proc: #igt_helper_process structure
>>>> @@ -2670,17 +2663,22 @@ static bool helper_was_alive(struct igt_helper_process *proc,
>>>>    void igt_stop_helper(struct igt_helper_process *proc)
>>>>    {
>>>>    	int status;
>>>> +	int ret;
>>>>    
>>>>    	if (!proc->running) /* never even started */
>>>>    		return;
>>>>    
>>>> +	ret = waitpid(proc->pid, &status, WNOHANG);
>>> put waitpid in a do/while loop to catch any EINT return that may need a
>>> retry.
>> Do you mean EINTR? EINTR would happen only if WNOHANG was not passed in
>> options.
> Yes it was a typo  and you're right on WNOHANG according to [1]
>>
>> This call is to check if the child process was still alive by the time
>> we are about to kill it.
>>
>>>> +
>>>> +	if (ret == proc->pid)
>>>> +		igt_debug("Helper died too early with status=%d\n", status);
>>>> +
>>>>    	/* failure here means the pid is already dead and so waiting is safe */
>>>>    	kill(proc->pid, proc->use_SIGKILL ? SIGKILL : SIGTERM);
>>>>    
>>>> -	status = igt_wait_helper(proc);
>>>> -	if (!helper_was_alive(proc, status))
>>>> -		igt_debug("Helper died too early with status=%d\n", status);
>>>> -	assert(helper_was_alive(proc, status));
>>>> +	assert(ret == 0);
>>> This assert can be moved before kill call to make sure the correct
>>> process is killed.
>> Hm... This function already assumes proc->pid is the correct process to
>> be killed.  The objective of the waitpid() call above is rather to check
>> if the process was still alive (the expected scenario).
>>
>> If we move this assert() to happen before the kill() call, we risk
>> leaking the process, because an assertion failure would keep us from
>> killing the child process.
>
> The process will be already reaped after waitpid if it returns the PID, 
> then kill may signal a different process that took the same PID.

Hm... Yeah, I can see this edge case where the PID could get re-used
between the waitpid() and kill() call.  Not sure, how easily we would
hit it, but this doesn't make the patch right and I agree we should
avoid trying to kill the process if the waitpid() call tells us the
process is already dead.

>
> So call kill only when ret == 0 ? and keep the assert in the current place.

I think we should call kill() only if ret != proc->pid, which ensures we
also try to kill() if the waitpid() call fails for unexpected reasons.

--
Gustavo Sousa

>
>
> [1] https://man7.org/linux/man-pages/man2/wait.2.html#ERRORS
>
> -- 
>
> Daniel.
>
>> --
>> Gustavo Sousa
>>
>>> -- Daniel.
>>>
>>>> +
>>>> +	igt_wait_helper(proc);
>>>>    }
>>>>    
>>>>    static void children_exit_handler(int sig)