Re: [PATCH 2/8] perf machine: Fix NULL parent dereference in fork event processing

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Uninitialized pointer dereference in `threads__remove`.
- [Medium] TOCTOU / Key-value mismatch in `threads__remove` deletes the wrong thread.
--

--- Patch [2]: [PATCH 2/8] perf machine: Fix NULL parent dereference in fork event processing ---
commit 44c208f6def23d87c4b23fe6a8843f00fca91108
Author: Arnaldo Carvalho de Melo <acme@redhat.com>

perf machine: Fix NULL parent dereference in fork event processing

This commit fixes a NULL pointer dereference when processing fork events
by adding a NULL guard before accessing the parent thread.

> diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
> --- a/tools/perf/util/machine.c
> +++ b/tools/perf/util/machine.c
> @@ -1923,7 +1923,8 @@ int machine__process_fork_event(struct machine *machine, union perf_event *event
>  	 * (fork) event that would have removed the thread was lost. Assume the
>  	 * latter case and continue on as best we can.
>  	 */
> -	if (thread__pid(parent) != (pid_t)event->fork.ppid) {
> +	if (parent != NULL &&
> +	    thread__pid(parent) != (pid_t)event->fork.ppid) {
>  		dump_printf("removing erroneous parent thread %d/%d\n",
>  			    thread__pid(parent), thread__tid(parent));
>  		machine__remove_thread(machine, parent);

[Severity: High]
This isn't a bug introduced by this patch, but looking at how
machine__remove_thread() eventually calls into threads__remove(), is it
possible for thread__put() to dereference an uninitialized pointer?

In tools/perf/util/threads.c:threads__remove():

struct thread *old_value;
...
hashmap__delete(&table->shard, thread__tid(thread), NULL, &old_value);
thread__put(old_value);

If the thread's TID is not found in the hashmap, hashmap__delete() returns
false and leaves old_value uninitialized. Since the return value is not
checked, thread__put(old_value) will unconditionally execute with whatever
data was left on the stack.

Could this happen during malformed event processing here in
machine__process_fork_event() if thread recreation fails due to memory
pressure, and a subsequent thread removal fails to find the TID in the
hashmap?

[Severity: Medium]
This is also a pre-existing issue, but looking at the same deletion logic
in tools/perf/util/threads.c:threads__remove():

hashmap__delete(&table->shard, thread__tid(thread), NULL, &old_value);

Does this unconditionally delete whatever entry currently matches the TID,
without verifying that the stored value actually matches the thread pointer
passed in?

If machine__process_fork_event() encounters a malformed event where the
TID matches the parent TID but the PIDs differ, the parent is replaced.
Could a subsequent removal of the child thread incorrectly delete this
newly created parent thread from the hashmap instead of failing gracefully?

> 		thread__put(parent);
> 		parent = machine__findnew_thread(machine, event->fork.ppid,
> 						 event->fork.ptid);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260726234014.63111-1-acme@kernel.org?part=2