Re: [RFC PATCH v3 5/5] perf test: Add test validating trace.dat generated by 'perf data convert --to-trace-dat'

Thank you for your contribution! Sashiko AI review found 5 potential issue(=
s) to consider:
- [High] Deleting a securely created temporary file and reusing its path cr=
eates a race condition vulnerable to symlink attacks.
- [Medium] The exit trap incorrectly overrides intentional test skips (exit=
 code 2), converting them into test failures (exit code 1).
- [Medium] Hardcoding the 'cycles' hardware PMU event leads to brittle, spu=
rious test failures on environments without hardware performance counters.
- [Low] Stale output file verification due to incorrect cleanup ordering on=
 error paths.
- [Low] A temporary file may be leaked if a subsequent initialization step =
fails before the cleanup trap is registered.
--

--- Patch [5]: [RFC PATCH v3 5/5] perf test: Add test validating trace.dat =
generated by 'perf data convert --to-trace-dat' ---
commit d717cd6622789cb3b2cd038490f59972dc4015dd
Author: Tanushree Shah <[email protected]>

perf test: Add test validating trace.dat generated by 'perf data convert --=
to-trace-dat'
   =20
Add a shell test covering perf data convert --to-trace-dat,
alongside the existing --to-json and --to-ctf tests.

> diff --git a/tools/perf/tests/shell/test_perf_data_converter_tracepoints_=
trace_dat.sh b/tools/perf/tests/shell/test_perf_data_converter_tracepoints_=
trace_dat.sh
> new file mode 100755
> index 0000000000000..9ca6432618cb2
> --- /dev/null
> +++ b/tools/perf/tests/shell/test_perf_data_converter_tracepoints_trace_d=
at.sh
> @@ -0,0 +1,169 @@
> +#!/bin/bash

[ ... ]

> +perfdata=3D$(mktemp /tmp/__perf_test.perf.data.XXXXX)
> +result=3D$(mktemp /tmp/__perf_test.output.trace.dat.XXXXX)

[Severity: Low]
Could a temporary file be leaked if an early initialization step fails?

If the second mktemp fails, the script will exit immediately due to 'set -e=
'.
Since the trap is not yet registered, the first temporary file allocated for
$perfdata wouldn't be cleaned up.

[ ... ]

> +trap_cleanup()
> +{
> +	echo "Unexpected signal in ${FUNCNAME[1]}"
> +	cleanup
> +	exit 1
> +}
> +trap trap_cleanup exit term int

[Severity: Medium]
Does this trap handler unintentionally override test skips?

When the script explicitly executes 'exit 2' for a skip condition (like
missing libtraceevent), the EXIT trap is still active. This handler then
executes 'exit 1', which converts the skip into a spurious test failure.

[ ... ]

> +test_trace_converter_command()
> +{
> +	echo "Testing Perf Data Conversion Command to trace.dat"
> +
> +	if ! perf record -e sched:sched_switch -o "$perfdata" -- sleep 0.1
> +	then
> +		echo "Failed to record perf data"
> +		err=3D1
> +		return
> +	fi
> +
> +	rm -f "$result"

[Severity: High]
Does removing the securely created file path here open a symlink race
condition?

The script uses mktemp to securely allocate the path for $result, but delet=
es
it before passing it to the converter with the --force flag. This leaves
a time window where a predictably named path is free. If run as root, this
could potentially be replaced with a symlink, allowing an arbitrary file
overwrite.

[ ... ]

> +test_trace_converter_pipe()
> +{
> +	echo "Testing Perf Data Conversion Command to trace.dat (Pipe mode)"
> +
> +	rm -f "$result"

[Severity: High]
This test mode appears to have the same symlink race condition as above,
since the securely allocated temporary file is removed prior to use.

[ ... ]

> +test_trace_converter_mixed_events()
> +{
> +	echo "Testing Perf Data Conversion with tracepoint and non-tracepoint e=
vents"
> +
> +	# Record both tracepoint and non-tracepoint events
> +	if ! perf record -e sched:sched_switch,cycles -o "$perfdata" -- sleep 0=
.1

[Severity: Medium]
Could hardcoding the 'cycles' hardware PMU event cause spurious test failur=
es
on restricted environments?

If the hardware 'cycles' event fails to open on virtual machines or
unprivileged containers, perf record will return an error and fail the test
step instead of skipping it or falling back to a software event.

> +	then
> +		echo "Failed to record perf data (mixed events)"
> +		err=3D1
> +		return
> +	fi
> +
> +	rm -f "$result"

[Severity: Low]
Does this cleanup ordering leave stale data available for validation on err=
or
paths?

If the 'perf record' command above fails, it returns early before this file
is removed. When validate_trace_format() is later called in the global flow,
it will check the $result file left over from the preceding
test_trace_converter_pipe() execution, which might erroneously report succe=
ss
for a failed command.

--=20
Sashiko AI review =C2=B7 https://sashiko.dev/#/patchset/20260803145958.2999=
[email protected]?part=3D5