Re: [PATCH v1 56/58] perf script: Refactor to support standalone scripts and remove legacy features

sashiko-bot
2026-04-20T01:01:00Z

Sashiko AI review found 7 potential issue(s):
- [High] Stack buffer overflow in find_script due to incorrect strncat size argument
- [High] Uninitialized status variable evaluated due to missing EINTR check on waitpid
- [High] execvp bypasses the verified local script by searching $PATH
- [High] Standalone scripts lose parsed command-line options like input file
- [High] The 'record' and 'report' subcommands are broken because they are intercepted but not implemented
- [Medium] Premature loop termination completely breaks script description parsing
- [Medium] Silent failure when standalone scripts fail to execute
--

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

perf script: Refactor to support standalone scripts and remove legacy features

This commit refactors the perf script command to support running standalone
scripts directly via fork and execvp. It also removes legacy scripting
features, updating the script search path to look directly in the python
directory.

> diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c
> index c0949556d1bbb..8d8bfbd321f42 100644
> --- a/tools/perf/builtin-script.c
> +++ b/tools/perf/builtin-script.c
[ ... ]
> /* Find script file relative to current directory or exec path */
> static char *find_script(const char *script)
> {
> char path[PATH_MAX];
> + char *exec_path;
>
> - if (!scripting_ops) {
> - const char *ext = strrchr(script, '.');
> + if (access(script, R_OK) == 0)
> + goto found;
>
[ ... ]
> + exec_path = get_argv_exec_path();
> + if (!exec_path)
> + return NULL;
>
> + snprintf(path, sizeof(path), "%s/python/%s", exec_path, script);
> + free(exec_path);
> + script = path;
>
> + if (access(path, R_OK) == 0)
> + goto found;
>
> + /* Try with .py suffix. */
> + strncat(path, ".py", sizeof(path) - 1);

Does this code overflow path[]?

The third argument to strncat specifies the maximum number of characters to
append from the source string, not the remaining capacity of the destination
buffer. Because path is populated by snprintf right before, if the string
length reaches PATH_MAX - 1, strncat will blindly append 3 characters plus a
null terminator out of bounds, overwriting stack memory.

> +
> + if (access(script, R_OK) == 0)
> + goto found;
> +
> + /* Failure to find script. */
> + return NULL;
> +
> +found:
> return strdup(script);
> }
[ ... ]
> @@ -3608,15 +3651,59 @@ static int read_script_info(struct script_desc *desc, const char *filename)
> {
> char line[BUFSIZ], *p;
> FILE *fp;
> + bool in_docstring = false;
> + bool found_description = false;
>
> fp = fopen(filename, "r");
> if (!fp)
> return -1;
>
> while (fgets(line, sizeof(line), fp)) {
> + static const char * const triple_quote_str[] = {
> + "\"\"\"",
> + "'''",
> + "r\"\"\"",
> + };
> p = skip_spaces(line);
> if (strlen(p) == 0)
> continue;
> +
> + if (in_docstring) {
> + if (strlen(p) && p[strlen(p) - 1] == '\n')
> + p[strlen(p) - 1] = '\0';
> + desc->half_liner = strdup(skip_spaces(p));
> + in_docstring = false;
> + found_description = true;
> + break;
> + }
> +
> +
> + for (size_t i = 0; i < ARRAY_SIZE(triple_quote_str); i++) {
> + const char *quote = triple_quote_str[i];
> +
> + if (!strstarts(p, quote))
> + continue;
> +
> + p += strlen(quote);
> + p = skip_spaces(p);
> + if (strlen(p) > 0) {
> + if (p[strlen(p) - 1] == '\n')
> + p[strlen(p) - 1] = '\0';
> + p = skip_spaces(p);
> + if (str_ends_with(p, quote))
> + p[strlen(p) - strlen(quote)] = '\0';
> + desc->half_liner = strdup(skip_spaces(p));
> + found_description = true;
> + break;
> + }
> + in_docstring = true;
> + break;
> + }
> + if (found_description)
> + break;

Is it possible this loop terminates prematurely?

If a comment line is an SPDX identifier, it is stored as a fallback description
and found_description is set to true below. However, at the top of the fgets
loop (right after checking triple_quote_str), this check will cause the loop
to terminate immediately on the next iteration. This entirely defeats the
comment further down that suggests it should keep looking for a better
description: tag.

> + if (in_docstring)
> + continue;
> +
[ ... ]
> @@ -4198,7 +4193,6 @@ int cmd_script(int argc, const char **argv)
>
> perf_set_singlethreaded();
>
> - setup_scripting();
>
> argc = parse_options_subcommand(argc, argv, options, script_subcommands, script_usage,
> PARSE_OPT_STOP_AT_NON_OPTION);

If "record" and "report" are removed from cmd_script, shouldn't they be
removed from the script_subcommands array as well?

Because "record" and "report" are left in the script_subcommands array,
parse_options_subcommand intercepts these as valid subcommands, leaving
argv[0] as "record". The logic then attempts to find and execute a script
literally named "record", failing with "Couldn't find script 'record'" and
breaking the documented subcommand functionality.

Also, since parse_options_subcommand consumes global options (like -i),
wouldn't the standalone scripts lose these parsed command-line options?
The remaining arguments are passed to the standalone script via execvp,
but the standalone script never receives the -i flag and might fall
back to default files, silently breaking the user's intent to process a
specific input file.

[ ... ]
> + if (script_name) {
> + pid_t pid = fork();
> if (pid < 0) {
> - perror("failed to fork");
> - return -1;
> + pr_err("failed to fork\n");
> + return -errno;
> }
> -
> - if (!pid) {
> + if (pid == 0) { /* child */
> + __argv = calloc(argc + 2, sizeof(const char *));
> j = 0;
[ ... ]
> if (!__argv) {
> + exit(-ENOMEM);
> }
> -
> - __argv[j++] = "/bin/sh";
> - __argv[j++] = rec_script_path;
[ ... ]
> + __argv[j++] = script_name;
> + for (i = 0; i < argc; i++)
> __argv[j++] = argv[i];
> __argv[j++] = NULL;
>
> - execvp("/bin/sh", (char **)__argv);
> - free(__argv);
> - exit(-1);
> - }
> -
[ ... ]
> + execvp(script_name, (char **)__argv);

If a user specifies a script without a slash (e.g. myscript.py), find_script
verifies its local existence using access(script, R_OK) and returns the string
unmodified. However, execvp() searches the system $PATH. If . is not in $PATH,
won't execvp either fail with ENOENT or execute a completely different
myscript.py located elsewhere in the system, bypassing the verified local file?

Additionally, if execvp fails to launch a script (e.g. because the script lacks
execute permissions or a valid shebang, which were not required by the previous
embedded interpreter), does it silently fail? The child process seems to
execute exit(-errno) without printing any error message to stderr, leaving
the user with a generic non-zero exit code and no diagnostic information.

> + exit(-errno);
> + } else { /* parent */
> + int status;
>
> - j = 0;
[ ... ]
> + waitpid(pid, &status, 0);

Does this code evaluate an uninitialized status variable?

If waitpid is interrupted by a signal (e.g. SIGINT), it returns -1 and sets
errno to EINTR, leaving the stack variable status uninitialized. The code
then immediately evaluates WIFEXITED(status), reading garbage memory. This
undefined behavior can cause perf script to prematurely return an arbitrary
exit code, abandoning the standalone script process in the background.

> + if (WIFEXITED(status)) {
> + return WEXITSTATUS(status);
> + } else {
> + return -1;
> }
> }
> -

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