Re: [PATCH] perf build-id: Fix wrong return value checking

[sashiko-bot@kernel.org]

Sashiko AI review found 2 potential issue(s):
- [High] Missing O_NONBLOCK during build-ID extraction enables indefinite hangs on FIFOs via TOCTOU race.
- [Medium] The strict length check rejects valid 16-byte build IDs (e.g., MD5/UUID), causing `perf buildid-list` to fail for them.
--

commit 7e5fc011b65e40f9f999c08196173059dcbc7f79
Author: Namhyung Kim <namhyung@kernel.org>

perf build-id: Fix wrong return value checking

This commit adjusts the return value checks of the build-id snprintf
functions to correctly account for the terminating NUL byte when comparing
lengths. This ensures perf buildid-list properly displays build IDs instead
of returning errors.

> diff --git a/tools/perf/builtin-buildid-list.c b/tools/perf/builtin-buildid-list.c
> --- a/tools/perf/builtin-buildid-list.c
> +++ b/tools/perf/builtin-buildid-list.c
> @@ -60,8 +60,8 @@ static int sysfs__fprintf_build_id(FILE *fp)
>  	char sbuild_id[SBUILD_ID_SIZE];
>  	int ret;
>  
>  	ret = sysfs__snprintf_build_id("/", sbuild_id, sizeof(sbuild_id));
> -	if (ret != sizeof(sbuild_id))
> +	if (ret != sizeof(sbuild_id) - 1)

Could this strict length check reject valid 16-byte build IDs?

The kernel and perf subsystem natively support shorter 16-byte build IDs
like MD5 or UUID. For these, sysfs__snprintf_build_id() would correctly
return 32.

Since 32 does not equal sizeof(sbuild_id) - 1 (which evaluates to 40), will
this erroneously return -EINVAL and cause perf buildid-list to silently
ignore valid kernel maps and binaries?

>  		return ret < 0 ? ret : -EINVAL;
>  
>  	return fprintf(fp, "%s\n", sbuild_id);
>  }

[ ... ]

> @@ -72,8 +72,8 @@ static int filename__fprintf_build_id(const char *name, FILE *fp)
>  	char sbuild_id[SBUILD_ID_SIZE];
>  	int ret;
>  
>  	ret = filename__snprintf_build_id(name, sbuild_id, sizeof(sbuild_id));

Does the underlying build-ID extraction sequence miss an O_NONBLOCK flag,
potentially allowing indefinite hangs?

When filename__snprintf_build_id() processes user-supplied file paths, the
underlying open() call happens without O_NONBLOCK. 

While there is an is_regular_file() check beforehand, could a time-of-check
to time-of-use race allow swapping the file with a FIFO or blocking device
immediately after the check, causing the tool to hang indefinitely?

> -	if (ret != sizeof(sbuild_id))
> +	if (ret != sizeof(sbuild_id) - 1)
>  		return ret < 0 ? ret : -EINVAL;
>  
>  	return fprintf(fp, "%s\n", sbuild_id);

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