Re: [PATCH v1 12/14] perf test: Add -j/--junit option for JUnit XML test reports

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Medium] `xml_escape` silently discards all non-ASCII/UTF-8 characters due to signed char comparison.
- [Medium] Test suites skipped via the '--skip' command-line option are omitted from the JUnit XML, creating a structural mismatch.
- [Medium] Unsafe file creation for the JUnit XML report is vulnerable to a symlink attack (Arbitrary File Overwrite).
- [Low] The globally scoped `junit_xml_buf` buffer is leaked if the test harness aborts on an error path.
--

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

perf test: Add -j/--junit option for JUnit XML test reports

This commit adds a -j/--junit command line option to perf test to generate
JUnit XML format test reports. The report captures execution latency, failures,
and skip reasons.

> diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c
> index 765724817776e..99f5afba10822 100644
> --- a/tools/perf/tests/builtin-test.c
> +++ b/tools/perf/tests/builtin-test.c
[ ... ]
> @@ -366,8 +372,34 @@ static unsigned int summary_tests_skipped;
>  static unsigned int summary_tests_failed;
>  static struct strbuf summary_failed_tests_buf = STRBUF_INIT;
>  
> +static char *xml_escape(const char *str)
> +{
> +	struct strbuf buf = STRBUF_INIT;
> +	const char *p;
> +	char *res;
> +
> +	if (!str)
> +		return strdup("");
> +
> +	for (p = str; *p; p++) {
> +		if (*p == '&')
> +			strbuf_addstr(&buf, "&amp;");
> +		else if (*p == '<')
> +			strbuf_addstr(&buf, "&lt;");
> +		else if (*p == '>')
> +			strbuf_addstr(&buf, "&gt;");
> +		else if (*p == '"')
> +			strbuf_addstr(&buf, "&quot;");
> +		else if (*p >= 32 || *p == '\n' || *p == '\t')

On architectures where char is signed, will *p >= 32 evaluate to false for
non-ASCII characters (like UTF-8 sequences) since their most significant bit
is set?

Could this silently discard multi-byte characters from test logs, and should
the pointer value be cast to unsigned char before comparison to avoid this?

> +			strbuf_addch(&buf, *p);
> +	}
> +	res = strbuf_detach(&buf, NULL);
> +	return res ? res : strdup("");
> +}
[ ... ]
> @@ -999,6 +1078,29 @@ static void print_tests_summary(void)
>  	} else {
>  		color_fprintf(stderr, PERF_COLOR_GREEN, "Failed tests      : 0\n");
>  	}
> +
> +	if (junit_filename) {
> +		FILE *fp;
> +
> +		fp = fopen(junit_filename, "w");

If perf test -j is executed with elevated privileges in a shared directory like
/tmp, could fopen() with mode "w" be vulnerable to a symlink attack?

Would it be safer to use open() with O_CREAT | O_TRUNC | O_WRONLY | O_NOFOLLOW
to prevent arbitrarily overwriting system files?

> +		if (fp) {
> +			unsigned int total = summary_tests_passed + summary_subtests_passed +
> +					     summary_tests_skipped + summary_tests_failed;
> +			fprintf(fp, "<?xml version=\"1.0\" encoding=\"utf-8\"?>\n");
> +			fprintf(fp, "<testsuites>\n");
> +			fprintf(fp, "  <testsuite name=\"perf-tests\" tests=\"%u\" failures=\"%u\" skipped=\"%u\">\n",
> +				total, summary_tests_failed, summary_tests_skipped);

If a test suite is skipped via the user override (-s/--skip), __cmd_test()
increments summary_tests_skipped but skips calling print_test_result().

Will this cause a structural mismatch in the XML report between the reported
skipped count here and the actual number of <testcase> elements emitted?

> +			fprintf(fp, "%s", junit_xml_buf.buf);
> +			fprintf(fp, "  </testsuite>\n");
> +			fprintf(fp, "</testsuites>\n");
> +			fclose(fp);
> +			pr_info("Wrote junit XML output to %s\n", junit_filename);
> +		} else {
> +			pr_err("Failed to open %s for writing junit XML output: %s\n",
> +			       junit_filename, strerror(errno));
> +		}
> +	}
> +	strbuf_release(&junit_xml_buf);

If the test harness catches a signal (such as Ctrl-C) and jumps to the err_out
label in __cmd_test(), it bypasses print_tests_summary().

Could this cause junit_xml_buf to leak and trigger LeakSanitizer failures?

>  	strbuf_release(&summary_failed_tests_buf);
>  }
>

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