Re: [RFC PATCH 4/4] perf data: Add --to-trace-dat option for converting perf.data tracepoint events into trace.dat format

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] The `trace_convert__perf2dat` tool fails to register required `.attr`, `.feature`, and `.tracing_data` callbacks, breaking pipe mode.
- [High] Accessing unverified `perf_env` field `nr_cpus_online` leads to deterministic failure when CPU topology feature is missing.
- [High] Failing `realloc()` in `trace_dat__collect_cpu_event()` leaks memory and causes a NULL pointer dereference in the cleanup path.
- [Low] Missing semicolon after `return -1` in the `#else` branch inside `cmd_data_convert`.
--

commit 789bd3d29fe2134b1ed0b9a17d6c674bc7d7db23
Author: Tanushree Shah <tshah@linux.ibm.com>

perf data: Add --to-trace-dat option for converting perf.data tracepoint events into trace.dat format
    
Add new command-line option to perf data convert for generating
trace.dat output files. Accepts output filename for trace.dat format,
mutually exclusive with --to-ctf and --to-json, and calls
trace_convert__perf2dat() to perform conversion.

> diff --git a/tools/perf/builtin-data.c b/tools/perf/builtin-data.c
> index 4c08ccb8c06b7..96ccab77456a3 100644
> --- a/tools/perf/builtin-data.c
> +++ b/tools/perf/builtin-data.c

[ ... ]

> @@ -65,16 +72,43 @@ static int cmd_data_convert(int argc, const char **argv)
[ ... ]
>  #else
> +	#ifdef HAVE_LIBTRACEEVENT
> +	if (!to_json && !trace_dat_output) {
> +		pr_err("You must specify --to-json or --to-trace-dat.\n");
> +		return -1;
> +	}
> +	#else
>  	if (!to_json) {
>  		pr_err("You must specify --to-json.\n");
> -	return -1;
> -}
> +		return -1
> +		}

[Severity: Low]
Is there a missing semicolon here? It looks like this will break compilation
for environments lacking both HAVE_LIBBABELTRACE_SUPPORT and
HAVE_LIBTRACEEVENT.

> +	#endif
> +#endif
> +
> +#ifdef HAVE_LIBTRACEEVENT
> +	if (trace_dat_output)
> +		return trace_convert__perf2dat(input_name ? input_name : "perf.data",
> +					       trace_dat_output, &opts);

[Severity: High]
Does trace_convert__perf2dat() register the required callbacks for pipe mode?

Looking at the tool initialization in trace_convert__perf2dat():

	/* Initialize tool with all required callbacks */
	perf_tool__init(&tc.tool, /*ordered_events=*/true);
	tc.tool.sample = process_sample_event;

If .attr and .feature are not overridden, won't incoming
perf_event_header_attr and feature events from a pipe be silently dropped,
preventing the creation of evlists/evsels and breaking event processing?

[Severity: High]
How does this code handle missing CPU topology features?

Looking at trace_convert__perf2dat():

		if (trace_dat__init_cpu_buffers(session->header.env.nr_cpus_online) < 0) {

If the feature callback is not registered (like in pipe mode) or the CPU
topology feature wasn't recorded, wouldn't session->header.env.nr_cpus_online
remain 0?

If trace_dat__init_cpu_buffers(0) succeeds with 0 elements, won't
trace_dat__collect_cpu_event() later check cpu >= trace_dat_nr_cpus (0 >= 0)
and return -EINVAL, causing process_sample_event to return -ENOMEM and abort
the conversion?

[Severity: High]
Is it possible for a reallocation failure to cause a memory leak and
subsequent NULL pointer dereference?

Looking at trace_dat__collect_cpu_event():

		cpu_events->events   = realloc(cpu_events->events,
				       cpu_events->capacity * sizeof(*cpu_events->events));
		if (!cpu_events->events)
			return -ENOMEM;

If realloc fails, doesn't it overwrite cpu_events->events with NULL, leaking
the old array?

Furthermore, if this returns -ENOMEM and cascades up to trigger the cleanup
path in trace_dat__free_cpu_buffers():

		for (i = 0; i < trace_cpu_data[cpu].count; i++)
			free(trace_cpu_data[cpu].events[i].raw);

Since count was not reset, won't this loop access events[i] and dereference
the NULL pointer?

> +#endif

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260608125951.90425-2-tshah@linux.ibm.com?part=4