Re: [PATCH v2 11/14] perf test: Split monolithic 'util' test suite into sub-tests

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] The fallback logic in check_if_command_finished() deadlocks on zombie processes when /proc/pid/status fails to open.
- [High] Raw characters from test output are passed to the tolower() macro without being cast to unsigned char, causing undefined behavior and potential out-of-bounds reads.
--

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

perf test: Split monolithic 'util' test suite into sub-tests

This commit refactors the monolithic 'util' test suite into distinct 'String
replacement' and 'BLAKE2s hash' sub-tests using the struct test_case framework.
This improves test reporting granularity for subsequent JUnit XML reporting.

> diff --git a/tools/perf/tests/util.c b/tools/perf/tests/util.c
> index bf2c5b1338844..f9abd9911e6ca 100644
> --- a/tools/perf/tests/util.c
> +++ b/tools/perf/tests/util.c
[ ... ]

While reviewing this series, I noticed a couple of regressions introduced by
earlier patches in the series.

tools/lib/subcmd/run-command.c:check_if_command_finished() {
...
	status_file = fopen(filename, "r");
	if (status_file == NULL) {
...
		if (errno == ENOENT)
			return 1;
		if (kill(cmd->pid, 0) == -1 && errno == ESRCH)
			return 1;
		return 0;
	}
...
}

[Severity: High]
This isn't a bug introduced by this patch, but by a previous patch in the
series ('perf test: Refactor parallel poll loop to drain all pipes
simultaneously'). Does this fallback logic deadlock on zombie processes when
/proc/pid/status fails to open?

When running tests in parallel, file descriptors can be exhausted, causing
fopen() to fail with EMFILE.

The code falls back to kill(cmd->pid, 0), which succeeds (returns 0) for
zombie processes. Since check_if_command_finished() returns 0, the test
runner never calls finish_command() to reap the zombie via waitpid().

Will the zombie remain in the process table forever, causing the parent
finish_tests_parallel() loop to spin indefinitely in a deadlock?


tools/perf/tests/builtin-test.c:find_next_keyword() {
...
			for (i = 0; i < len; i++) {
				if (tolower(s[i]) != fail_keywords[k][i])
					break;
			}
...
}

[Severity: High]
This is a pre-existing issue, but it was introduced by a previous patch in
the series ('perf test: Show snippet failure output for verbose=1'). Are raw
characters from test output being passed to the tolower() macro without
being cast to unsigned char?

Characters from the raw stderr output buffer are passed directly to
tolower(s[i]).

On architectures where char is signed, characters with the high bit set (>= 128)
are sign-extended to negative integers. In glibc, tolower() uses the integer
directly as an index into a lookup array.

Can these negative values cause out-of-bounds memory reads, potentially crashing
the test runner when it attempts to parse arbitrary or localized child test
output?

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