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

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Deleting a function-view child entry causes an out-of-bounds heap read in `free_callchain()` because the child entry is allocated without trailing `callchain_root` memory.
--

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

perf c2c: add function view hierarchy entry creation
    
This patch adds the entry-creation layer for the function view hierarchy.
It implements level-1/2/3 lookup-or-create functions and synthetic child
allocation logic, reusing cacheline_idx to preserve source entry identity.

> diff --git a/tools/perf/ui/browsers/c2c-function.c b/tools/perf/ui/browsers/c2c-function.c
> index 4bf8406fde556..4099d4c7e7b44 100644
> --- a/tools/perf/ui/browsers/c2c-function.c
> +++ b/tools/perf/ui/browsers/c2c-function.c

[ ... ]

> @@ -847,6 +865,259 @@ static int __maybe_unused c2c_he__prune_empty_writers(struct hist_entry *l1_he)
>  	return surviving;
>  }
>  
> +static void *c2c_function_he_zalloc(size_t size)
> +{
> +	struct c2c_hist_entry *c2c_he = zalloc(sizeof(*c2c_he) + size);
> +
> +	if (!c2c_he)
> +		return NULL;
> +
> +	init_stats(&c2c_he->cstats.lcl_hitm);
> +	init_stats(&c2c_he->cstats.rmt_hitm);
> +	init_stats(&c2c_he->cstats.lcl_peer);
> +	init_stats(&c2c_he->cstats.rmt_peer);
> +	init_stats(&c2c_he->cstats.load);
> +
> +	return &c2c_he->he;
> +}
> +
> +/* Entry operations for function view */
> +static struct hist_entry_ops c2c_function_entry_ops = {
> +	.new	= c2c_function_he_zalloc,
> +	.free	= c2c_function_he_free,
> +};
> +
> +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]
Will this cause an out-of-bounds memory read when the entry is freed?

If perf c2c report is run with callchains enabled (symbol_conf.use_callchain
is true), this entry is allocated with 0 extra bytes for the callchain.

When this undersized entry is deleted on the error path below or during
cleanup via c2c_he__free_hierarchy(), hist_entry__delete() accesses
he->callchain based on the global symbol_conf.use_callchain flag:

tools/perf/util/hist.c:hist_entry__delete() {
    ...
    if (symbol_conf.use_callchain)
        free_callchain(he->callchain);
    ...
}

Could this read past the end of the c2c_hist_entry structure and dereference
garbage memory in free_callchain_node()?

> +
> +	child_c2c = container_of(child_he, struct c2c_hist_entry, he);
> +	child_he->ops = &c2c_function_entry_ops;
> +	map_symbol__copy(&child_he->ms, &src_he->ms);

[ ... ]

> +	return child_c2c;
> +
> +out_free:
> +	hist_entry__delete(child_he);
> +	return NULL;
> +}

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