Re: [PATCH 2/9] perf machine: Use snprintf() for guestmount path construction

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Unconditional memory leak of `dirent` elements returned by `scandir()`.
- [High] NULL pointer dereference when `strlist__new()` fails.
- [High] NULL pointer dereference when `machine__findnew_thread()` fails to allocate a parent thread.
- [Medium] Loop iteration logic bug causes silent skipping of valid guest VM directories if one directory is inaccessible.
- [Medium] File descriptor leak on an early error return during nested directory processing.
- [Medium] Improper error handling missing `errno = 0` before `strtol()` can cause valid PIDs to be skipped.
--

commit 02cca33a0758d115755760c916ffc005383e2736
Author: Arnaldo Carvalho de Melo <acme@redhat.com>

perf machine: Use snprintf() for guestmount path construction

This commit replaces sprintf() with snprintf() when constructing guestmount
paths. This prevents potential stack buffer overflows caused by unbounded
user-provided guestmount lengths.

> diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
> index 7cf20ace3dbb5..fb960fda55f4f 100644
> --- a/tools/perf/util/machine.c
> +++ b/tools/perf/util/machine.c
> @@ -334,7 +334,7 @@ struct machine *machines__findnew(struct machines *machines, pid_t pid)
>  	if ((pid != HOST_KERNEL_ID) &&
>  	    (pid != DEFAULT_GUEST_KERNEL_ID) &&
>  	    (symbol_conf.guestmount)) {
> -		sprintf(path, "%s/%d", symbol_conf.guestmount, pid);
> +		snprintf(path, sizeof(path), "%s/%d", symbol_conf.guestmount, pid);
>  		if (access(path, R_OK)) {
>  			static struct strlist *seen;

[Severity: High]
This is a pre-existing issue, but can this cause a null pointer dereference
if strlist__new() fails? 

Looking just below this in machines__findnew():

			if (!seen)
				seen = strlist__new(NULL, NULL);

			if (!strlist__has_entry(seen, path)) {

If memory allocation fails, seen becomes null, and it is immediately passed
to strlist__has_entry() which might try to dereference it.

[ ... ]
> @@ -1261,9 +1261,9 @@ int machines__create_guest_kernel_maps(struct machines *machines)
>  					 namelist[i]->d_name);
>  				continue;
>  			}
> -			sprintf(path, "%s/%s/proc/kallsyms",
> -				symbol_conf.guestmount,
> -				namelist[i]->d_name);
> +			snprintf(path, sizeof(path), "%s/%s/proc/kallsyms",
> +				 symbol_conf.guestmount,
> +				 namelist[i]->d_name);
>  			ret = access(path, R_OK);
>  			if (ret) {
>  				pr_debug("Can't access file %s\n", path);

[Severity: Medium]
This is a pre-existing issue, but should this loop continue instead of
failing completely? Looking just below this in
machines__create_guest_kernel_maps(), the code jumps to the failure label:

				goto failure;
			}
			machines__create_kernel_maps(machines, pid);

By jumping to the failure label, it seems we skip processing all remaining
valid guest directories if just one is inaccessible.

While reviewing this file, I noticed a few other pre-existing issues:

[Severity: Medium]
This is a pre-existing issue, but does this correctly handle errno?

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)) {
...
}

The code checks if errno is ERANGE, but errno isn't set to 0 before calling
strtol(). Could a previous operation leave errno as ERANGE, causing valid
PIDs to be skipped here?

[Severity: High]
This is a pre-existing issue, but does this leak the individual directory
entries returned by scandir()?

tools/perf/util/machine.c:machines__create_guest_kernel_maps() {
...
failure:
		free(namelist);
	}

	return ret;
}

The namelist array is freed here, but it looks like the individual struct
dirent pointers allocated by scandir() and stored in namelist[i] are never
freed before the array itself is freed.

[Severity: High]
This is a pre-existing issue, but in machine__process_fork_event(), is it
possible to dereference a null pointer if thread allocation fails?

tools/perf/util/machine.c:machine__process_fork_event() {
...
	struct thread *parent = machine__findnew_thread(machine,
							event->fork.ppid,
							event->fork.ptid);
	...
	if (thread__pid(parent) != (pid_t)event->fork.ppid) {
...
}

If machine__findnew_thread() returns null due to memory pressure, it seems
the pointer is accessed without a check.

[Severity: Medium]
This is a pre-existing issue, but does maps__set_modules_path_dir() leak a
file descriptor on an early return?

tools/perf/util/machine.c:maps__set_modules_path_dir() {
...
	io_dir__init(&iod, open(path, O_CLOEXEC | O_DIRECTORY | O_RDONLY));
	...
	/* Bounds check, should never happen. */
	if (root_len >= path_size)
		return -1;
...
}

If the bounds check fails, the function returns immediately and skips the
close() call for the directory file descriptor.

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