Re: [PATCH 06/15] perf dso: Set error code when open() fails on uncompressed fallback path

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Type punning by accessing a char array through an incompatible pointer type in tools/.
--

commit 4b3994ef70950d73fb48b1cdff6e7ffb7ee129d2
Author: Arnaldo Carvalho de Melo <acme@redhat.com>

perf dso: Set error code when open() fails on uncompressed fallback path

filename__decompress() has an early return for files that are not
actually compressed, where it calls open() directly. When open()
fails, the function returns -1 but never sets *err, leading to a stale
or zero errno. This causes callers to treat fd 0 as a valid DSO file
descriptor.

> diff --git a/tools/perf/util/symbol-minimal.c b/tools/perf/util/symbol-minimal.c
> index 091071d06416e..ea2de3d50d33c 100644
> --- a/tools/perf/util/symbol-minimal.c
> +++ b/tools/perf/util/symbol-minimal.c

[ ... ]

> @@ -217,29 +220,36 @@ int sysfs__read_build_id(const char *filename, struct build_id *bid)
>  {
>  	int fd;
>  	int ret = -1;
> -	struct stat stbuf;
> -	size_t buf_size;
> -	void *buf;
> +	/*
> +	 * read_build_id() casts this buffer to a u32 note header pointer,
> +	 * so it must be aligned for u32 access.
> +	 *
> +	 * Accessing a char[] through a u32* is technically type-punning
> +	 * under C strict aliasing rules, but perf unconditionally builds
> +	 * with -fno-strict-aliasing (Makefile.config), so this is safe.
> +	 *
> +	 * This file is only compiled when libelf is not available — the
> +	 * common case uses the libelf-based path in symbol-elf.c instead.
> +	 */
> +	char buf[BUFSIZ] __aligned(4);

[Severity: Medium]
The comment mentions that perf unconditionally builds with
-fno-strict-aliasing, making this safe. However, do files under the tools/
directory generally assume standard -fstrict-aliasing?

When this character array is passed to read_build_id() and cast to an
incompatible structure pointer type:

sysfs__read_build_id()->read_build_id() {
	nhdr = ptr;
	if (need_swap) {
		nhdr->n_namesz = bswap_32(nhdr->n_namesz);

Could this type punning lead to undefined behavior or compiler optimization
issues if standard compiler flags are used for tools?

> +	ssize_t len;
>  
>  	fd = open(filename, O_RDONLY);
>  	if (fd < 0)
>  		return -1;
>  
> -	if (fstat(fd, &stbuf) < 0)
> -		goto out;
> +	/*
> +	 * Use a fixed buffer and a single read() instead of fstat() + malloc(),
> +	 * because sysfs pseudo-files often report st_size=0 or 4096
> +	 * regardless of actual content size.
> +	 *
> +	 * BUFSIZ (8192) is more than sufficient: a sysfs build-id note is
> +	 * ~36 bytes (12-byte note header + 4-byte "GNU" name + 20-byte SHA1).
> +	 */
> +	len = read(fd, buf, sizeof(buf));
> +	if (len > 0)
> +		ret = read_build_id(buf, len, bid, false);
>  
> -	buf_size = stbuf.st_size;
> -	buf = malloc(buf_size);
> -	if (buf == NULL)
> -		goto out;
> -
> -	if (read(fd, buf, buf_size) != (ssize_t) buf_size)
> -		goto out_free;
> -
> -	ret = read_build_id(buf, buf_size, bid, false);
> -out_free:
> -	free(buf);
> -out:
>  	close(fd);
>  	return ret;
>  }

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