Re: [PATCH v13 24/32] perf timechart: Fix memory leaks

[Namhyung Kim <namhyung@kernel.org>]

On Tue, May 12, 2026 at 03:29:53PM -0700, Ian Rogers wrote:
> Fix missing free from cat_backtrace, which requires copies of the
> backtrace being made in functions that save it.
> 
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/perf/builtin-timechart.c | 23 ++++++++++++++---------
>  1 file changed, 14 insertions(+), 9 deletions(-)
> 
> diff --git a/tools/perf/builtin-timechart.c b/tools/perf/builtin-timechart.c
> index 1d24d8738519..64f69bd67c6b 100644
> --- a/tools/perf/builtin-timechart.c
> +++ b/tools/perf/builtin-timechart.c
> @@ -299,7 +299,7 @@ static void pid_put_sample(struct timechart *tchart, int pid, int type,
>  	sample->type = type;
>  	sample->next = c->samples;
>  	sample->cpu = cpu;
> -	sample->backtrace = backtrace;
> +	sample->backtrace = backtrace ? strdup(backtrace) : NULL;
>  	c->samples = sample;
>  
>  	if (sample->type == TYPE_RUNNING && end > start && start > 0) {
> @@ -433,7 +433,7 @@ static void sched_wakeup(struct timechart *tchart, int cpu, u64 timestamp,
>  
>  	we->time = timestamp;
>  	we->waker = waker;
> -	we->backtrace = backtrace;
> +	we->backtrace = backtrace ? strdup(backtrace) : NULL;
>  
>  	if ((flags & TRACE_FLAG_HARDIRQ) || (flags & TRACE_FLAG_SOFTIRQ))
>  		we->waker = -1;
> @@ -489,9 +489,9 @@ static void sched_switch(struct timechart *tchart, int cpu, u64 timestamp,
>  	}
>  }
>  
> -static const char *cat_backtrace(union perf_event *event,
> -				 struct perf_sample *sample,
> -				 struct machine *machine)
> +static char *cat_backtrace(union perf_event *event,
> +			   struct perf_sample *sample,
> +			   struct machine *machine)
>  {
>  	struct addr_location al;
>  	unsigned int i;
> @@ -544,8 +544,10 @@ static const char *cat_backtrace(union perf_event *event,
>  				 * It seems the callchain is corrupted.
>  				 * Discard all.
>  				 */
> -				zfree(&p);
> -				goto exit;
> +				addr_location__exit(&al);
> +				fclose(f);
> +				free(p);
> +				return NULL;

Nit: it looks like an unnecessary change.


>  			}
>  			continue;
>  		}
> @@ -577,6 +579,7 @@ static int process_sample_event(const struct perf_tool *tool,
>  {
>  	struct timechart *tchart = container_of(tool, struct timechart, tool);
>  	struct evsel *evsel = sample->evsel;
> +	int ret = 0;
>  
>  	if (evsel->core.attr.sample_type & PERF_SAMPLE_TIME) {
>  		if (!tchart->first_time || tchart->first_time > sample->time)
> @@ -587,11 +590,13 @@ static int process_sample_event(const struct perf_tool *tool,
>  
>  	if (evsel->handler != NULL) {
>  		tracepoint_handler f = evsel->handler;
> +		char *backtrace = cat_backtrace(event, sample, machine);
>  
> -		return f(tchart, sample, cat_backtrace(event, sample, machine));
> +		ret = f(tchart, sample, backtrace);
> +		free(backtrace);

It's suboptimial that it generates backtrace always and copy it again.
But it can be fixed separately.

Thanks,
Namhyung

>  	}
>  
> -	return 0;
> +	return ret;
>  }
>  
>  static int
> -- 
> 2.54.0.563.g4f69b47b94-goog
>