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

It's annoying that sashiko removes all recipients.

On Tue, Apr 21, 2026 at 12:30:04AM +0000, [email protected] 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 <[email protected]>
> 
> 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/[email protected]?part=1