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

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

[Namhyung Kim]

The {sysfs,filename}__snprintf_build_id() both return the actual length
of build-ID.  But in their fprintf counterparts check with sbuild_id
length which contain a terminating NUL bytes.  So they cannot match.

This resulted in perf buildid-list -k prints nothing.  With this fix,
it can show the following result.

  $ perf buildid-list -k
  9524074000cb36cf2559a5b52f0555dab47da553

Fixes: fccaaf6fbbc59910e ("perf build-id: Change sprintf functions to snprintf")
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/builtin-buildid-list.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/perf/builtin-buildid-list.c b/tools/perf/builtin-buildid-list.c
index a91bbb34ac946360..8ad39ff9a4b8a4b4 100644
--- a/tools/perf/builtin-buildid-list.c
+++ b/tools/perf/builtin-buildid-list.c
@@ -61,7 +61,7 @@ static int sysfs__fprintf_build_id(FILE *fp)
 	int ret;
 
 	ret = sysfs__snprintf_build_id("/", sbuild_id, sizeof(sbuild_id));
-	if (ret != sizeof(sbuild_id))
+	if (ret != sizeof(sbuild_id) - 1)
 		return ret < 0 ? ret : -EINVAL;
 
 	return fprintf(fp, "%s\n", sbuild_id);
@@ -73,7 +73,7 @@ static int filename__fprintf_build_id(const char *name, FILE *fp)
 	int ret;
 
 	ret = filename__snprintf_build_id(name, sbuild_id, sizeof(sbuild_id));
-	if (ret != sizeof(sbuild_id))
+	if (ret != sizeof(sbuild_id) - 1)
 		return ret < 0 ? ret : -EINVAL;
 
 	return fprintf(fp, "%s\n", sbuild_id);
-- 
2.54.0.rc1.555.g9c883467ad-goog

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

[sashiko-bot]

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

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

[James Clark]

On 20/04/2026 19:08, Namhyung Kim wrote:
> The {sysfs,filename}__snprintf_build_id() both return the actual length
> of build-ID.  But in their fprintf counterparts check with sbuild_id
> length which contain a terminating NUL bytes.  So they cannot match.
> 
> This resulted in perf buildid-list -k prints nothing.  With this fix,
> it can show the following result.
> 
>    $ perf buildid-list -k
>    9524074000cb36cf2559a5b52f0555dab47da553
> 
> Fixes: fccaaf6fbbc59910e ("perf build-id: Change sprintf functions to snprintf")
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>

Reviewed-by: James Clark <james.clark@linaro.org>

> ---
>   tools/perf/builtin-buildid-list.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/perf/builtin-buildid-list.c b/tools/perf/builtin-buildid-list.c
> index a91bbb34ac946360..8ad39ff9a4b8a4b4 100644
> --- a/tools/perf/builtin-buildid-list.c
> +++ b/tools/perf/builtin-buildid-list.c
> @@ -61,7 +61,7 @@ static int sysfs__fprintf_build_id(FILE *fp)
>   	int ret;
>   
>   	ret = sysfs__snprintf_build_id("/", sbuild_id, sizeof(sbuild_id));
> -	if (ret != sizeof(sbuild_id))
> +	if (ret != sizeof(sbuild_id) - 1)
>   		return ret < 0 ? ret : -EINVAL;
>   
>   	return fprintf(fp, "%s\n", sbuild_id);
> @@ -73,7 +73,7 @@ static int filename__fprintf_build_id(const char *name, FILE *fp)
>   	int ret;
>   
>   	ret = filename__snprintf_build_id(name, sbuild_id, sizeof(sbuild_id));
> -	if (ret != sizeof(sbuild_id))
> +	if (ret != sizeof(sbuild_id) - 1)
>   		return ret < 0 ? ret : -EINVAL;
>   
>   	return fprintf(fp, "%s\n", sbuild_id);

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

[Namhyung Kim]

It's annoying that sashiko removes all recipients.

On Tue, Apr 21, 2026 at 12:30:04AM +0000, sashiko-bot@kernel.org wrote:
> 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?

I'm not sure about this but I guess it's supported (by padding zeros at
the end?) as we've been using this for a while.

Also this is not related to this patch anyway.

> 
> >  		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?

Well, I think that's technically possible but I'm not sure how practical
it is.  Maybe it's ok to add O_NONBLOCK anyway.

And again, this is not from this patch.

Thanks,
Namhyung

> 
> > -	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