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

sashiko-bot
2026-04-25T23:23:39Z

Sashiko AI review found 3 potential issue(s):
- [High] Unhandled `waitpid()` failure causes unintended execution of the native `perf script` data processing loop.
- [Low] Overwriting `desc->half_liner` without freeing the previous allocation causes a memory leak.
- [Low] The script `args` formatting is preserved in `list_available_scripts` but the population logic is removed, leaving dead code.
--

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

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

This commit refactors perf script to support standalone Python scripts, removing
legacy scripting options. It updates script path resolution and adds the ability
to launch scripts directly via fork and execvp.

> 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!
> }
> }

Is there a memory leak here if we find a better description later?

If desc->half_liner is assigned from the SPDX-License-Identifier line,
and the loop continues, finding the actual description: tag will overwrite
desc->half_liner with a new strdup() without freeing the initial allocation.

Also, since the args: parsing was removed here, should the corresponding
printing logic in list_available_scripts() be removed as well?

[ ... ]

> @@ -3695,26 +3789,24 @@ static int list_available_scripts(const struct option *opt __maybe_unused,
> exit(-1);
> }
>
[ ... ]
> - fprintf(stdout, "List of available trace scripts:\n");
> + fprintf(stdout, "List of available scripts:\n");
> list_for_each_entry(desc, &script_descs, node) {
> sprintf(first_half, "%s %s", desc->name,
> desc->args ? desc->args : "");

This isn't a bug, but this logic still checks and prints desc->args. Since
the parsing for it was removed above, desc->args will always be NULL.

[ ... ]

> @@ -4253,149 +4235,54 @@ int cmd_script(int argc, const char **argv)
> /* make sure PERF_EXEC_PATH is set for scripts */
> set_argv_exec_path(get_argv_exec_path());
>
[ ... ]
> + 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 */
[ ... ]
> + execvp(script_name, (char **)__argv);
> + 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;
> }
> }
> -
[ ... ]
> }
>
> if (dlfilter_file) {

If waitpid() is interrupted by a signal and returns -1 (with errno == EINTR),
or fails for another reason, the condition waitpid(pid, &status, 0) == pid
evaluates to false.

Since there is no else clause or return statement on the failure path,
execution will fall through the if (script_name) block and proceed to the rest
of cmd_script(). This could unintentionally initialize a perf session and dump
perf.data to stdout instead of terminating.

Should waitpid() be wrapped in a retry loop for EINTR, and return an error if
it ultimately fails?

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