Re: [PATCH v4 07/15] perf test: Refactor parallel poll loop to drain all pipes simultaneously

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] A double-free vulnerability exists on `global_pfds` and `global_pfd_indices` if an asynchronous signal (e.g., SIGINT) interrupts the execution of `finish_tests_parallel()`.
- [Medium] If the test harness is interrupted by a signal, any stderr output already accumulated for running tests is silently dropped and never printed.
--

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

perf test: Refactor parallel poll loop to drain all pipes simultaneously

When running tests in parallel with verbose output, child processes fill up
pipe buffers and block. This patch implements finish_tests_parallel() to
poll and drain all pipes simultaneously, avoiding the block and drastically
speeding up parallel testing.

> diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c
> index 2ccb52a776ccb..9f71f11928c60 100644
> --- a/tools/perf/tests/builtin-test.c
> +++ b/tools/perf/tests/builtin-test.c
[ ... ]
> @@ -503,12 +509,205 @@ static void finish_test(struct child_test **child_tests, int running_test, int c
>  		fprintf(stderr, "%s", err_output.buf);
>  
>  	strbuf_release(&err_output);
> +	strbuf_release(&child_test->err_output);

[Severity: Medium]
If a signal arrives during finish_tests_parallel() and triggers the signal
handler's siglongjmp to err_out, finish_test() is called to clean up.

At this point, finish_tests_parallel() may have already drained some of the
child's stderr into child_test->err_output. Because finish_test() drains
any remaining output into its local err_output buffer and prints that, will
the previously accumulated data in child_test->err_output be silently
released and lost here?

>  	print_test_result(t, curr_suite, curr_test_case, ret, width, /*running=*/0);
>  	if (err > 0)
>  		close(err);
>  	zfree(&child_tests[running_test]);
>  }
[ ... ]
> +static int finish_tests_parallel(struct child_test **child_tests, size_t num_tests, int width)
> +{
[ ... ]
> +		for (i = next_to_print; i < num_tests; i++) {
> +			struct child_test *child = child_tests[i];
[ ... ]
> +			print_test_result(child->test, child->suite_num, child->test_case_num,
> +					  child->result, width, 0);
> +			strbuf_release(&child->err_output);
> +			child_tests[next_to_print] = NULL;
> +			zfree(&child);
> +			next_to_print++;
> +		}
> +	}
> +
> +	free(global_pfds);
> +	free(global_pfd_indices);
> +	global_pfds = NULL;
> +	global_pfd_indices = NULL;

[Severity: Medium]
Can this sequence trigger a double free if a signal interrupts execution?

If a signal (like SIGINT) is caught immediately after free(global_pfds)
but before global_pfds = NULL, the signal handler invokes siglongjmp
and jumps to err_out in __cmd_test().

The err_out cleanup block unconditionally calls free(global_pfds) again,
which would lead to a double-free since the pointer hasn't been set to
NULL yet.

> +	return 0;
> +}
[ ... ]
> @@ -683,6 +883,10 @@ 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);
>  	}
> +	free(global_pfds);
> +	free(global_pfd_indices);
> +	global_pfds = NULL;
> +	global_pfd_indices = NULL;
>  	free(child_tests);
>  	return err;
>  }

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