* Re: [PATCH] perf build-id: Fix wrong return value checking
[not found] ` <20260421003005.38E7CC19425@smtp.kernel.org>
@ 2026-04-22 7:12 ` Namhyung Kim
0 siblings, 0 replies; only message in thread
From: Namhyung Kim @ 2026-04-22 7:12 UTC (permalink / raw)
To: sashiko; +Cc: linux-perf-users
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
^ permalink raw reply [flat|nested] only message in thread