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

[Daniel Charles <daniel.charles@intel.com>]

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.

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


[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)