Re: [PATCH perf-tools-next v2 4/4] perf tests: Add shell test for kernel symbol beautifier

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Medium] The regular expression used to verify kernel symbol beautification incorrectly matches raw hexadecimal addresses, causing the test to falsely pass even if symbolization fails.
- [Medium] Temporary file created by `mktemp` is leaked on early exits.
- [Low] Unquoted `$0` in `dirname` command substitution can cause the script to fail if the execution path contains spaces.
- [Low] Command output is directly interpolated into a `printf` format string, which will mangle output containing `%` or `\` characters.
--

--- Patch [4]: [PATCH perf-tools-next v2 4/4] perf tests: Add shell test for kernel symbol beautifier ---
commit 3b67c2553760890642357a39f8d2c206545b9020
Author: Aaron Tomlin <[email protected]>

perf tests: Add shell test for kernel symbol beautifier

Add a dedicated shell test script, trace_ksym_beautifier.sh, to verify
that 'perf trace' properly symbolises kernel virtual addresses and
function pointers using both the default kallsyms beautifier (SCA_KSYM)
and BTF type routing i.e., --force-btf.

> diff --git a/tools/perf/tests/shell/trace_ksym_beautifier.sh b/tools/perf/tests/shell/trace_ksym_beautifier.sh
> new file mode 100755
> index 0000000000000..2916f78503f71
> --- /dev/null
> +++ b/tools/perf/tests/shell/trace_ksym_beautifier.sh
> @@ -0,0 +1,52 @@
> +#!/bin/bash
> +# perf trace kernel symbol beautifier tests
> +# SPDX-License-Identifier: GPL-2.0
> +
> +err=0
> +OUTPUT=$(mktemp /tmp/perf_trace_test.XXXXX)
> +
> +# shellcheck source=lib/probe.sh
> +. "$(dirname $0)"/lib/probe.sh

[Severity: Low]
Does the unquoted $0 in this dirname substitution cause the script to fail if
the execution path contains spaces?

> +skip_if_no_perf_trace || exit 2
> +[ "$(id -u)" = 0 ] || exit 2

[Severity: Medium]
Since OUTPUT is created via mktemp before these early exit checks, and the
cleanup trap isn't registered until later, do these exits leak the temporary
file in /tmp?

> +
> +cleanup() {
> +  rm -f ${OUTPUT}
> +}
> +
> +trap cleanup EXIT TERM INT HUP
> +
> +test_ksym_kallsyms() {
> +  echo "Testing perf trace kernel symbol beautifier (default kallsyms)"
> +  perf trace -e kmem:kmalloc --max-events=1 > ${OUTPUT} 2>&1
> +  if ! grep -q -E "call_site: [a-zA-Z0-9_]+" ${OUTPUT}

[Severity: Medium]
Does this regular expression inadvertently match raw hexadecimal addresses?
Because the character x and hex digits fall within the [a-zA-Z0-9_]+
character class, it appears this could falsely pass if symbolization fails
and perf trace outputs un-symbolized addresses like 0xffffffff12345678.

> +  then
> +    printf "Default kallsyms function symbolization failed, output:\n$(cat ${OUTPUT})\n"

[Severity: Low]
Could directly interpolating the command output into the printf format string
cause issues? If the output contains % or \ characters, printf might
interpret them as format specifiers or escape sequences.

> +    err=1
> +  fi
> +}
> +
> +test_ksym_btf() {
> +  echo "Testing perf trace kernel symbol beautifier (BTF)"
> +  if [ ! -f /sys/kernel/btf/vmlinux ]; then
> +    echo "Skipping BTF test due to missing vmlinux BTF"
> +    return
> +  fi
> +
> +  perf trace -e kmem:kmalloc --force-btf --max-events=1 > ${OUTPUT} 2>&1
> +  if ! grep -q -E "call_site: [a-zA-Z0-9_]+" ${OUTPUT}

[Severity: Medium]
Similar to the kallsyms test above, does this regex incorrectly match raw hex
addresses when BTF symbolization fails?

> +  then
> +    printf "BTF function symbolization failed, output:\n$(cat ${OUTPUT})\n"

[Severity: Low]
As with the previous printf, does embedding the output directly into the
format string risk mangling the output if it contains format specifiers?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4