Re: [PATCH v5 5/6] perf tools: Show memory region in perf-script subcommand

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Low] Full inclusion of `"session.h"` in a header file instead of using a forward declaration.

Pre-existing issues:
- [Critical] Unsafe accumulation of `i` and integer underflow on `sz` leading to out-of-bounds stack writes in `perf_mem__region_scnprintf`.
--

--- Patch [5]: [PATCH v5 5/6] perf tools: Show memory region in perf-script subcommand ---
commit bbe5b1b70fc8933143d1646f7f4d406b9364e4b3
Author: Dapeng Mi <[email protected]>

perf tools: Show memory region in perf-script subcommand

This patch adds support to show the memory region in the perf-script
subcommand. It extracts the region from the mem_region field of the memory
information data source, printing it when the appropriate feature bits or
memory ranges are present.

Link: https://lore.kernel.org/all/[email protected]/

> diff --git a/tools/perf/util/mem-events.c b/tools/perf/util/mem-events.c
> index 4fd48fd200552..8ce4996cad8d3 100644
> --- a/tools/perf/util/mem-events.c
> +++ b/tools/perf/util/mem-events.c
> @@ -604,8 +604,77 @@ int perf_mem__blk_scnprintf(char *out, size_t sz, const struct mem_info *mem_inf
>  	return l;
>  }
>  
> -int perf_script__meminfo_scnprintf(char *out, size_t sz, const struct mem_info *mem_info)
> +static int perf_mem__region_scnprintf(char *out, size_t sz, const struct mem_info *mem_info)
>  {
> +	size_t l = 0;
> +	u64 mem = PERF_MEM_REGION_NA;
> +
> +	sz -= 1; /* -1 for null termination */
> +	out[0] = '\0';

[Severity: Critical]
This is a pre-existing issue, but can this underflow and write out of bounds
if the accumulated length exceeds the buffer size?

If the preceding format functions in perf_script__meminfo_scnprintf() return a
combined length that exceeds the buffer size (sz = 200), sz - i will
underflow to a large unsigned value. When passed as sz into this function,
sz -= 1 and out[0] = '\0' will write a null byte and subsequent region string
out of bounds on the stack.

[ ... ]

> @@ -620,6 +689,21 @@ int perf_script__meminfo_scnprintf(char *out, size_t sz, const struct mem_info *
>  	i += perf_mem__lck_scnprintf(out + i, sz - i, mem_info);
>  	i += scnprintf(out + i, sz - i, "|BLK ");
>  	i += perf_mem__blk_scnprintf(out + i, sz - i, mem_info);
> +	if (session) {
> +		/*
> +		 * In case the feature bits are not available, as in
> +		 * pipe mode, fallback to checking for the existence of
> +		 * memory ranges
> +		 */
> +		env = perf_session__env(session);
> +		if ((env && session->data->is_pipe && env->nr_memory_ranges) ||
> +		    perf_header__has_feat(&session->header,
> +					  HEADER_MEMORY_RANGES)) {
> +			i += scnprintf(out + i, sz - i, "|Region ");
> +			i += perf_mem__region_scnprintf(out + i, sz - i,
> +							mem_info);

Should we verify that i < sz before computing sz - i and calling
perf_mem__region_scnprintf()?

> +		}
> +	}
>  
>  	return i;
>  }
> diff --git a/tools/perf/util/mem-events.h b/tools/perf/util/mem-events.h
> index daa22748f9fea..4ebb8109fc3c4 100644
> --- a/tools/perf/util/mem-events.h
> +++ b/tools/perf/util/mem-events.h
> @@ -4,6 +4,7 @@
>  
>  #include <stdbool.h>
>  #include <linux/types.h>
> +#include "session.h"

[Severity: Low]
Could we use a forward declaration like struct perf_session; here instead of
including the entire session.h header?

The header only needs the structure pointer handle for the prototype of
perf_script__meminfo_scnprintf(). Including the full header unnecessarily
increases compilation time and risk of namespace pollution.

>  
>  struct perf_mem_event {
>  	bool		supported;

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