Re: [PATCH v3 08/14] perf test: Add summary reporting

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The emergency signal cleanup path is vulnerable to heap corruption and deadlocks due to non-async-signal-safe string buffer operations. `print_tests_summary()` is explicitly called on the signal abort path (`err_out`) and both reads from `summary_failed_tests_buf` and calls `strbuf_release()` on it. Since `strbuf_addf()` allocates memory using the heap lock during normal execution, a signal (e.g., SIGINT) interrupting it will cause `siglongjmp()` to leave the heap lock held. Calling `strbuf_release()` will then re-enter `free()`, attempting to acquire the same heap lock, causing an irreversible deadlock. Additionally, the buffer may be left in an inconsistent state, leading to a crash when its contents are read.
--

commit 15adcea6916685e103e56321c61c30c9432f83d1
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 the pass, skip, and fail counts, along with
descriptions of failed tests accumulated in a string buffer. The summary is
printed just before freeing the test array, ensuring it is displayed even
if an internal signal cleanup occurs.

> diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c
> index a29925656865b..bd356249c70ba 100644
> --- a/tools/perf/tests/builtin-test.c
> +++ b/tools/perf/tests/builtin-test.c
> @@ -393,6 +403,15 @@ static int print_test_result(struct test_suite *t, int curr_suite, int curr_test
>  		break;
>  	case TEST_FAIL:
>  	default:
> +		summary_tests_failed++;
> +		if (test_suite__num_test_cases(t) > 1)
> +			strbuf_addf(&summary_failed_tests_buf, "  %3d.%1d: %s\n",
> +				    curr_suite + 1, curr_test_case + 1,
> +				    test_description(t, curr_test_case));
> +		else
> +			strbuf_addf(&summary_failed_tests_buf, "  %3d: %s\n",
> +				    curr_suite + 1,
> +				    test_description(t, curr_test_case));

[Severity: Medium]
If a signal like SIGINT interrupts execution while strbuf_addf() is dynamically
allocating memory (and holding the heap lock), will siglongjmp() leave the
heap lock held?

>  		color_fprintf(stderr, PERF_COLOR_RED, " FAILED!\n");
>  		break;
>  	}
> @@ -975,6 +994,23 @@ static void cmd_test_sig_handler(int sig)
[ ... ]
> +static void print_tests_summary(void)
> +{
[ ... ]
> +	if (summary_tests_failed > 0) {
> +		color_fprintf(stderr, PERF_COLOR_RED, "Failed tests      : %u\n",
> +			      summary_tests_failed);
> +		pr_info("List of failed tests:\n");
> +		pr_info("%s", summary_failed_tests_buf.buf);
> +	} else {
> +		color_fprintf(stderr, PERF_COLOR_GREEN, "Failed tests      : 0\n");
> +	}
> +	strbuf_release(&summary_failed_tests_buf);

[Severity: Medium]
Can reading from summary_failed_tests_buf and calling strbuf_release() cause
a crash or deadlock if called after a signal interrupt?

If the signal interrupted strbuf_addf(), the buffer might be left in an
inconsistent state causing a crash when read.

Similarly, strbuf_release() calls free(), which could attempt to acquire
the heap lock that was already held when the signal fired, resulting in a
deadlock.

> +}
> +
>  static int __cmd_test(struct test_suite **suites, int argc, const char *argv[],
>  		      struct intlist *skiplist)
>  {
[ ... ]
> @@ -1087,6 +1126,7 @@ static int __cmd_test(struct test_suite **suites, int argc, const char *argv[],
>  		for (size_t x = 0; x < num_tests; x++)
>  			finish_test(child_tests, x, num_tests, width);
>  	}
> +	print_tests_summary();

[Severity: Medium]
Does invoking print_tests_summary() here on the emergency signal abort path
make the tool vulnerable to the deadlock and crash mentioned above?

>  	free(global_pfds);
>  	free(global_pfd_indices);
>  	global_pfds = NULL;

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