Re: [PATCH v6 6/6] perf c2c: print memory region data with stdio output

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The dynamically allocated string `output_str` is leaked on intermediate error paths.
- [Low] The new function `print_memory_ranges_info` relies on standard I/O structures and functions (`FILE`, `fprintf`) but fails to explicitly include `<stdio.h>`, violating musl compatibility guidelines.
--

commit 56f9a74e597ecb10ca78e8a87ceec53476ce6327
Author: Thomas Falcon <thomas.falcon@intel.com>

perf c2c: print memory region data with stdio output

This commit adds memory range printing to the perf c2c subcommand. It outputs
memory range data only in stdio mode currently, printing nothing if ranges are
unsupported or not present.

> diff --git a/tools/perf/builtin-c2c.c b/tools/perf/builtin-c2c.c
> index 715b75d42f2a8..da669136e5452 100644
> --- a/tools/perf/builtin-c2c.c
> +++ b/tools/perf/builtin-c2c.c

[ ... ]

> @@ -2530,6 +2580,34 @@ static void print_c2c_info(FILE *out, struct perf_session *session)
>  	fprintf(out, "  Cacheline data grouping           : %s\n", c2c.cl_sort);
>  }
>  
> +
> +static void print_memory_ranges_info(FILE *out, struct perf_session *session)
> +{
> +	struct perf_env *env = perf_session__env(session);
> +	int nr_ranges = 0;
> +
> +	if (!perf_header__has_feat(&session->header, HEADER_MEMORY_RANGES))
> +		return;
> +	nr_ranges = env->nr_memory_ranges;
> +	if (nr_ranges == 0) {
> +		pr_debug("No memory ranges found, skipping\n");
> +		return;
> +	}
> +
> +	fprintf(out, "\n");

[Severity: Low]
Does this new function miss the required standard I/O header inclusion? 

The print_memory_ranges_info() function directly relies on FILE and fprintf(),
but the file does not explicitly include <stdio.h>. Can this omission cause a
build failure under musl?

> +	fprintf(out, "=================================================\n");
> +	fprintf(out, "                  Memory Ranges                  \n");
> +	fprintf(out, "=================================================\n");

[ ... ]

> @@ -3271,9 +3355,16 @@ static int perf_c2c__report(int argc, const char **argv)
>  		goto out_mem2node;
>  	}
>  
> -	if (c2c.display != DISPLAY_SNP_PEER)
> -		output_str = "cl_idx,"
> +	c2c.show_mem_region = perf_header__has_feat(&session->header,
> +						HEADER_MEMORY_RANGES);
> +	if (c2c.show_mem_region)
> +		dim_dcacheline.header.line[0].span = 3;
> +
> +	if (c2c.display != DISPLAY_SNP_PEER) {
> +		if (asprintf(&output_str,
>  			     "dcacheline,"
> +			     "%s"
>  			     "dcacheline_node,"

[ ... ]

> @@ -3329,11 +3433,13 @@ static int perf_c2c__report(int argc, const char **argv)
>  
>  	if (ui_quirks()) {
>  		pr_err("failed to setup UI\n");
> -		goto out_mem2node;
> +		goto out_str;
>  	}

[Severity: Medium]
Does this code leak the newly allocated output_str on intermediate error paths?

The string output_str is now dynamically allocated using asprintf(), but some
error paths in perf_c2c__report() still jump to out_mem2node instead of
out_str, bypassing the newly added free(output_str) cleanup.

For example, when c2c_hists__reinit() fails:

	err = c2c_hists__reinit(&c2c.hists, output_str, sort_str, perf_session__env(session));
	if (err) {
		pr_err("Failed to reinitialize hists\n");
		goto out_mem2node;
	}

And similarly when hists__iterate_cb() fails:

	err = hists__iterate_cb(&c2c.hists.hists, resort_cl_cb, perf_session__env(session));
	if (err)
		goto out_mem2node;

Should these jumps be updated to out_str to prevent the memory leak?

>  
>  	perf_c2c_display(session);
>  
> +out_str:
> +	free(output_str);
>  out_mem2node:
>  	mem2node__exit(&c2c.mem2node);
>  out_session:

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260831204613.2793526-1-thomas.falcon@intel.com?part=6