Re: [PATCH v3 10/14] perf test: Skip shebang and SPDX comments in shell test descriptions

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Uninitialized stack memory access due to `readlink()` not null-terminating its output string.
- [High] Stack buffer overflow when constructing the script filename due to an unchecked `readlink()` return value.
- [High] `io__get_char()` implicitly sign-extends non-ASCII characters, causing premature EOF on architectures with signed chars.
--

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

perf test: Skip shebang and SPDX comments in shell test descriptions

When extracting shell test descriptions in tests-scripts.c, the parser
skipped the first line assuming it was the shebang (#!/bin/sh) and then read
the first comment line on line 2 as the test description.

However, checkpatch.pl expects shell scripts to declare their SPDX license
identifier on line 2 (# SPDX-License-Identifier: ...). This caused the test
harness to extract the SPDX license string as the test description.

Refactor shell_test__description to use io__getline, skipping both shebang
and SPDX comment lines. This allows shell tests to include standard SPDX
headers without breaking test suite description extraction.

> diff --git a/tools/perf/tests/tests-scripts.c b/tools/perf/tests/tests-scripts.c
> index f18c4cd337c8f..11ad1263ef85c 100644
> --- a/tools/perf/tests/tests-scripts.c
> +++ b/tools/perf/tests/tests-scripts.c
> @@ -51,7 +51,7 @@ static int shell_tests__dir_fd(void)
>  		return -1;
>  	/* Follow another level of symlink if there */
>  	if (lstat(path2, &st) == 0 && (st.st_mode & S_IFMT) == S_IFLNK) {
> -		scnprintf(path, sizeof(path), path2);
> +		scnprintf(path, sizeof(path), "%s", path2);

[Severity: High]
This isn't a bug introduced by this patch, but does this code read
uninitialized stack memory?

Looking at the preceding readlink() call:

	if (readlink("/proc/self/exe", path2, sizeof path2) < 0)
		return -1;

Since readlink() populates path2 but does not append a null terminator, path2
remains an uninitialized stack array. When lstat() and scnprintf() expect a
null-terminated string, won't this cause reads past the valid resolved path
until a random null byte is found?

[ ... ]
> @@ -78,43 +78,49 @@ static int shell_tests__dir_fd(void)
>  static char *shell_test__description(int dir_fd, const char *name)
>  {
>  	struct io io;
> -	char buf[128], desc[256];
> -	int ch, pos = 0;
> +	char buf[128], *line = NULL;
> +	size_t line_len = 0;
> +	ssize_t len;
> +	char *desc = NULL;
>  
>  	io__init(&io, openat(dir_fd, name, O_RDONLY), buf, sizeof(buf));
>  	if (io.fd < 0)
>  		return NULL;
>  
> -	/* Skip first line - should be #!/bin/bash Shebang */
> -	if (io__get_char(&io) != '#')
> -		goto err_out;
[ ... ]
> +	while ((len = io__getline(&io, &line, &line_len)) > 0) {

[Severity: High]
This isn't a bug introduced by this patch, but does io__get_char() implicitly
sign-extend non-ASCII characters?

io__getline() uses io__get_char() under the hood:

tools/lib/api/io.h:io__get_char() {
	...
	if (io->data == io->end) {
		int ret = io__fill_buffer(io);

		if (ret)
			return ret;
	}
	return *io->data++;
}

Since io__get_char() returns *io->data++ directly as an int, on architectures
where char is signed (like x86), wouldn't characters >= 0x80 (such as UTF-8
multibyte sequences) be sign-extended into negative integers?

Callers (such as io__getdelim()) use if (ch < 0) to check for EOF or errors.
Could any non-ASCII character in the parsed text incorrectly trigger an EOF
condition and truncate the read operation?

[ ... ]

While reviewing this file, I noticed another pre-existing issue in
append_script() in tools/perf/tests/tests-scripts.c:

tools/perf/tests/tests-scripts.c:append_script() {
	...
	len = readlink(link, filename, sizeof(filename));
	if (len < 0) {
		pr_err("Failed to readlink %s", link);
		return;
	}
	filename[len++] = '/';
	strcpy(&filename[len], name);
	...
}

[Severity: High]
This is a pre-existing issue, but is there a risk of a stack buffer overflow
when constructing the script filename here?

readlink() returns the number of bytes written to filename. If the resolved
path length is exactly PATH_MAX, len will equal sizeof(filename). Wouldn't
filename[len++] = '/' then write a byte past the end of the filename array on
the stack, with strcpy() continuing to write out-of-bounds?

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