Re: [PATCH v1 03/13] perf top: Fix sb_evlist leak

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Error path in `record__setup_sb_evlist()` bypasses state update, leading to a segmentation fault from an uninitialized `pthread_join()`.
- [High] Sideband thread and `sb_evlist` are leaked when using `--switch-output-event` together with `--no-bpf-event`.
--

commit 9bc374ffd1b7ae00f5342c051ca479b32bc92942
Author: Ian Rogers <irogers@google.com>

perf top: Fix sb_evlist leak

Fix a memory leak in cmd_top() where top.sb_evlist was not freed if
evlist__add_bpf_sb_event() fails. Note that evlist__start_sb_thread() and
evlist__stop_sb_thread() take ownership of the evlist and free it, so
we must only free it if we fail before starting the thread.

> diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
> index 5933c46ee137e..abc74b8ab7599 100644
> --- a/tools/perf/builtin-top.c
> +++ b/tools/perf/builtin-top.c
> @@ -1881,6 +1881,7 @@ int cmd_top(int argc, const char **argv)
>  		if (evlist__add_bpf_sb_event(top.sb_evlist, &host_env)) {
>  			pr_err("Couldn't ask for PERF_RECORD_BPF_EVENT side band events.\n.");
>  			status = -EINVAL;
> +			evlist__put(top.sb_evlist);
>  			goto out_put_evlist;
>  		}
>  	}

[Severity: High]
This is a pre-existing issue, but does tools/perf/builtin-record.c have a
similar problem where the error path in record__setup_sb_evlist() bypasses
updating the state?

If evlist__add_bpf_sb_event() fails in record__setup_sb_evlist(), it
returns -1 without updating the opts->no_bpf_event flag:

tools/perf/builtin-record.c:record__setup_sb_evlist() {
    ...
	if (evlist__add_bpf_sb_event(rec->sb_evlist, perf_session__env(rec->session))) {
		pr_err("Couldn't ask for PERF_RECORD_BPF_EVENT side band events.\n.");
		return -1;
	}
    ...
}

During cleanup in __cmd_record(), the unmodified no_bpf_event flag causes
evlist__stop_sb_thread() to be unconditionally invoked:

tools/perf/builtin-record.c:__cmd_record() {
    ...
	if (!opts->no_bpf_event)
		evlist__stop_sb_thread(rec->sb_evlist);
    ...
}

This could blindly call pthread_join() on the uninitialized thread ID in
evlist__stop_sb_thread():

tools/perf/util/sideband_evlist.c:evlist__stop_sb_thread() {
    ...
	pthread_join(*evlist__sb_thread_th(evlist), NULL);
    ...
}

Could this trigger undefined behavior and a crash?

[Severity: High]
This is also a pre-existing issue, but is there a leak of the sideband
thread and sb_evlist in tools/perf/builtin-record.c when using
--switch-output-event together with --no-bpf-event?

In record__setup_sb_evlist(), the sideband thread is started when using
--switch-output-event, even if BPF events are disabled:

tools/perf/builtin-record.c:record__setup_sb_evlist() {
    ...
	if (evlist__start_sb_thread(rec->sb_evlist, &rec->opts.target)) {
    ...
}

However, cleanup in __cmd_record() is bypassed because opts->no_bpf_event
evaluates to true:

tools/perf/builtin-record.c:__cmd_record() {
    ...
	if (!opts->no_bpf_event)
		evlist__stop_sb_thread(rec->sb_evlist);
    ...
}

Does this permanently strand the background poll thread and leak the
allocated evlist during process teardown?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260623053515.2677888-1-irogers@google.com?part=3