[PATCHES 0/8 v1] perf machine fixes

[PATCHES 0/8 v1] perf machine fixes

[Arnaldo Carvalho de Melo]

Hi,

    Here is a series of patches fixing issues with the 'struct machine'
methods, please consider applying to perf-tools-next.

- Arnaldo

Arnaldo Carvalho de Melo (8):
  perf machine: Fix fd leak on bounds check in maps__set_modules_path_dir()
  perf machine: Fix NULL parent dereference in fork event processing
  perf machine: Guard against NULL strlist in machines__findnew()
  perf machine: Check snprintf truncation in machines__findnew()
  perf machine: Don't abort guest map creation on first inaccessible dir
  perf machine: Reset errno before strtol in guest kernel map creation
  perf machine: Free scandir entries in guest kernel map creation
  perf machine: Check snprintf truncation for guest kallsyms path

 tools/perf/util/machine.c | 43 +++++++++++++++++++++++++++------------
 1 file changed, 30 insertions(+), 13 deletions(-)

-- 
2.55.0

[PATCH 1/8] perf machine: Fix fd leak on bounds check in maps__set_modules_path_dir()

[Arnaldo Carvalho de Melo]

From: Arnaldo Carvalho de Melo <acme@redhat.com>

The bounds check for root_len >= path_size returns -1 directly without
closing the directory fd opened by io_dir__init() a few lines above.

Jump to the out label instead, which calls close(iod.dirfd).

Fixes: e7af1946818b ("perf machine: Reuse module path buffer")
Reported-by: sashiko-bot <sashiko-bot@kernel.org>
Cc: Ian Rogers <irogers@google.com>
Assisted-by: Claude:claude-opus-4.6
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/machine.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
index 0d2ebf6a84bcf880..503f5a65e0cca639 100644
--- a/tools/perf/util/machine.c
+++ b/tools/perf/util/machine.c
@@ -1411,8 +1411,10 @@ static int maps__set_modules_path_dir(struct maps *maps, char *path, size_t path
 		return -1;
 	}
 	/* Bounds check, should never happen. */
-	if (root_len >= path_size)
-		return -1;
+	if (root_len >= path_size) {
+		ret = -1;
+		goto out;
+	}
 	path[root_len++] = '/';
 	while ((dent = io_dir__readdir(&iod)) != NULL) {
 		if (io_dir__is_dir(&iod, dent)) {
-- 
2.55.0

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

[Arnaldo Carvalho de Melo]

From: Arnaldo Carvalho de Melo <acme@redhat.com>

machine__process_fork_event() calls machine__findnew_thread() for the
parent thread, which can return NULL on allocation failure.  The code
then dereferences parent via thread__pid(parent) without a NULL check
when validating whether the parent PID matches.  The later NULL check
at thread__fork() does not prevent this earlier dereference.

Add a NULL guard before accessing the parent thread.

Fixes: 5cb73340d92a ("perf tools: Make fork event processing more resilient")
Reported-by: sashiko-bot <sashiko-bot@kernel.org>
Cc: Adrian Hunter <adrian.hunter@intel.com>
Assisted-by: Claude:claude-opus-4.6
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/machine.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
index 503f5a65e0cca639..8b213482e5648e97 100644
--- 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);
-- 
2.55.0

[PATCH 3/8] perf machine: Guard against NULL strlist in machines__findnew()

[Arnaldo Carvalho de Melo]

From: Arnaldo Carvalho de Melo <acme@redhat.com>

The static 'seen' strlist caches guestmount paths that have already
been reported as inaccessible, to avoid repeating the error message.
If strlist__new() fails (OOM), 'seen' stays NULL and the next call
dereferences it via strlist__has_entry() and strlist__add().

Guard both calls so that on allocation failure the error message is
still printed (just not deduplicated) instead of crashing.

Fixes: c80c3c269011 ("perf kvm: Limit repetitive guestmount message to once per directory")
Reported-by: sashiko-bot <sashiko-bot@kernel.org>
Cc: David Ahern <dsahern@gmail.com>
Assisted-by: Claude:claude-opus-4.6
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/machine.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
index 8b213482e5648e97..baf855e596c267cd 100644
--- a/tools/perf/util/machine.c
+++ b/tools/perf/util/machine.c
@@ -340,9 +340,10 @@ struct machine *machines__findnew(struct machines *machines, pid_t pid)
 			if (!seen)
 				seen = strlist__new(NULL, NULL);
 
-			if (!strlist__has_entry(seen, path)) {
+			if (!seen || !strlist__has_entry(seen, path)) {
 				pr_err("Can't access file %s\n", path);
-				strlist__add(seen, path);
+				if (seen)
+					strlist__add(seen, path);
 			}
 			machine = NULL;
 			goto out;
-- 
2.55.0

[PATCH 4/8] perf machine: Check snprintf truncation in machines__findnew()

[Arnaldo Carvalho de Melo]

From: Arnaldo Carvalho de Melo <acme@redhat.com>

The guestmount path is built with snprintf() into a PATH_MAX buffer
without checking the return value.  If symbol_conf.guestmount is long
enough to cause truncation, the truncated path could match a different
directory, causing the wrong guest to be associated with the pid.

Check for truncation and bail out early.

Fixes: a1645ce12adb ("perf: 'perf kvm' tool for monitoring guest performance from host")
Reported-by: sashiko-bot <sashiko-bot@kernel.org>
Cc: Zhang, Yanmin <yanmin_zhang@linux.intel.com>
Assisted-by: Claude:claude-opus-4.6
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/machine.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
index baf855e596c267cd..05724277c2a9753c 100644
--- a/tools/perf/util/machine.c
+++ b/tools/perf/util/machine.c
@@ -333,7 +333,12 @@ struct machine *machines__findnew(struct machines *machines, pid_t pid)
 	if ((pid != HOST_KERNEL_ID) &&
 	    (pid != DEFAULT_GUEST_KERNEL_ID) &&
 	    (symbol_conf.guestmount)) {
-		snprintf(path, sizeof(path), "%s/%d", symbol_conf.guestmount, pid);
+		if (snprintf(path, sizeof(path), "%s/%d",
+			     symbol_conf.guestmount, pid) >= (int)sizeof(path)) {
+			pr_err("Guest path too long for pid %d\n", pid);
+			machine = NULL;
+			goto out;
+		}
 		if (access(path, R_OK)) {
 			static struct strlist *seen;
 
-- 
2.55.0

[PATCH 5/8] perf machine: Don't abort guest map creation on first inaccessible dir

[Arnaldo Carvalho de Melo]

From: Arnaldo Carvalho de Melo <acme@redhat.com>

machines__create_guest_kernel_maps() jumps to the failure label when one
guest directory's kallsyms file fails access(), skipping all remaining
valid guest directories.  An inaccessible directory is not fatal — other
guests may still be reachable.

Replace 'goto failure' with 'continue' so the loop processes all
directories, and remove the now-unreferenced failure label.

Fixes: a1645ce12adb ("perf: 'perf kvm' tool for monitoring guest performance from host")
Reported-by: sashiko-bot <sashiko-bot@kernel.org>
Cc: Zhang, Yanmin <yanmin_zhang@linux.intel.com>
Assisted-by: Claude:claude-opus-4.6
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/machine.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
index 05724277c2a9753c..83d38637afe34fc9 100644
--- a/tools/perf/util/machine.c
+++ b/tools/perf/util/machine.c
@@ -1269,14 +1269,12 @@ int machines__create_guest_kernel_maps(struct machines *machines)
 			snprintf(path, sizeof(path), "%s/%s/proc/kallsyms",
 				 symbol_conf.guestmount,
 				 namelist[i]->d_name);
-			ret = access(path, R_OK);
-			if (ret) {
+			if (access(path, R_OK)) {
 				pr_debug("Can't access file %s\n", path);
-				goto failure;
+				continue;
 			}
 			machines__create_kernel_maps(machines, pid);
 		}
-failure:
 		free(namelist);
 	}
 
-- 
2.55.0

[PATCH 6/8] perf machine: Reset errno before strtol in guest kernel map creation

[Arnaldo Carvalho de Melo]

From: Arnaldo Carvalho de Melo <acme@redhat.com>

machines__create_guest_kernel_maps() checks errno == ERANGE after
strtol() to detect overflow, but does not clear errno first.  A stale
ERANGE from an earlier library call (e.g. scandir internals) causes
valid numeric directory names to be incorrectly skipped.

Set errno = 0 before strtol() so only the current conversion can
trigger the ERANGE check.

Fixes: a1645ce12adb ("perf: 'perf kvm' tool for monitoring guest performance from host")
Reported-by: sashiko-bot <sashiko-bot@kernel.org>
Cc: Zhang, Yanmin <yanmin_zhang@linux.intel.com>
Assisted-by: Claude:claude-opus-4.6
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/machine.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
index 83d38637afe34fc9..1700130adedb3a89 100644
--- a/tools/perf/util/machine.c
+++ b/tools/perf/util/machine.c
@@ -1258,6 +1258,7 @@ int machines__create_guest_kernel_maps(struct machines *machines)
 				/* Filter out . and .. */
 				continue;
 			}
+			errno = 0;
 			pid = (pid_t)strtol(namelist[i]->d_name, &endp, 10);
 			if ((*endp != '\0') ||
 			    (endp == namelist[i]->d_name) ||
-- 
2.55.0

[PATCH 7/8] perf machine: Free scandir entries in guest kernel map creation

[Arnaldo Carvalho de Melo]

From: Arnaldo Carvalho de Melo <acme@redhat.com>

machines__create_guest_kernel_maps() calls scandir() which allocates
both the namelist array and each individual dirent entry.  The code
frees the namelist array but not the individual entries, leaking memory
proportional to the number of directories under guestmount.

Free each namelist[i] after it is no longer needed.

Fixes: a1645ce12adb ("perf: 'perf kvm' tool for monitoring guest performance from host")
Reported-by: sashiko-bot <sashiko-bot@kernel.org>
Cc: Zhang, Yanmin <yanmin_zhang@linux.intel.com>
Assisted-by: Claude:claude-opus-4.6
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/machine.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
index 1700130adedb3a89..48c4b963e8097f4a 100644
--- a/tools/perf/util/machine.c
+++ b/tools/perf/util/machine.c
@@ -1256,6 +1256,7 @@ int machines__create_guest_kernel_maps(struct machines *machines)
 		for (i = 0; i < items; i++) {
 			if (!isdigit(namelist[i]->d_name[0])) {
 				/* Filter out . and .. */
+				free(namelist[i]);
 				continue;
 			}
 			errno = 0;
@@ -1265,6 +1266,7 @@ int machines__create_guest_kernel_maps(struct machines *machines)
 			    (errno == ERANGE)) {
 				pr_debug("invalid directory (%s). Skipping.\n",
 					 namelist[i]->d_name);
+				free(namelist[i]);
 				continue;
 			}
 			snprintf(path, sizeof(path), "%s/%s/proc/kallsyms",
@@ -1272,9 +1274,11 @@ int machines__create_guest_kernel_maps(struct machines *machines)
 				 namelist[i]->d_name);
 			if (access(path, R_OK)) {
 				pr_debug("Can't access file %s\n", path);
+				free(namelist[i]);
 				continue;
 			}
 			machines__create_kernel_maps(machines, pid);
+			free(namelist[i]);
 		}
 		free(namelist);
 	}
-- 
2.55.0

[PATCH 8/8] perf machine: Check snprintf truncation for guest kallsyms path

[Arnaldo Carvalho de Melo]

From: Arnaldo Carvalho de Melo <acme@redhat.com>

machines__create_guest_kernel_maps() builds the guest kallsyms path
with snprintf() without checking the return value.  A truncated path
could pass the access() check if a prefix directory happens to contain
a file named "kallsyms", leading to the wrong file being used for
symbol resolution.

Check for truncation and skip the directory.

Fixes: a1645ce12adb ("perf: 'perf kvm' tool for monitoring guest performance from host")
Reported-by: sashiko-bot <sashiko-bot@kernel.org>
Cc: Zhang, Yanmin <yanmin_zhang@linux.intel.com>
Assisted-by: Claude:claude-opus-4.6
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/machine.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
index 48c4b963e8097f4a..f86b3b7df742e0c1 100644
--- a/tools/perf/util/machine.c
+++ b/tools/perf/util/machine.c
@@ -1269,9 +1269,14 @@ int machines__create_guest_kernel_maps(struct machines *machines)
 				free(namelist[i]);
 				continue;
 			}
-			snprintf(path, sizeof(path), "%s/%s/proc/kallsyms",
-				 symbol_conf.guestmount,
-				 namelist[i]->d_name);
+			if (snprintf(path, sizeof(path), "%s/%s/proc/kallsyms",
+				     symbol_conf.guestmount,
+				     namelist[i]->d_name) >= (int)sizeof(path)) {
+				pr_debug("Guest kallsyms path too long for %s. Skipping.\n",
+					 namelist[i]->d_name);
+				free(namelist[i]);
+				continue;
+			}
 			if (access(path, R_OK)) {
 				pr_debug("Can't access file %s\n", path);
 				free(namelist[i]);
-- 
2.55.0

Re: [PATCH 6/8] perf machine: Reset errno before strtol in guest kernel map creation

[sashiko-bot]

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

Pre-existing issues:
- [Low] Missing explicit POSIX header inclusions for <stdio.h> and <limits.h>.
--

--- Patch [6]: [PATCH 6/8] perf machine: Reset errno before strtol in guest kernel map creation ---
commit 8cf9920f21a3b72d2b6dbb2087000cbeceb43bbb
Author: Arnaldo Carvalho de Melo <acme@redhat.com>

perf machine: Reset errno before strtol in guest kernel map creation

This commit resets errno to 0 before calling strtol() in
machines__create_guest_kernel_maps(). This prevents a stale ERANGE from
earlier library calls from causing valid directory names to be skipped.

> diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
> index 83d38637afe34..1700130adedb3 100644
> --- a/tools/perf/util/machine.c
> +++ b/tools/perf/util/machine.c

[Severity: Low]
This is a pre-existing issue, but is this file missing explicit POSIX header
inclusions for <stdio.h> and <limits.h>?

Looking at the include block at the top of tools/perf/util/machine.c:

// SPDX-License-Identifier: GPL-2.0
#include <dirent.h>
#include <errno.h>
#include <inttypes.h>
#include <regex.h>
#include <stdlib.h>
#include <string.h>
#include "callchain.h"

The code relies on PATH_MAX and snprintf() further down in
machines__create_guest_kernel_maps(). Relying on implicit header inclusions
works for glibc, but does it cause build failures under musl libc environments?

Could <stdio.h> and <limits.h> be explicitly included to ensure strict musl
compatibility?

> @@ -1258,6 +1258,7 @@ int machines__create_guest_kernel_maps(struct machines *machines)
>  				/* Filter out . and .. */
>  				continue;
>  			}
> +			errno = 0;
>  			pid = (pid_t)strtol(namelist[i]->d_name, &endp, 10);
>  			if ((*endp != '\0') ||
>  			    (endp == namelist[i]->d_name) ||

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

Re: [PATCH 5/8] perf machine: Don't abort guest map creation on first inaccessible dir

[sashiko-bot]

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

Pre-existing issues:
- [High] errno is checked for ERANGE after calling strtol() without explicitly clearing it to 0 beforehand, causing a logic error where one invalid directory skips all subsequent valid guest directories.
--

--- Patch [5]: [PATCH 5/8] perf machine: Don't abort guest map creation on first inaccessible dir ---
commit 3942aff28b363eb417be58b368d8c95bd54e8551
Author: Arnaldo Carvalho de Melo <acme@redhat.com>

perf machine: Don't abort guest map creation on first inaccessible dir

machines__create_guest_kernel_maps() is modified to use 'continue'
instead of jumping to a failure label when a guest directory's
kallsyms file is inaccessible. This allows the loop to process
remaining valid guest directories rather than aborting early.

> diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
> index 05724277c2a97..83d38637afe34 100644
> --- a/tools/perf/util/machine.c
> +++ b/tools/perf/util/machine.c
> @@ -1269,14 +1269,12 @@ int machines__create_guest_kernel_maps(struct machines *machines)

[Severity: High]
This is a pre-existing issue, but does this loop correctly handle errno
after a failed strtol() call?

Looking at machines__create_guest_kernel_maps(), the loop checks errno
for ERANGE after calling strtol(), but there doesn't appear to be an
explicit reset of errno beforehand:

tools/perf/util/machine.c:machines__create_guest_kernel_maps() {
    ...
    pid = (pid_t)strtol(namelist[i]->d_name, &endp, 10);
    if ((*endp != '\0') ||
        (endp == namelist[i]->d_name) ||
        (errno == ERANGE)) {
    ...
}

If a directory name causes an overflow (e.g., exceeding LONG_MAX), strtol()
sets errno to ERANGE. In the next loop iteration, if errno is not cleared
to 0, it will retain the ERANGE value. When strtol() processes a valid PID,
it succeeds, but the (errno == ERANGE) check evaluates to true, causing the
valid directory to be incorrectly skipped.

Since this loop processes directories from guestmount, could an unprivileged
user create a bogus directory to prevent symbol resolution for all subsequent
guests?

>  			snprintf(path, sizeof(path), "%s/%s/proc/kallsyms",
>  				 symbol_conf.guestmount,
>  				 namelist[i]->d_name);
> -			ret = access(path, R_OK);
> -			if (ret) {
> +			if (access(path, R_OK)) {
>  				pr_debug("Can't access file %s\n", path);
> -				goto failure;
> +				continue;
>  			}
>  			machines__create_kernel_maps(machines, pid);
>  		}
> -failure:
>  		free(namelist);
>  	}

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

Re: [PATCH 7/8] perf machine: Free scandir entries in guest kernel map creation

[sashiko-bot]

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

Pre-existing issues:
- [High] Crash during cleanup if `maps__insert()` fails when creating kernel maps.
--

--- Patch [7]: [PATCH 7/8] perf machine: Free scandir entries in guest kernel map creation ---
commit 39417761e2f7dcdcc875be7b6d926b0a7b9bade6
Author: Arnaldo Carvalho de Melo <acme@redhat.com>

perf machine: Free scandir entries in guest kernel map creation

machines__create_guest_kernel_maps() calls scandir() which allocates
both the namelist array and each individual dirent entry.  The code
frees the namelist array but not the individual entries, leaking memory
proportional to the number of directories under guestmount.

> diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
> index 1700130adedb3..48c4b963e8097 100644
> --- a/tools/perf/util/machine.c
> +++ b/tools/perf/util/machine.c

[ ... ]

> @@ -1265,6 +1266,7 @@ int machines__create_guest_kernel_maps(struct machines *machines)
>  			    (errno == ERANGE)) {
>  				pr_debug("invalid directory (%s). Skipping.\n",
>  					 namelist[i]->d_name);
> +				free(namelist[i]);
>  				continue;
>  			}
>  			snprintf(path, sizeof(path), "%s/%s/proc/kallsyms",
> @@ -1272,9 +1274,11 @@ int machines__create_guest_kernel_maps(struct machines *machines)
>  				 namelist[i]->d_name);
>  			if (access(path, R_OK)) {
>  				pr_debug("Can't access file %s\n", path);
> +				free(namelist[i]);
>  				continue;
>  			}
>  			machines__create_kernel_maps(machines, pid);

[Severity: High]
This is a pre-existing issue, but since we are executing the guest kernel
map creation path here, could this lead to an out-of-bounds memory access if
map insertion fails?

If machines__create_kernel_maps() ends up calling
__machine__create_kernel_maps() and maps__insert() fails:

tools/perf/util/machine.c:__machine__create_kernel_maps() {
    ...
    machine->vmlinux_map = map__new2(0, kernel);
    if (machine->vmlinux_map == NULL)
        return -ENOMEM;

    map__set_mapping_type(machine->vmlinux_map, MAPPING_TYPE__IDENTITY);
    return maps__insert(machine__kernel_maps(machine), machine->vmlinux_map);
}

A memory allocation failure in maps__insert() will return an error code, but
leave the uninserted map attached to machine->vmlinux_map. Later, during
teardown in machine__destroy_kernel_maps(), this dangling map is passed to
maps__remove().

Since it was never inserted, maps__by_address_index() in __maps__remove()
returns -1 (UINT_MAX):

tools/perf/util/maps.c:__maps__remove() {
    ...
    address_idx = maps__by_address_index(maps, map);
    map__put(maps_by_address[address_idx]);
    memmove(&maps_by_address[address_idx],
            &maps_by_address[address_idx + 1],
    ...
}

Does this use the UINT_MAX index directly to access and memmove the
maps_by_address array, causing an out-of-bounds memory access and potential
memory corruption during cleanup?

> +			free(namelist[i]);
>  		}
>  		free(namelist);
>  	}

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

Re: [PATCH 4/8] perf machine: Check snprintf truncation in machines__findnew()

[sashiko-bot]

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

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but missing NULL check for `machine` in event delivery causes NULL pointer dereference for non-SAMPLE events when guest machine creation fails.
--

--- Patch [4]: [PATCH 4/8] perf machine: Check snprintf truncation in machines__findnew() ---
commit 6d258a0499e2df813b5de954dd914c53ea5c78dc
Author: Arnaldo Carvalho de Melo <acme@redhat.com>

perf machine: Check snprintf truncation in machines__findnew()

This commit adds a truncation check to the guestmount path built with
snprintf(). If truncation occurs, it bails out early by setting the machine
pointer to NULL and returning.

> diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
> index baf855e596c26..05724277c2a97 100644
> --- a/tools/perf/util/machine.c
> +++ b/tools/perf/util/machine.c
> @@ -333,7 +333,12 @@ struct machine *machines__findnew(struct machines *machines, pid_t pid)
>  	if ((pid != HOST_KERNEL_ID) &&
>  	    (pid != DEFAULT_GUEST_KERNEL_ID) &&
>  	    (symbol_conf.guestmount)) {
> -		snprintf(path, sizeof(path), "%s/%d", symbol_conf.guestmount, pid);
> +		if (snprintf(path, sizeof(path), "%s/%d",
> +			     symbol_conf.guestmount, pid) >= (int)sizeof(path)) {
> +			pr_err("Guest path too long for pid %d\n", pid);
> +			machine = NULL;
> +			goto out;

[Severity: High]
This isn't a bug introduced by this patch, but does returning NULL here
cause a NULL pointer dereference later in the event delivery pipeline?

When machines__findnew() returns NULL because of the truncated guestmount
path, the NULL propagates to machines__deliver_event(). For non-SAMPLE
events, it appears machine is passed to tool callbacks without validation:

tools/perf/util/session.c:machines__deliver_event() {
    ...
    return tool->mmap2(tool, event, sample, machine);
}

This flows into the callback and dereferences the NULL pointer:

tools/perf/util/machine.c:machine__process_mmap2_event() {
    ...
    ret = machine__process_kernel_mmap_event(machine, &xm, &dso_id.build_id);
}

tools/perf/util/machine.c:machine__process_kernel_mmap_event() {
    ...
    const char *mmap_name = machine->mmap_name;
}

Can we safely check for a NULL machine in machines__deliver_event() for
these non-SAMPLE events to prevent a segmentation fault?

> +		}
>  		if (access(path, R_OK)) {
>  			static struct strlist *seen;

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

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

[sashiko-bot]

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