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

[Tanushree Shah <tshah@linux.ibm.com>]

Hi,

Thank you for the Sashiko AI review feedback. I acknowledge the issues 
identified and will address them in v2..
On 08/06/26 18:42, sashiko-bot@kernel.org wrote:
> 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.
> 
yes, I will fix it.
>> +	#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?
yes, As mentioned in the reply of patch 3 review, I will add these 
callbacks.
> 
> [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?

Implement lazy initialization by deferring CPU buffer setup until the 
first sample event, after process_feature() and process_tracing_data() 
have populated the session header. This will also ensure that we use the 
recorded machine's CPU count also in case of pipe perf.data , and not 
the host system's, which is critical for cross-platform analysis.
> 
> 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;
As I mentioned in reply of 1st patch review, use a temporary pointer for 
realloc() and only update cpu_events->events and cpu_events->capacity 
after successful allocation.
> 
> 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?
> 

Additionally, add defensive NULL check in trace_dat__free_cpu_buffer() 
cleanup.


>> +#endif
>