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

sashiko-bot
2026-04-24T17:38:07Z

Sashiko AI review found 2 potential issue(s):
- [High] Unhandled waitpid() failure causes fall-through to unintended trace event processing.
- [Medium] Memory leak in read_script_info() due to un-freed string allocations when parsing descriptions.
--

commit a9d5ccc903f5f6a0818669d88a426ab7c03198db
Author: Ian Rogers <irogers@google.com>
Subject: perf script: Refactor to support standalone scripts and remove legacy features

This commit refactors perf script to support standalone Python scripts,
removing legacy scripting features and options. It allows launching scripts
directly via fork and execvp while skipping the event processing loop.

> --- a/tools/perf/builtin-script.c
> +++ b/tools/perf/builtin-script.c
[ ... ]
> @@ -3608,15 +3659,57 @@ static int read_script_info(struct script_desc *desc, const char *filename)
[ ... ]
> + 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 (!strncmp(p, "description:", strlen("description:"))) {
> p += strlen("description:");
> desc->half_liner = strdup(skip_spaces(p));
> + found_description = true;
> + break;
> }
>
> + 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!
> }

If a script contains multiple description-like elements, such as a fallback
SPDX comment followed by an explicit "description:" line, does this code leak
the memory allocated for desc->half_liner?

Since the outer loop continues when "SPDX-License-Identifier" is matched
without breaking, a subsequent match on "description:" will overwrite the
desc->half_liner pointer without freeing the previously allocated string.

[ ... ]
> @@ -4253,149 +4235,54 @@ int cmd_script(int argc, const char **argv)
[ ... ]
> + if (script_name) {
> + pid_t pid = fork();
> if (pid < 0) {
> + pr_err("failed to fork\n");
> + return -errno;
> }
> + 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) {

What happens if waitpid() fails or is interrupted by a signal, returning -1
with errno set to EINTR?

Because there is no else branch or early return on failure, would execution
fall out of the waitpid() block and continue to the rest of cmd_script()?

If this fall-through occurs, it looks like the parent process would incorrectly
initialize a session and begin parsing and dumping trace events concurrently
with the child script, rather than safely exiting.

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