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

sashiko-bot
2026-04-25T18:24:40Z

Sashiko AI review found 3 potential issue(s):
- [High] Unhandled waitpid() failure causes unintended fallthrough to event processing loop.
- [Medium] Memory leak in read_script_info() when overwriting the fallback description.
- [Medium] Command-line filtering options for perf script are silently consumed and ignored when running standalone scripts.
--

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

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

This commit removes the legacy -g/--gen-script option and updates the
-s/--script option to imply running standalone scripts directly. It adds
support for launching these scripts via fork and execvp while bypassing
the normal event processing loop.

> diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c
> --- a/tools/perf/builtin-script.c
> +++ b/tools/perf/builtin-script.c
> @@ -3630,13 +3723,15 @@ static int read_script_info(struct script_desc *desc, const char *filename)
> if (!strncmp(p, "description:", strlen("description:"))) {
> p += strlen("description:");
> desc->half_liner = strdup(skip_spaces(p));
> - continue;
> + found_description = true;
> + break;
> }
>
> - if (!strncmp(p, "args:", strlen("args:"))) {
> - p += strlen("args:");
> - desc->args = strdup(skip_spaces(p));
> - continue;
> + if (!found_description && strlen(p) > 0 &&
> + strncmp(p, "SPDX-License-Identifier", 23)) {
> + desc->half_liner = strdup(p);
> + found_description = true;
> + // Don't break, maybe we find a better "description:" later!
> }

Does this leak the desc->half_liner string? If a script has a fallback line
that allocates memory here, and then a proper "description:" tag is found
later in the loop, it looks like desc->half_liner is overwritten without
freeing the original allocation.

[ ... ]

> @@ -4185,22 +4186,17 @@ int cmd_script(int argc, const char **argv)
> OPTS_EVSWITCH(&script.evswitch),
> OPT_END()
> };
[ ... ]
> - argc = parse_options_subcommand(argc, argv, options, script_subcommands, script_usage,
> + argc = parse_options_subcommand(argc, argv, options, NULL, script_usage,
> PARSE_OPT_STOP_AT_NON_OPTION);

[ ... ]

> @@ -4253,149 +4235,54 @@ int cmd_script(int argc, const char **argv)
[ ... ]
> 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 + 4, sizeof(const char *));
> j = 0;
[ ... ]
> + for (i = 0; i < argc; i++)
> __argv[j++] = argv[i];
> __argv[j++] = NULL;
>
> - execvp("/bin/sh", (char **)__argv);
> + execvp(script_name, (char **)__argv);

Since parse_options_subcommand() was already called earlier in this function,
are the parsed command-line options (like --cpu or --time) getting silently
discarded?

When running a standalone script, the parent process skips the perf_session
event processing loop where these options are normally applied. Because argc
and argv are already stripped of these options, the child script won't see
them either, which might lead to filtering options being ignored without
notifying the user.

> + pr_err("failed to execute script '%s': %s\n", script_name, strerror(errno));
> + exit(-errno);
> + } else { /* parent */
> + int status = 0;
> +
> + if (waitpid(pid, &status, 0) == pid) {
> + if (WIFEXITED(status))
> + return WEXITSTATUS(status);
> + else
> + return -1;
> }
> }

What happens if waitpid() is interrupted by a signal, like a terminal resize
causing an EINTR error?

If waitpid() returns -1, the condition evaluates to false and the function
continues execution instead of returning. This looks like it would
inadvertently fall through to initialize a perf_session and process or dump
the rest of the data file while the script is still running.

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