Re: [PATCH 2/4] perf: Assemble split thread maps in script and stat report

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] perf: NULL dereference on incomplete thread map when
  set_maps() is deferred
- [Low] perf stat: leak of perf_stat.threads and perf_stat.cpus in
  __cmd_report()
It also noticed 1 related pre-existing issue this patch does not
introduce:
- [Medium] perf stat: memory leaks of session, evlist, and maps in
  __cmd_report() [1]
--

commit 296ec71892d6fd09202f3090775a27df449bf978
Author: Leo Yan <leo.yan@arm.com>

perf: Assemble split thread maps in script and stat report

This commit adds thread_map__append_event() to assemble split
PERF_RECORD_THREAD_MAP records in perf script and perf stat report before
publishing the completed thread map to the evlist and allocating stats.

> diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c
> index b6691ebb1b4ef..e41925fb86b8d 100644
> --- a/tools/perf/builtin-script.c
> +++ b/tools/perf/builtin-script.c

[ ... ]

> @@ -3416,6 +3417,10 @@ static int __cmd_script(struct perf_script *script)
>  	}
>  
>  	ret = perf_session__process_events(script->session);
> +	if (!ret && !session_done() && script->threads_is_appending) {
> +		pr_err("Incomplete thread map: missing final record.\n");
> +		ret = -EINVAL;
> +	}
>  
>  	if (script->per_event_dump)
>  		perf_script__exit_per_event_dump_stats(script);
> @@ -3975,6 +3980,10 @@ static int set_maps(struct perf_script *script)
>  {
>  	struct evlist *evlist = script->session->evlist;
>  
> +	/* Appending can move the map, so share it only after the final record. */
> +	if (script->threads_is_appending)
> +		return 0;
> +
>  	if (!script->cpus || !script->threads)
>  		return 0;

[Severity: Medium]
Can deferring set_maps() while threads_is_appending is true cause a NULL
pointer dereference in perf_counts() when the final PERF_RECORD_THREAD_MAP
record is missing?

If a PERF_RECORD_THREAD_MAP record with PERF_RECORD_MISC_THREAD_MAP_MORE set
is followed by PERF_RECORD_CPU_MAP and stat events without the final thread
map record, set_maps() returns early and skips perf_evlist__set_maps() and
evlist__alloc_stats(), leaving counter->counts NULL on each evsel.  Because
threads_is_appending is only checked after perf_session__process_events()
returns, subsequent PERF_RECORD_STAT_ROUND or PERF_RECORD_STAT events are
processed first: process_stat_round_event() -> perf_stat_process_counter() ->
process_counter_maps() calls perf_counts(counter->counts, idx, thread) and
dereferences NULL, or perf_event__process_stat_event() fails earlier with an
"Invalid CPU -1" error that masks the incomplete thread map message.  The
same regression affects set_maps() and __cmd_report() in
tools/perf/builtin-stat.c.

> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> index 37752d6bfef45..19d120b647065 100644
> --- a/tools/perf/builtin-stat.c
> +++ b/tools/perf/builtin-stat.c

[ ... ]

> @@ -2301,15 +2306,19 @@ int process_thread_map_event(const struct perf_tool *tool,
>  			     union perf_event *event)
>  {
>  	struct perf_stat *st = container_of(tool, struct perf_stat, tool);
> +	int ret;
>  
> -	if (st->threads) {
> +	if (st->threads && !st->threads_is_appending) {
>  		pr_warning("Extra thread map event, ignoring.\n");
>  		return 0;
>  	}
>  
> -	st->threads = thread_map__new_event(&event->thread_map);
> -	if (!st->threads)
> -		return -ENOMEM;
> +	ret = thread_map__append_event(&st->threads, &event->thread_map);
> +	if (ret)
> +		return ret;
> +
> +	st->threads_is_appending =
> +		event->header.misc & PERF_RECORD_MISC_THREAD_MAP_MORE;
>  
>  	return set_maps(st);
>  }

[Severity: Low]
Does this leak st->threads when thread_map__append_event() fails partway
through or when the final thread map record is missing?

thread_map__append_event() stores the allocated map in st->threads before
duplicating the comm strings, and set_maps() keeps st->threads private while
st->threads_is_appending is true.  Unlike cmd_script(), which calls
perf_script__exit() to release script->threads and script->cpus,
__cmd_report() only calls perf_session__delete(session) and never calls
perf_thread_map__put(perf_stat.threads).

> @@ -2405,11 +2414,13 @@ static int __cmd_report(int argc, const char **argv)
>  	evsel_list         = session->evlist;
>  
>  	ret = perf_session__process_events(session);
> -	if (ret)
> -		return ret;
> +	if (!ret && !session_done() && perf_stat.threads_is_appending) {
> +		pr_err("Incomplete thread map: missing final record.\n");
> +		ret = -EINVAL;
> +	}
>  
>  	perf_session__delete(session);
> -	return 0;
> +	return ret;
>  }

[Severity: Medium]
This problem wasn't introduced by this patch, but does __cmd_report() leak
perf_stat.threads, perf_stat.cpus, evsel_list, and the aggregation maps
[1]?

cmd_stat() allocates evsel_list with evlist__new() and returns directly from
__cmd_report(), bypassing the cleanup at the end of cmd_stat().  If
perf_session__new() fails in __cmd_report(), returning early before
evlist__put(evsel_list) leaks the initial evsel_list.  During event
processing, set_maps() passes perf_stat.cpus and perf_stat.threads to
perf_evlist__set_maps(), which takes an extra reference on each map, and
process_stat_config_event() allocates aggregation maps;perf_session__delete()
only drops session->evlist's references and __cmd_report() never drops
perf_stat.threads, perf_stat.cpus, or the aggregation maps via
perf_stat__exit_aggr_mode().

[1] https://sashiko.dev/#/bug/linux-b77574e3-cc57-4338-8810-73a0cfa6fff7

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261115-perf_fix_large_threads-v1-0-69c9b86b9ae5@arm.com?part=2