Re: [PATCH v2 2/2] perf report: Fix memory leaks in the hierarchy mode

[Falcon, Thomas]

On Thu, 2025-03-06 at 22:12 -0800, Namhyung Kim wrote:
> Ian told me that there are many memory leaks in the hierarchy mode. 
> I
> can easily reproduce it with the follwing command.
> 
>   $ make DEBUG=1 EXTRA_CFLAGS=-fsanitize=leak
> 
>   $ perf record --latency -g -- ./perf test -w thloop
> 
>   $ perf report -H --stdio
>   ...
>   Indirect leak of 168 byte(s) in 21 object(s) allocated from:
>       #0 0x7f3414c16c65 in malloc
> ../../../../src/libsanitizer/lsan/lsan_interceptors.cpp:75
>       #1 0x55ed3602346e in map__get util/map.h:189
>       #2 0x55ed36024cc4 in hist_entry__init util/hist.c:476
>       #3 0x55ed36025208 in hist_entry__new util/hist.c:588
>       #4 0x55ed36027c05 in hierarchy_insert_entry util/hist.c:1587
>       #5 0x55ed36027e2e in hists__hierarchy_insert_entry
> util/hist.c:1638
>       #6 0x55ed36027fa4 in hists__collapse_insert_entry
> util/hist.c:1685
>       #7 0x55ed360283e8 in hists__collapse_resort util/hist.c:1776
>       #8 0x55ed35de0323 in report__collapse_hists
> /home/namhyung/project/linux/tools/perf/builtin-report.c:735
>       #9 0x55ed35de15b4 in __cmd_report
> /home/namhyung/project/linux/tools/perf/builtin-report.c:1119
>       #10 0x55ed35de43dc in cmd_report
> /home/namhyung/project/linux/tools/perf/builtin-report.c:1867
>       #11 0x55ed35e66767 in run_builtin
> /home/namhyung/project/linux/tools/perf/perf.c:351
>       #12 0x55ed35e66a0e in handle_internal_command
> /home/namhyung/project/linux/tools/perf/perf.c:404
>       #13 0x55ed35e66b67 in run_argv
> /home/namhyung/project/linux/tools/perf/perf.c:448
>       #14 0x55ed35e66eb0 in main
> /home/namhyung/project/linux/tools/perf/perf.c:556
>       #15 0x7f340ac33d67 in __libc_start_call_main
> ../sysdeps/nptl/libc_start_call_main.h:58
>   ...
> 
>   $ perf report -H --stdio 2>&1 | grep -c '^Indirect leak'
>   93
> 
> I found that hist_entry__delete() missed to release child entries in
> the
> hierarchy tree (hroot_{in,out}).  It needs to iterate the child
> entries
> and call hist_entry__delete() recursively.
> 
> After this change:
> 
>   $ perf report -H --stdio 2>&1 | grep -c '^Indirect leak'
>   0
> 
> Reported-by: Ian Rogers <irogers@xxxxxxxxxx>
> Signed-off-by: Namhyung Kim <namhyung@xxxxxxxxxx>

Tested on an Arrow Lake system.

Tested-by Thomas Falcon <thomas.falcon@xxxxxxxxx>

> ---
> v2) Use rbtree_postorder_for_each_entry_safe()  (Ian)
> 
>  tools/perf/util/hist.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
> index fbf131aeae7ffe9b..d65228c1141251fb 100644
> --- a/tools/perf/util/hist.c
> +++ b/tools/perf/util/hist.c
> @@ -1385,6 +1385,16 @@ void hist_entry__delete(struct hist_entry *he)
>  {
>  	struct hist_entry_ops *ops = he->ops;
>  
> +	if (symbol_conf.report_hierarchy) {
> +		struct rb_root *root = &he->hroot_out.rb_root;
> +		struct hist_entry *child, *tmp;
> +
> +		rbtree_postorder_for_each_entry_safe(child, tmp,
> root, rb_node)
> +			hist_entry__delete(child);
> +
> +		*root = RB_ROOT;
> +	}
> +
>  	thread__zput(he->thread);
>  	map_symbol__exit(&he->ms);
>