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

sashiko-bot
2026-04-23T04:57:52Z

Sashiko AI review found 2 potential issue(s):
- [High] Unintended fallthrough to event processing loop when waitpid() is interrupted or fails.
- [Medium] Memory leak when parsing script descriptions containing both comments and description tags.
--

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

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

This commit removes the legacy gen-script option, updates the script
search to look directly in the python directory, and launches standalone
scripts using fork and execvp. It also cleans up references to scripting_ops.

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

Can this leak the memory allocated for the fallback description?

If the loop continues to search for a better description tag and finds one
later, it appears desc->half_liner is overwritten with a new strdup()
without freeing the previous allocation.

[ ... ]

> @@ -4253,149 +4235,54 @@ int cmd_script(int argc, const char **argv)
[ ... ]
> + } 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, such as EINTR, or
otherwise fails?

If the condition evaluates to false, execution will fall out of the if block
and continue. Does this unintentionally allow the parent process to keep
executing the rest of cmd_script() and process perf.data natively, instead
of returning an error or retrying the wait?

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