Re: [PATCH v7 4/7] perf test: Introduce storing logs for shell tests

[sashiko-bot@kernel.org]

Sashiko AI review found 6 potential issue(s):
- [High] Returning 1 from the `nftw` callback to enforce a depth limit incorrectly aborts the entire directory traversal, resulting in permanent temporary file leaks.
- [High] The directory filtering logic in `delete_file` fails to protect the contents of unknown directories and causes parent directories to be leaked.
- [High] The size calculation for the log directory array `tmpdir` is vulnerable to integer overflow on 32-bit systems, leading to a heap buffer overflow.
- [Medium] The `PERFSUITE_RUN_DIR` environment variable is leaked to subsequent test suites that do not request logs.
- [Medium] Test suites are executed even when log directory creation fails, causing them to use a stale environment.
- [Medium] Using unsanitized test suite descriptions in the `mkdtemp` template causes predictable directory creation failures or shell variable escaping issues.
--

commit d3121febf07f1014f3c0ec5faf10998a2927559e
Author: Jakub Brnak <jbrnak@redhat.com>

perf test: Introduce storing logs for shell tests

Create temporary directories for storing log files for shell tests
that could help while debugging. If the variable PERFTEST_KEEP_LOGS
is set keep the logs, else delete them.

> diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c
> index 76fc37b440be4..306ecfe5e22ff 100644
> --- a/tools/perf/tests/builtin-test.c
> +++ b/tools/perf/tests/builtin-test.c

[ ... ]

> @@ -288,6 +292,91 @@ static bool test_exclusive(const struct test_suite *t, int test_case)
>  	return t->test_cases[test_case].exclusive;
>  }
>  
> +static int delete_file(const char *fpath, const struct stat *sb __maybe_unused,
> +		       int typeflag, struct FTW *ftwbuf)
> +{
> +	/* Stop traversal if going too deep */
> +	if (ftwbuf->level > 5) {
> +		pr_err("Tree traversal reached level %d, stopping.", ftwbuf->level);
> +		return 1;
> +	}

Does returning 1 from the nftw callback incorrectly abort the entire
directory traversal? Per POSIX specifications, returning a non-zero
value stops nftw completely. This could abandon all remaining unvisited
files and directories within the test's log directory and potentially
lead to /tmp exhaustion.

> +
> +	/* Remove only expected directories */
> +	if (typeflag == FTW_D || typeflag == FTW_DP) {
> +		const char *dirname = fpath + ftwbuf->base;
> +
> +		if (strcmp(dirname, "logs") && strcmp(dirname, "examples") &&
> +		    strcmp(dirname, "header_tar") && strncmp(dirname, "perf_", 5)) {
> +			pr_err("Unknown directory %s", dirname);
> +			return 0;
> +		}
> +	}
> +
> +	/* Attempt to remove the file, continue on failure */
> +	if (remove(fpath))
> +		pr_err("Failed to remove file: %s", fpath);
> +
> +	return 0;
> +}

Because nftw is called with FTW_DEPTH (post-order traversal), files inside
the directory are visited first. For regular files (FTW_F), this directory
name check is bypassed, and remove() unconditionally deletes them.

Are all files within the protected directory deleted before the directory
itself is evaluated? 

Also, because the now-empty unknown directory is not deleted, would the
parent directory's subsequent remove() fail with ENOTEMPTY, leaking the
directory structure?

[ ... ]

> +static char *setup_shell_logs(const char *name)
> +{
> +	char template[PATH_MAX];
> +	char *temp_dir;
> +
> +	if (snprintf(template, PATH_MAX, "/tmp/perf_test_%s.XXXXXX", name) >= PATH_MAX) {
> +		pr_err("Failed to create log dir template");
> +		return NULL; /* Skip the testsuite */
> +	}
> +
> +	temp_dir = mkdtemp(template);

Can the test suite description string contain slashes or spaces? Slashes
would cause mkdtemp to fail with ENOENT because intermediate directories do
not exist. Spaces could cause issues when the unquoted PERFSUITE_RUN_DIR
is used in shell scripts.

I see this issue is addressed later in the series in commit
deaef0503b790e27361344881ded63d280631c12.

> +	if (temp_dir) {
> +		setenv("PERFSUITE_RUN_DIR", temp_dir, 1);
> +		return strdup(temp_dir);
> +	}
> +
> +	pr_err("Failed to create the temporary directory");
> +
> +	return NULL; /* Skip the testsuite */
> +}
> +
> +static void cleanup_shell_logs(char *dirname)
> +{
> +	char *keep_logs = getenv("PERFTEST_KEEP_LOGS");
> +
> +	/* Check if logs should be kept or do cleanup */
> +	if (dirname) {
> +		if (!keep_logs || strcmp(keep_logs, "y") != 0)
> +			nftw(dirname, delete_file, 8, FTW_DEPTH | FTW_PHYS);
> +		free(dirname);
> +	}
> +
> +	unsetenv("PERFSUITE_RUN_DIR");
> +}

Is the PERFSUITE_RUN_DIR environment variable leaked to subsequent test
suites that do not request logs? The unsetenv() is only called during
cleanup_shell_logs(). If a test does not request logs, could it inherit
the value pointing to the previous test's directory?

[ ... ]

> @@ -627,9 +716,19 @@ static int __cmd_test(struct test_suite **suites, int argc, const char *argv[],
>  	 * runs the exclusive tests sequentially. In other modes all tests are
>  	 * run in pass 1.
>  	 */
> +
>  	for (int pass = 1; pass <= 2; pass++) {
>  		int child_test_num = 0;
>  		int curr_suite = 0;
> +		size_t tmpdir_sz = num_logged_suites * runs_per_test;
> +		char **tmpdir = tmpdir_sz ? calloc(tmpdir_sz, sizeof(*tmpdir)) : NULL;

Is there an integer overflow risk on 32-bit systems here? The runs_per_test
variable is controlled via a command-line argument. Providing a large value
could cause the multiplication to wrap around, resulting in calloc allocating
a small buffer, which may lead to an out-of-bounds heap write during the
loop.

[ ... ]

>  			for (unsigned int run = 0; run < runs_per_test; run++) {
> +				/* Setup temporary log directories for shell test suites */
> +				if (create_logs(*t, pass)) {
> +					struct shell_test_info *info = (*t)->priv;
> +
> +					tmpdir[logged_suites] = setup_shell_logs((*t)->desc);
> +					/* Couldn't create log dir, skip test suite */
> +					if (tmpdir[logged_suites] == NULL) {
> +						if (info->has_setup != NO_SETUP)
> +							info->has_setup = FAILED_SETUP;
> +					} else {
> +						logged_suites++;
> +						if (info->has_setup == PASSED_SETUP ||
> +						    info->has_setup == FAILED_SETUP)
> +							info->has_setup = RUN_SETUP;
> +					}
> +				}
> +
>  				test_suite__for_each_test_case(*t, curr_test_case) {

The comment says to skip the test suite, but there is no continue statement
here. Are the test cases still executed without the intended logging setup
if setup_shell_logs() returns NULL?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260416111419.385010-1-jbrnak@redhat.com?part=4