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

On Mon, Aug 3, 2026 at 9:50 AM <[email protected]> wrote:
>
> 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 creates 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, spurious 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'
>
> 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_dat.sh
> > @@ -0,0 +1,169 @@
> > +#!/bin/bash
>
> [ ... ]
>
> > +perfdata=$(mktemp /tmp/__perf_test.perf.data.XXXXX)
> > +result=$(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=1
> > +             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 deletes
> 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 events"
> > +
> > +     # 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 failures
> 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.

As the `perf record` lacks the `-a` flag this will just record events
for the sleep command and the number of cycles there will be limited.
I'd suggest making this:
perf record -e sched:sched_switch,cpu-clock -a -o "$perfdata" -- sleep 0.1
The cpu-clock is a perf event based on a timer interrupt and should
always exist whether the system be an S390 or a limited system such as
running in a hypervisor.

Thanks,
Ian

> > +     then
> > +             echo "Failed to record perf data (mixed events)"
> > +             err=1
> > +             return
> > +     fi
> > +
> > +     rm -f "$result"
>
> [Severity: Low]
> Does this cleanup ordering leave stale data available for validation on error
> 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 success
> for a failed command.
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5
>