Re: [PATCH 09/15] perf test: Add summary reporting

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The patch completely removes the restoration of the signal mask in `strbuf_addstr_safe()`, causing SIGINT and SIGTERM to be permanently blocked.
- [Medium] Unprotected `vsnprintf()`, `malloc()`, and `free()` calls in the newly introduced `strbuf_addf_safe()` function are vulnerable to heap corruption and deadlocks via `siglongjmp()`.
--

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

perf test: Add summary reporting

This commit introduces an automated execution summary printed at the end
of a perf test run. It tracks pass, skip, and fail counts, and accumulates
descriptions of failed test cases for easy cross-referencing.

> diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c
> index 6d7b4a952e909..5d319b9da7624 100644
> --- a/tools/perf/tests/builtin-test.c
> +++ b/tools/perf/tests/builtin-test.c

[ ... ]

> @@ -728,10 +748,36 @@ static int strbuf_addstr_safe(struct strbuf *sb, const char *s)
>  	sigaddset(&set, SIGTERM);
>  	sigprocmask(SIG_BLOCK, &set, &oldset);
>  	ret = strbuf_addstr(sb, s);
> -	sigprocmask(SIG_SETMASK, &oldset, NULL);

[Severity: Medium]
Does the removal of this sigprocmask() permanently block SIGINT and SIGTERM?

Because strbuf_addstr_safe() is invoked routinely during normal test execution,
such as via drain_child_process_err(), it seems the very first invocation will
permanently block these signals since the oldset is never restored. This would
prevent users from interrupting a hanging perf test run.

>  	return ret;
>  }
>  
> +static int __printf(2, 3) strbuf_addf_safe(struct strbuf *sb, const char *fmt, ...)
> +{
> +	char buf[1024];
> +	va_list ap;
> +	int len;
> +
> +	va_start(ap, fmt);
> +	len = vsnprintf(buf, sizeof(buf), fmt, ap);
> +	va_end(ap);
> +
> +	if (len < 0)
> +		return len;
> +	if ((size_t)len >= sizeof(buf)) {
> +		char *dynamic_buf = malloc(len + 1);

[Severity: Medium]
Are these calls to vsnprintf(), malloc(), and free() vulnerable to deadlocks
if interrupted by the signal handler?

The perf test harness uses a custom signal handler that executes siglongjmp()
to abort hanging runs. If the signal handler interrupts non-async-signal-safe
functions like vsnprintf() or malloc() here before signals are masked, their
internal mutexes could be left permanently locked.

The subsequent error recovery path performs multiple free() operations that
could deadlock trying to acquire the same locks. Should signals be blocked
during these operations, similar to how they are handled in
strbuf_addstr_safe()?

> +		int ret;
> +		if (!dynamic_buf)
> +			return -ENOMEM;
> +		va_start(ap, fmt);
> +		vsnprintf(dynamic_buf, len + 1, fmt, ap);
> +		va_end(ap);
> +		ret = strbuf_addstr_safe(sb, dynamic_buf);
> +		free(dynamic_buf);
> +		return ret;
> +	}
> +	return strbuf_addstr_safe(sb, buf);
> +}

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