Re: [PATCH v2 07/14] perf test: Show snippet failure output for verbose=1

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Passing uncast characters to `tolower()` causes undefined behavior and potential out-of-bounds reads for non-ASCII characters.
- [Medium] The extraction algorithm's implementation order violates the priority described in the commit message, causing execution boundary markers to be discarded if failure keywords exhaust the line quota.
--

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

perf test: Show snippet failure output for verbose=1

Currently, when running tests in verbose mode (-v), if a test case fails,
the entire raw standard error buffer is dumped to stderr.
This patch implements a smart, bounded snippet string processor to improve
failure triaging by extracting keywords and test boundary markers.

>     2. Parse the raw error buffer dynamically into lines and run a three-pass
>        extraction algorithm:
>        - Pass 0: Prioritize retaining outline marker lines containing dashes
>          (====) to beautifully frame the start and end execution boundaries.
>        - Pass 1: Scan forward from the top of the log to pick up to N lines
>          that contain case-insensitive failure keywords (error, fail, segv,

[Severity: Medium]
The commit message states that outline marker lines "====" are prioritized as
Pass 0. However, the implementation processes failure keywords first as Pass 1
and then searches for "----" as Pass 2.

[ ... ]

> diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c
> index 87c6a67beb10e..ec44b24d419c1 100644
> --- a/tools/perf/tests/builtin-test.c
> +++ b/tools/perf/tests/builtin-test.c

[ ... ]

> @@ -395,6 +397,207 @@ static int print_test_result(struct test_suite *t, int curr_suite, int curr_test
>  	return 0;
>  }
>  
> +static const char * const fail_keywords[] = {
> +	"error", "fail", "segv", "abort",
> +	"signal", "fatal", "panic", "corrupt", NULL
> +};
> +
> +static const char *find_next_keyword(const char *str, size_t max_len, size_t *kw_len)
> +{
> +	const char *best = NULL;
> +	size_t best_len = 0;
> +	int k;
> +
> +	for (k = 0; fail_keywords[k]; k++) {
> +		const char *s = str;
> +		size_t len = strlen(fail_keywords[k]);
> +
> +		while ((size_t)(s - str) + len <= max_len) {
> +			size_t i;
> +
> +			for (i = 0; i < len; i++) {
> +				if (tolower(s[i]) != fail_keywords[k][i])

[Severity: Medium]
Since s is a const char *, s[i] can be negative if it's a non-ASCII character.
Passing negative values to tolower() can trigger undefined behavior or
out-of-bounds array reads in glibc implementations.

Should s[i] be cast to unsigned char before being passed to tolower()?

> +					break;
> +			}
> +			if (i == len) {
> +				if (!best || s < best) {

[ ... ]

> +static void print_test_failure_snippet(FILE *fp, const char *buf)
> +{

[ ... ]

> +	/* Pass 1: Pick lines with failure keywords from start (Highest Priority) */
> +	for (i = 0; i < num_lines && picked_count < failure_snippet_lines; i++) {
> +		size_t dummy;
> +
> +		if (find_next_keyword(lines[i], line_lens[i], &dummy)) {
> +			if (!pick[i]) {
> +				pick[i] = true;
> +				picked_count++;
> +			}
> +			/* Prioritize getting the immediate next line for context */
> +			if (i + 1 < num_lines && !pick[i + 1] &&
> +			    picked_count < failure_snippet_lines) {
> +				pick[i + 1] = true;
> +				picked_count++;
> +			}
> +		}
> +	}
> +
> +	/* Pass 2: Pick marker lines up to the remaining quota */
> +	for (i = 0; i < num_lines && picked_count < failure_snippet_lines; i++) {
> +		if (!pick[i] && line_contains(lines[i], line_lens[i], "----")) {

[Severity: Medium]
The extraction algorithm's implementation order differs from the priority
described in the commit message.

Because failure keywords are processed here as Pass 1, if a failure log is
highly verbose and contains enough failure keywords to hit the
failure_snippet_lines limit (default 10) in Pass 1, will the execution boundary
markers be silently discarded in Pass 2?

Additionally, this code matches "----" rather than the "====" described in
the commit message.

> +			pick[i] = true;
> +			picked_count++;
> +		}
> +	}

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