Re: [PATCH v3 3/5] perf trace-event: Avoid double free in trace_event__cleanup()

Thanks for the review

On 24/07/26 20:25, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [High] Calling trace_event__init() repeatedly on the same struct trace_event leaks the previously allocated tep_handle and plugins.
> --
> 
> --- Patch [3]: [PATCH v3 3/5] perf trace-event: Avoid double free in trace_event__cleanup() ---
> commit 1eb2775370e0095c8df022b4a83dcd46f4b90b94
> Author: Tanushree Shah <[email protected]>
> 
> perf trace-event: Avoid double free in trace_event__cleanup()
> 
> trace_event__cleanup() frees t->pevent but never clears the pointer. It can be
> called twice on the same trace_event: once from trace_report()'s error path,
> and again from perf_session__delete() during session teardown, resulting in a
> double free / use-after-free.
> 
>> diff --git a/tools/perf/util/trace-event.c b/tools/perf/util/trace-event.c
>> index 6a8c66c64b705..cf40e98d16177 100644
>> --- a/tools/perf/util/trace-event.c
>> +++ b/tools/perf/util/trace-event.c
>> @@ -63,8 +63,12 @@ int trace_event__register_resolver(struct machine *machine,
>>   
>>   void trace_event__cleanup(struct trace_event *t)
>>   {
>> +	if (!t->pevent)
>> +		return;
>> +
> 
> [Severity: High]
> This isn't a bug introduced by this patch, but while we are making it safe
> to call trace_event__cleanup() multiple times, should we also call it from
> trace_event__init() to prevent memory leaks?
> 
> If a maliciously crafted perf.data file contains multiple
> PERF_RECORD_HEADER_TRACING_DATA headers, trace_report() can be called
> multiple times on the same session, invoking trace_event__init() repeatedly:

Confirmed, this is a real leak. I'll fold a fix into v4 alongside
the trace_event__cleanup() double-free fix, since the two are
related.
> 
> tools/perf/util/trace-event-read.c:trace_report() {
>      ...
> 	if (trace_event__init(tevent)) {
>      ...
> }
> 
> This overwrites the previously allocated tep_handle and plugins without
> freeing them, leaking the memory:
> 
> tools/perf/util/trace-event.c:trace_event__init() {
>      ...
> 		t->plugin_list = tep_load_plugins(pevent);
> 		t->pevent  = pevent;
>      ...
> }
> 
>>   	tep_unload_plugins(t->plugin_list, t->pevent);
>>   	tep_free(t->pevent);
>> +	t->pevent = NULL;
>>   }
>>   
>>   /*
>