From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-alma10-1.taild15c8.ts.net [100.103.45.18]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 947D1279DAD for ; Sat, 22 Aug 2026 06:36:43 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=100.103.45.18 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1787380605; cv=none; b=M4zSQcFBJ/Ds8l0EeS7UiBd+KVrkkJIGB24UhyUVaywMMQLQ3jo554KSPgJli58XBuOf77apem6Ym8J8dzF+LluyqpIAJgiIjhg+KM+D9C2eg2ghJYaeJZp52eXcaZGXFfSxXs08RVLwvkf1fpoPmpPyUUV2nRNto9WmR97Ku8A= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1787380605; c=relaxed/simple; bh=Oq0m7He/55OFmmviP/wnij+vEQNP4X0hex2enef5o44=; h=From:Subject:To:Cc:In-Reply-To:References:Content-Type:Date: Message-Id; b=ZrFY3xuM6snfq/w8AiIHoyk4rSrqL/E7tiIyP6GVuz4sZihNqcNL8QVHCjv01FkZtD8KS1Lr71yTj8Nsd/uwrRWOKjZcIpomyIxQXI+e2xFgKE/w6mts6jFeIuPaCM5OV0u2vLO7tYan0rI9BxYDosO7oY3/zEmyalQT67dNqvE= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=AOvxGy+n; arc=none smtp.client-ip=100.103.45.18 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="AOvxGy+n" Received: by smtp.kernel.org (Postfix) with ESMTPSA id 7CAA41F000E9; Sat, 22 Aug 2026 06:36:42 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=kernel.org; s=k20260515; t=1787380602; bh=ZFWVmD6+oiu2QMQcca0VPwgSs3xaL9W+SoGEWYL+5qE=; h=From:Subject:Reply-To:To:Cc:In-Reply-To:References:Date; b=AOvxGy+nw/HMeve3oQp2eXIQcjDfxA9Xj6RBNGZ2aJmshxZDhYg0P6Ds6QfexDMF8 FrnaynHJZ0CqSobcrXXfFwnbM6SvZ8diPJvixs2j6SPKPaQgHgRY9op++/E7i8LaCd 9RbYxt6XnwN0tItDA5Cpuv9N0dg63PeYfcZf5uN+BPgiWoj2ZvQeMlvuLrZaKlSDme cK6EOXbVDyD2dmpJKlp4e+7bLDCeM+jG0LqArMcsuXlM84/vXjBEDkJ5jKo+ScCWnR 1vbYenN82eGirvWC5PL2kTHqmoOIvn//Qk136huC1vma1DeCI8wQ+IZay1CnNsUoWS h9XH4tcQwcORw== From: sashiko-bot@kernel.org Subject: Re: [RFC PATCH v4 5/5] perf test: Add test validating trace.dat generated by 'perf data convert --to-trace-dat' Reply-To: sashiko-reviews@lists.linux.dev To: "Tanushree Shah" Cc: linux-perf-users@vger.kernel.org In-Reply-To: <20260822062729.882005-6-tshah@linux.ibm.com> References: <20260822062729.882005-1-tshah@linux.ibm.com> <20260822062729.882005-6-tshah@linux.ibm.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Date: Sat, 22 Aug 2026 06:36:42 +0000 Message-Id: <20260822063642.7CAA41F000E9@smtp.kernel.org> Precedence: bulk X-Mailing-List: linux-perf-users@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: Thank you for your contribution! Sashiko AI review found 3 potential issue(= s) to consider: - [Medium] Insecure temporary file reuse creates a time-of-check to time-of= -use (TOCTOU) race condition and symlink attack vulnerability. - [Medium] The commit message claims to test the 'cycles' event, but the sc= ript uses 'cpu-clock'. - [Medium] The script leaks a temporary file (`${perfdata}.old`) in `/tmp` = because `perf record` automatically backs up the existing non-empty output = file. -- --- Patch [5]: [RFC PATCH v4 5/5] perf test: Add test validating trace.dat = generated by 'perf data convert --to-trace-dat' --- commit 9c32c2021bfda9ef574a5486cbcea176af233faa Author: Tanushree Shah 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. The test: - skips if perf isn't linked with libtraceevent - converts a plain sched:sched_switch recording to trace.dat - converts a pipe-mode recording (perf record -o - | perf data convert -i -) > - converts a recording with both tracepoint and non-tracepoint > events (sched:sched_switch,cycles) [Severity: Medium] Is this description accurate? The commit message mentions the "cycles" event, but the code later tests the "cpu-clock" event instead. - validates the resulting file with trace-cmd report if trace-cmd is installed, otherwise falls back to a basic non-empty-file check Signed-off-by: Tanushree Shah > 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..1c0bce270c389 > --- /dev/null > +++ b/tools/perf/tests/shell/test_perf_data_converter_tracepoints_trace_d= at.sh > @@ -0,0 +1,173 @@ > +#!/bin/bash > +# 'perf data convert --to-trace-dat' command test > +# SPDX-License-Identifier: GPL-2.0-or-later > +# > +# Copyright 2026, IBM Corporation > +# Author: Tanushree Shah > + > +set -e > + > +err=3D0 > + > +perfdata=3D$(mktemp /tmp/__perf_test.perf.data.XXXXX) > + > +cleanup() > +{ > + rm -f "${perfdata}" [Severity: Medium] Does this code leak the backup file created by perf? When perf record is called a second time in test_trace_converter_mixed_even= ts() with -o "$perfdata", it detects that the file already exists and has conten= t. perf record automatically renames the existing file to "${perfdata}.old" be= fore creating a new one. Since cleanup() only removes "${perfdata}", does the .old file get left behind in /tmp? > + rm -f "${result}" > + trap - exit term int > +} > + [ ... ] > +test_trace_converter_pipe() > +{ > + echo "Testing Perf Data Conversion Command to trace.dat (Pipe mode)" > + > + rm -f "$result" > + > + if ! perf record -e sched:sched_switch -a -o - -- sleep 0.1 | \ > + perf data convert --to-trace-dat "$result" --force -i - [Severity: Medium] Does this code create a time-of-check to time-of-use race condition? The script securely creates a temporary file using mktemp, but explicitly deletes it here with rm -f "$result".=20 Since the predictable and unsecured path is then immediately reused for perf data convert, could a local attacker insert a symlink during this wind= ow, causing perf data convert to overwrite arbitrary files? > + then > + echo "Perf Data Converter Command to trace.dat (Pipe mode) [FAILED]" > + err=3D1 > + return > + fi > + [ ... ] > +test_trace_converter_mixed_events() > +{ > + echo "Testing Perf Data Conversion with tracepoint and non-tracepoint e= vents" > + > + rm -f "$result" > + > + # Record both tracepoint and non-tracepoint events > + if ! perf record -e sched:sched_switch,cpu-clock -a -o "$perfdata" -- s= leep 0.1 > + then > + echo "Failed to record perf data (mixed events)" > + err=3D1 > + return > + fi > + > + if ! perf data convert --to-trace-dat "$result" --force -i "$perfdata" [Severity: Medium] Does this code repeat the symlink race condition mentioned above? The securely created $result file is again explicitly deleted and its path is reused here. --=20 Sashiko AI review =C2=B7 https://sashiko.dev/#/patchset/20260822062729.8820= 05-1-tshah@linux.ibm.com?part=3D5