Re: [PATCH v7 1/7] perf tests: Create a structure for shell tests

sashiko-bot
2026-04-16T11:38:48Z

Sashiko AI review found 3 potential issue(s):
- [High] Stack buffer overflow when constructing file paths with readlink().
- [High] Unsanitized file paths are passed to system(), leading to test failures and arbitrary command execution.
- [Medium] Inconsistent symlink traversal in `is_directory_fd` can cause infinite recursion.
--

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

perf tests: Create a structure for shell tests

This commit implements a new structure for shell test suites, grouping them
by subdirectories within perf/tests/shell instead of placing them all into a
single list.

> diff --git a/tools/perf/tests/tests-scripts.c b/tools/perf/tests/tests-scripts.c
> index f18c4cd337c8f..680dfe650f146 100644
> --- a/tools/perf/tests/tests-scripts.c
> +++ b/tools/perf/tests/tests-scripts.c
> @@ -151,14 +200,64 @@ static char *strdup_check(const char *str)
> return newstr;
> }
>
> -static int shell_test__run(struct test_suite *test, int subtest __maybe_unused)
> +/* Check if dirent entry is a directory using file descriptor, handling DT_UNKNOWN case */
> +static bool is_directory_fd(int dir_fd, struct dirent *ent)
> {
> - const char *file = test->priv;
> + struct stat st;
> +
> + if (ent->d_type == DT_DIR)
> + return true;
> +
> + if (ent->d_type != DT_UNKNOWN)
> + return false;
> +
> + /* Need to stat the file to determine if it's a directory */
> + if (fstatat(dir_fd, ent->d_name, &st, 0) != 0)
> + return false;

Could this cause inconsistent symlink traversal depending on the filesystem?
If the filesystem returns DT_UNKNOWN, fstatat() without the AT_SYMLINK_NOFOLLOW
flag will follow symlinks and report them as directories. This differs from the
fast path which correctly skips symlinks because their d_type is DT_LNK.
Could this lead to infinite recursion if a symlink loops back to a parent
directory?

> +
> + return S_ISDIR(st.st_mode);
> +}

[ ... ]

> +static int shell_test__run(struct test_suite *test, int subtest)
> +{
> + const char *file;
> int err;
> char *cmd = NULL;
>
> + /* Get absolute file path */
> + if (subtest >= 0)
> + file = test->test_cases[subtest].name;
> + else /* Single test case */
> + file = test->test_cases[0].name;
> +
> if (asprintf(&cmd, "%s%s", file, verbose ? " -v" : "") < 0)
> return TEST_FAIL;
> +
> err = system(cmd);

Since file contains the absolute path to the test script, what happens if the
kernel source tree is located in a directory path containing spaces or shell
metacharacters? Will system() fail to execute the path properly?

[ ... ]

> @@ -167,65 +266,117 @@ static int shell_test__run(struct test_suite *test, int subtest __maybe_unused)
> return WEXITSTATUS(err) == 2 ? TEST_SKIP : TEST_FAIL;
> }
>
> -static void append_script(int dir_fd, const char *name, char *desc,
> - struct test_suite ***result,
> - size_t *result_sz)
> +static struct test_suite *prepare_test_suite(int dir_fd)
> {
> - char filename[PATH_MAX], link[128];
> - struct test_suite *test_suite, **result_tmp;
> - struct test_case *tests;
> + char dirpath[PATH_MAX], link[128];
> ssize_t len;
> - char *exclusive;
> + struct test_suite *test_suite = NULL;
> + struct shell_test_info *test_info;
>
> + /* Get dir absolute path */
> snprintf(link, sizeof(link), "/proc/%d/fd/%d", getpid(), dir_fd);
> - len = readlink(link, filename, sizeof(filename));
> + len = readlink(link, dirpath, sizeof(dirpath));
> if (len < 0) {
> pr_err("Failed to readlink %s", link);
> - return;
> + return NULL;
> }
> - filename[len++] = '/';
> - strcpy(&filename[len], name);
> + dirpath[len++] = '/';
> + dirpath[len] = '\0';

If readlink() fills the entire dirpath array, returning a length of PATH_MAX,
won't these assignments write out of bounds and corrupt the stack?

[ ... ]

> +static void append_script_to_suite(int dir_fd, const char *name, char *desc,
> + struct test_suite *test_suite, size_t *tc_count)
> +{
> + char file_name[PATH_MAX], link[128];
> + struct test_case *tests;
> + ssize_t len;
> + char *exclusive;

[ ... ]

> + /* Get path to the test script */
> + snprintf(link, sizeof(link), "/proc/%d/fd/%d", getpid(), dir_fd);
> + len = readlink(link, file_name, sizeof(file_name));
> + if (len < 0) {
> + pr_err("Failed to readlink %s", link);
> + free(desc);
> + return;
> + }
> + file_name[len++] = '/';
> + strcpy(&file_name[len], name);

Similarly here, if readlink() returns PATH_MAX, file_name[len++] will write past
the end of the array. Also, won't strcpy() overflow the buffer if there isn't
enough space left in file_name to hold the script name?

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