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

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.

--
Gustavo Sousa

>
> -- Daniel.
>
>> +
>> +	igt_wait_helper(proc);
>>   }
>>   
>>   static void children_exit_handler(int sig)