Re: [PATCH v5 v5 6/9] perf c2c: add function view hierarchy entry creation

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] Raw pointer assignment of reference-counted structs bypasses lifecycle helpers.
- [Medium] Custom destructor `c2c_child_he__free` leaks lazy-evaluated string fields.
- [Medium] Missing `ops` callback initialization on child histogram entries causes NULL pointer dereference crash risk.
--

--- Patch [6]: [PATCH v5 v5 6/9] perf c2c: add function view hierarchy entry creation ---
commit 53abae650d662cc8bba9920d631a2983397312a7
Author: Jiebin Sun <[email protected]>

perf c2c: add function view hierarchy entry creation

This commit adds the entry-creation layer for the function view hierarchy.
It introduces child allocation and insertion logic, alongside lookup-or-create
functions for levels 1, 2, and 3 based on function symbol or cacheline index.
These functions are marked as __maybe_unused until integrated by later patches.

> diff --git a/tools/perf/ui/browsers/c2c-function.c b/tools/perf/ui/browsers/c2c-function.c
> index 051c38697edd5..b3528edb387a7 100644
> --- a/tools/perf/ui/browsers/c2c-function.c
> +++ b/tools/perf/ui/browsers/c2c-function.c

[ ... ]

> @@ -793,11 +811,6 @@ static void c2c_child_he__free(struct hist_entry *child_he)
>  		zfree(&child_c2c->hists);
>  	}
>  
> -	zfree(&child_c2c->cpuset);
> -	zfree(&child_c2c->nodeset);
> -	zfree(&child_c2c->nodestr);
> -	zfree(&child_c2c->node_stats);
> -
>  	c2c_he__free_hierarchy(child_he);
>  

[Severity: Medium]
Does this custom destructor leak lazy-evaluated string fields like srcline
and srcfile?

If the user resolves source lines in the function view UI, hist_entry__srcline
dynamically allocates strings for srcline and srcfile.

Because this destructor manually cleans up fields instead of using the core
hist_entry__delete destructor, it misses cleaning up child_he->srcline and
child_he->srcfile, creating a memory leak when the hierarchy is torn down.

>  	free(child_c2c);
>  }

[ ... ]

> @@ -876,6 +889,260 @@ static int __maybe_unused c2c_he__prune_empty_writers(struct hist_entry *l1_he)

[ ... ]

> +static struct c2c_hist_entry *
> +c2c_child_entry__alloc(struct hist_entry *parent_he, struct hist_entry *src_he,
> +		       int depth, u64 ip)
> +{
> +	struct c2c_hist_entry *child_c2c;
> +	struct hist_entry *child_he;
> +
> +	/* Function-view children never own or display callchains. */
> +	child_he = c2c_function_he_zalloc(0);
> +	if (!child_he)
> +		return NULL;
> +

[Severity: Medium]
Can this regression cause a NULL pointer dereference?

This allocates a zeroed hist_entry but leaves the ops pointer uninitialized.

If core code ever iterates over the parent's hroot_out subtree and calls
hist_entry__delete on these children, it will unconditionally attempt to
execute ops->free(he).

> +	child_c2c = container_of(child_he, struct c2c_hist_entry, he);
> +	memcpy(&child_he->ms, &src_he->ms, sizeof(struct map_symbol));
> +
> +	if (src_he->mem_info) {
> +		child_he->mem_info = mem_info__clone(src_he->mem_info);
> +		if (!child_he->mem_info)
> +			goto out_free;
> +	}
> +
> +	child_he->thread = src_he->thread;

[Severity: Medium]
Does this code bypass reference count lifecycle helpers?

The map handle inside map_symbol is copied via memcpy, and the thread handle
is assigned via raw pointer assignment instead of using map__get and
thread__get.

If the core code ever iterates over these children and deletes them, it will
call thread__zput on borrowed references, leading to double-frees and
use-after-free crashes.

> +	child_he->cpumode = src_he->cpumode;
> +	child_he->cpu = src_he->cpu;

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=6