Re: [PATCH 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 3 potential issue(s) to consider:
- [High] Double-free vulnerability of `global_pfds` and `global_pfd_indices` via asynchronous signal handler interruption.
- [High] Heap corruption risk due to freeing an interrupted `strbuf` reallocation during signal recovery.
- [Medium] Partially accumulated stderr output from test children is silently dropped during error recovery or test interruption.
--

commit 87845502f87dde054c91ccdd4d7201529645fceb
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 (-v), child processes
    write to pipes. If a test produces significant output (e.G. Granite Rapids
    metric parsing printing hundreds of lines), it fills the 64KB pipe buffer
    and blocks.
    
    Previously, the parent harness (finish_test) only polled the pipe of the
    current test waiting to be printed. Other children blocked indefinitely
    until the parent reached them, severely sequentializing execution.
    
    Address this by implementing finish_tests_parallel() to poll and drain
    output pipes from all running children simultaneously into per-child
    buffers. Reaping occurs out of order as children finish, while final
    result printing remains strictly in order.

> 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]
Will this silently drop partially accumulated output during error recovery?

If the test suite is interrupted (e.g., via SIGINT), the signal handler jumps
to the err_out label in __cmd_test(), which attempts to gracefully reap all
started tests by invoking finish_test().

However, finish_test() uses a local strbuf err_output to collect only the
remaining output from the pipe. It then prints this local buffer and
unconditionally frees child_test->err_output without printing its contents.

This discards the output that was previously successfully collected by
finish_tests_parallel(), making it difficult to diagnose tests that were
running when the harness was interrupted.

>  	print_test_result(t, curr_suite, curr_test_case, ret, width, /*running=*/0);
>  	if (err > 0)
>  		close(err);
>  	zfree(&child_tests[running_test]);
>  }
>  
> +static void drain_child_process_err(struct child_test *child)
> +{
> +	char buf[512];
> +	ssize_t len;
> +
> +	while ((len = read(child->process.err, buf, sizeof(buf) - 1)) > 0) {
> +		buf[len] = '\0';
> +		strbuf_addstr(&child->err_output, buf);

[Severity: High]
Is there a risk of heap corruption if a signal interrupts this reallocation?

The drain_child_process_err() function appends child process stderr to
child->err_output using strbuf_addstr(), which dynamically grows the buffer by
invoking realloc().

If an asynchronous signal (e.g., SIGINT) arrives while the heap is actively
being modified inside realloc(), the signal handler's siglongjmp abruptly aborts
the process.

The recovery path then attempts to clean up memory in finish_test() by calling
strbuf_release(&child_test->err_output), which invokes free() on the potentially
corrupted heap state. This could deadlock the allocator or crash.

> +	}
> +}
> +
[ ... ]
> +		while (next_to_print < num_tests) {
> +			struct child_test *child = child_tests[next_to_print];
[ ... ]
> +			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);

[Severity: High]
Does this create a double-free vulnerability if a signal interrupts here?

The global variables global_pfds and global_pfd_indices are freed here, and
subsequently set to NULL. However, the test harness sets up an asynchronous
signal handler for SIGINT/SIGTERM which executes siglongjmp to jump back to
__cmd_test()'s error recovery path.

If a signal interrupts the execution exactly after free(global_pfds) but before
global_pfds = NULL, the signal handler unwinds the stack to __cmd_test(),
which then unconditionally executes free(global_pfds) in the err_out block.
Since the pointer hasn't been cleared yet, this causes a double free.

> +	free(global_pfd_indices);
> +	global_pfds = NULL;
> +	global_pfd_indices = NULL;
> +	return 0;
> +}

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