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

On 06/08/26 05:45, Ian Rogers wrote:
> 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
> 
Thanks Ian for the suggestion, It makes sense. I will change the command 
to "perf record -e sched:sched_switch,cpu-clock -a -o "$perfdata" -- 
sleep 0.1".

Thanks
Tanushree Shah

>>> +     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
>>