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

Hello,

On Mon, Aug 10, 2026 at 05:49:44AM +0000, [email protected] wrote:
> 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()?

It looks like a real issue.  Can you please fix?  I'm not sure if c2c
uses the callchains.

Thanks,
Namhyung

> 
> > +
> > +	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