Re: [PATCH v2 06/18] perf buildid-cache: Fix use of uninitialized value

[Ian Rogers]

On Sun, Oct 8, 2023 at 11:06 PM Namhyung Kim <namhyung@xxxxxxxxxx> wrote:
>
> On Thu, Oct 5, 2023 at 4:09 PM Ian Rogers <irogers@xxxxxxxxxx> wrote:
> >
> > The buildid filename is first determined and then from this the
> > buildid read. If getting the filename fails then the buildid will be
> > used for a later memcmp uninitialized. Detected by clang-tidy.
> >
> > Signed-off-by: Ian Rogers <irogers@xxxxxxxxxx>
> > ---
> >  tools/perf/builtin-buildid-cache.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/tools/perf/builtin-buildid-cache.c b/tools/perf/builtin-buildid-cache.c
> > index cd381693658b..e2a40f1d9225 100644
> > --- a/tools/perf/builtin-buildid-cache.c
> > +++ b/tools/perf/builtin-buildid-cache.c
> > @@ -277,8 +277,10 @@ static bool dso__missing_buildid_cache(struct dso *dso, int parm __maybe_unused)
> >         char filename[PATH_MAX];
> >         struct build_id bid;
> >
> > -       if (dso__build_id_filename(dso, filename, sizeof(filename), false) &&
> > -           filename__read_build_id(filename, &bid) == -1) {
> > +       if (!dso__build_id_filename(dso, filename, sizeof(filename), false))
> > +               return true;
>
> This won't print anything and ignore the file which changes
> the existing behavior.  But if it fails to read the build-id, I
> don't think there is not much we can do with it.  IIUC the
> original intention of -M/--missing option is to list files that
> have a build-id but it's not in the build-id cache.  So maybe
> it's ok to silently ignore it.

If getting the build id filename fails then 'bid' is uninitialized and
I don't think there is an expected behavior for what a memcmp on
uninitialized memory should do - we may hope that it fails and get the
pr_warning in the existing code, but that warning depends on reading
the filename too. This was the smallest change to not change behavior
but to avoid the undefined behavior (bugs) in the code. It could be a
signal the code needs to be worked on more.

Thanks,
Ian

> Thanks,
> Namhyung
>
>
> > +
> > +       if (filename__read_build_id(filename, &bid) == -1) {
> >                 if (errno == ENOENT)
> >                         return false;
> >
> > --
> > 2.42.0.609.gbb76f46606-goog
> >