Re: [RFC PATCH v4 3/5] perf data-convert: Add perf.data to trace.dat conversion backend

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Arbitrary file truncation and TOCTOU vulnerability via symlink in the `--force` path.
- [Medium] Directly accessing `perf_env` field without verifying initialization.
- [Low] Missing feature fallback stub for `trace_convert__perf2dat` in `data-convert.h`.
- [Low] Lost error code when `fopen()` fails in the `--force` path.
--

--- Patch [3]: [RFC PATCH v4 3/5] perf data-convert: Add perf.data to trace.dat conversion backend ---
commit aaf748bbf23e74fad4be33c3834c0413e8def0f8
Author: Tanushree Shah <[email protected]>

perf data-convert: Add perf.data to trace.dat conversion backend

This commit adds a new conversion backend to perf to translate tracepoint
events from perf.data into trace.dat format. It leverages a new per-CPU
buffering mechanism and implements required callbacks for pipe mode.

> diff --git a/tools/perf/util/data-convert-trace.c b/tools/perf/util/data-convert-trace.c
> new file mode 100644
> index 0000000000000..445479fae8884
> --- /dev/null
> +++ b/tools/perf/util/data-convert-trace.c

[ ... ]

> +	/*
> +	 * In pipe mode, CPU count and page size arrive via feature/tracing_data
> +	 * records before the first sample; initialize buffers lazily on first sample.
> +	 */
> +	if (!cpu_buffers_initialized) {
> +		int nr_cpus = trace_dat_session->header.env.nr_cpus_online;

[Severity: Medium]
Is it safe to access nr_cpus_online directly here without first verifying the
environment is initialized?

The perf subsystem guidelines suggest that accessing perf_env fields without
first verifying those fields are populated can lead to bugs. 

[ ... ]

> +	if (!opts->force) {
> +		int fd = open(to_trace, O_WRONLY | O_CREAT | O_EXCL, 0644);

[ ... ]

> +	} else {
> +		trace_dat_fp = fopen(to_trace, "wb");

[Severity: High]
Could this fopen() in the --force path be susceptible to an arbitrary file
truncation symlink attack?

If an attacker creates a symlink at the destination path, fopen() with "wb"
will follow it and truncate the target file. If the conversion subsequently
fails, the cleanup routine calls unlink(to_trace), which removes the symlink
itself. This leaves the target file truncated while hiding the attacker's
symlink from discovery.

> +		if (!trace_dat_fp) {
> +			pr_err("Failed to open output file: %s\n", to_trace);
> +			return -EINVAL;

[Severity: Low]
Should this return the actual -errno from fopen() instead of a hardcoded
-EINVAL so the exact filesystem error is not lost?

> +		}
> +	}

[ ... ]

> diff --git a/tools/perf/util/data-convert.h b/tools/perf/util/data-convert.h
> index a96240f156712..f041c23252263 100644
> --- a/tools/perf/util/data-convert.h
> +++ b/tools/perf/util/data-convert.h

[ ... ]

> +#ifdef HAVE_LIBTRACEEVENT
> +int trace_convert__perf2dat(const char *input, const char *to_trace,
> +			   struct perf_data_convert_opts *opts);
> +#endif /* HAVE_LIBTRACEEVENT */

[Severity: Low]
Does this declaration need an #else block with a compatible dummy inline stub?

Without a fallback stub when HAVE_LIBTRACEEVENT is absent, callers might
be forced to clutter their code with preprocessor blocks to prevent build
failures.

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