Re: [PATCH v4 06/15] tools subcmd: Robust fallback and existence checks for process reaping

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Medium] The `kill(pid, 0)` fallback causes infinite loops with zombies and contains mathematically dead code.
- [Medium] TOCTOU race condition in `kill(pid, 0)` fallback enabling PID reuse bugs.
- [Low] Undeclared variables `waiting` and `status` cause a build failure.
- [Low] Unused variables and unconditional `sprintf()` on non-Linux platforms.
--

commit eb3a93e2d845551ba621cb38b0af3a1f2bb00750
Author: Ian Rogers <irogers@google.com>

tools subcmd: Robust fallback and existence checks for process reaping

This commit updates process checking to handle negative PIDs and transitions
the procfs status fallback logic. It replaces active waitpid() calls with a
kill(pid, 0) check to detect if a process is still running or has exited.

> diff --git a/tools/lib/subcmd/run-command.c b/tools/lib/subcmd/run-command.c
> --- a/tools/lib/subcmd/run-command.c
> +++ b/tools/lib/subcmd/run-command.c
> @@ -229,20 +229,64 @@ static int wait_or_whine(struct child_process *cmd, bool block)
>  
>  int check_if_command_finished(struct child_process *cmd)
>  {
> -#ifdef __linux__
>  	char filename[6 + MAX_STRLEN_TYPE(typeof(cmd->pid)) + 7 + 1];
>  	char status_line[256];
>  	FILE *status_file;
>  
> +	if (cmd->finished)
> +		return 1;
> +	if (cmd->pid <= 0)
> +		return 1;
> +
>  	/*
>  	 * Check by reading /proc/<pid>/status as calling waitpid causes
>  	 * stdout/stderr to be closed and data lost.
>  	 */
>  	sprintf(filename, "/proc/%u/status", cmd->pid);
> +#ifdef __linux__

[Severity: Low]
Does this cause unused variable warnings and execute unnecessary formatting on
non-Linux platforms? By moving the #ifdef __linux__ guard below the variable
declarations and the sprintf() call, it looks like status_line, status_file,
and filename will trigger warnings and execute useless code on non-Linux
builds.

>  	status_file = fopen(filename, "r");
>  	if (status_file == NULL) {
> -		/* Open failed assume finish_command was called. */
> -		return true;
> +		/*
> +		 * fopen() can fail with ENOENT if the process has been reaped.
> +		 * It can also fail with EMFILE/ENFILE if RLIMIT_NOFILE is reached,
> +		 * or with EINTR/ENOMEM. Use kill(pid, 0) as a robust fallback
> +		 * to distinguish between active processes and dead ones without
> +		 * consuming file descriptors.
> +		 */
> +		if (errno == ENOENT)
> +			return 1;
> +		if (kill(cmd->pid, 0) < 0 && errno == ESRCH) {

[Severity: Medium]
Will this loop infinitely if the process is a zombie? The kill() call
returns 0 for zombie processes, which would bypass this ESRCH check and return
0 at the bottom of the block, potentially tricking callers into polling
forever.

Additionally, if errno == ESRCH is true, doesn't that mean the original
process is already fully reaped? If so, the waitpid() call in the block below
seems like dead code that will only ever fail with ECHILD.

[Severity: Medium]
Could this sequence also introduce a time-of-check to time-of-use race?
If kill() returns ESRCH, the PID is free. If another concurrent thread forks
a new child and reuses this PID right before the waitpid() call, could we
accidentally reap the unrelated child process?

> +			waiting = waitpid(cmd->pid, &status, WNOHANG);

[Severity: Low]
Are the waiting and status variables declared anywhere in this function?
It looks like this might cause a compilation failure due to undeclared
identifiers.

> +			if (waiting == cmd->pid) {
> +				int result;
> +				int code;

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260531082235.940833-1-irogers@google.com?part=6