Re: [PATCH 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 3 potential issue(s) to consider:

Pre-existing issues:
- [High] Incomplete Fix / Process Reaping Hazard via waitpid(-1)
- [High] Unprotected State Modification / Process Reaping Hazard via waitpid(0)
- [High] Uninitialized Thread Join in tpebs_stop
--

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

tools subcmd: Robust fallback and existence checks for process reaping

This commit updates check_if_command_finished() to handle cases where
PID is 0 or -1. It also adds a waitpid fallback when fopen fails due to
file descriptor limits.

> diff --git a/tools/lib/subcmd/run-command.c b/tools/lib/subcmd/run-command.c
> index b7510f83209a0..5ae7b9fa4daba 100644
> --- a/tools/lib/subcmd/run-command.c
> +++ b/tools/lib/subcmd/run-command.c
> @@ -233,7 +233,14 @@ int check_if_command_finished(struct child_process *cmd)
>  	char filename[6 + MAX_STRLEN_TYPE(typeof(cmd->pid)) + 7 + 1];
>  	char status_line[256];
>  	FILE *status_file;
> +#endif
> +
> +	if (cmd->finished)
> +		return 1;
> +	if (cmd->pid <= 0)
> +		return 1;
>  

[Severity: High]
This isn't a bug introduced by this patch, but by returning early without
setting cmd->finished = 1, does this code still leave the internal state
unchanged?

When error-handling paths like the err_out cleanup loop in __cmd_test()
invoke finish_command() on child structures with pid == -1:

tools/perf/tests/builtin-test.c:finish_test() {
    ...
    ret = finish_command(&child_test->process);
    ...
}

This calls wait_or_whine(), which sees cmd->finished is still 0 and
proceeds to execute waitpid(-1, ...):

tools/lib/subcmd/run-command.c:wait_or_whine() {
    ...
    pid_t waiting = waitpid(cmd->pid, &status, block ? 0 : WNOHANG);
    ...
}

Could this execute a wildcard wait for any child process, potentially
stealing the exit status of concurrent parallel tests?

[ ... ]

[Severity: High]
This is a pre-existing issue, but does the lack of validation for
non-positive PIDs also expose a hazard in tpebs_stop()?

In tpebs_stop(), the code zeroes tpebs_cmd.pid before calling
finish_command():

tools/perf/util/intel-tpebs.c:tpebs_stop() {
    ...
    tpebs_cmd.pid = 0;
    ...
    ret = finish_command(&tpebs_cmd);
    ...
}

Since cmd->finished is not checked or set to true before finish_command()
is called, wait_or_whine() will execute waitpid(0, ...):

tools/lib/subcmd/run-command.c:wait_or_whine() {
    ...
    pid_t waiting = waitpid(cmd->pid, &status, block ? 0 : WNOHANG);
    ...
}

Will this wildcard wait instruct the kernel to reap any child process in
the current process group, inadvertently consuming the exit statuses of
unrelated child tasks?

[Severity: High]
This is a pre-existing issue, but does leaving tpebs_cmd.pid as -1 lead
to an uninitialized thread join?

In evsel__tpebs_open(), if evsel__tpebs_start_perf_record() fails:

tools/perf/util/intel-tpebs.c:evsel__tpebs_open() {
    ...
    ret = evsel__tpebs_start_perf_record(evsel);
    if (ret)
        goto out;
    ...
}

This leaves tpebs_cmd.pid as -1 and aborts before initializing the thread.
Later, when evsel__tpebs_close() calls tpebs_stop(), the condition
if (tpebs_cmd.pid != 0) evaluates to true.

The code then erroneously calls pthread_join() on the uninitialized thread:

tools/perf/util/intel-tpebs.c:tpebs_stop() {
    ...
    pthread_join(tpebs_reader_thread, NULL);
    ...
}

Since tpebs_reader_thread is an uninitialized static variable, can passing
it to pthread_join() cause undefined behavior or a segmentation fault?

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