Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.35 as permitted sender) client-ip=23.128.96.35;
MIME-Version: 1.0
References: <20231005230851.3666908-1-irogers@google.com> <20231005230851.3666908-7-irogers@google.com>
 <CAM9d7cjB_Sm4xnXexDqq_Q4jmOrwhxBjQbAhr-UTmb_4CPLONw@mail.gmail.com>
In-Reply-To: <CAM9d7cjB_Sm4xnXexDqq_Q4jmOrwhxBjQbAhr-UTmb_4CPLONw@mail.gmail.com>
From:   Ian Rogers <irogers@google.com>
Date:   Mon, 9 Oct 2023 09:22:29 -0700
Message-ID: <CAP-5=fUEFcMpAS1q+M7RNdREy4gHaJmD8N5aVXowm5gyEJ_hnQ@mail.gmail.com>
Subject: Re: [PATCH v2 06/18] perf buildid-cache: Fix use of uninitialized value
To:     Namhyung Kim <namhyung@kernel.org>
Cc:     Nathan Chancellor <nathan@kernel.org>,
        Nick Desaulniers <ndesaulniers@google.com>,
        Tom Rix <trix@redhat.com>,
        Peter Zijlstra <peterz@infradead.org>,
        Ingo Molnar <mingo@redhat.com>,
        Arnaldo Carvalho de Melo <acme@kernel.org>,
        Mark Rutland <mark.rutland@arm.com>,
        Alexander Shishkin <alexander.shishkin@linux.intel.com>,
        Jiri Olsa <jolsa@kernel.org>,
        Adrian Hunter <adrian.hunter@intel.com>,
        Yicong Yang <yangyicong@hisilicon.com>,
        Jonathan Cameron <jonathan.cameron@huawei.com>,
        Yang Jihong <yangjihong1@huawei.com>,
        Kan Liang <kan.liang@linux.intel.com>,
        Ming Wang <wangming01@loongson.cn>,
        Huacai Chen <chenhuacai@kernel.org>,
        Sean Christopherson <seanjc@google.com>,
        K Prateek Nayak <kprateek.nayak@amd.com>,
        Yanteng Si <siyanteng@loongson.cn>,
        Yuan Can <yuancan@huawei.com>,
        Ravi Bangoria <ravi.bangoria@amd.com>,
        James Clark <james.clark@arm.com>, llvm@lists.linux.dev,
        linux-kernel@vger.kernel.org, linux-perf-users@vger.kernel.org,
        bpf@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Sun, Oct 8, 2023 at 11:06=E2=80=AFPM Namhyung Kim <namhyung@kernel.org> =
wrote:
>
> On Thu, Oct 5, 2023 at 4:09=E2=80=AFPM Ian Rogers <irogers@google.com> wr=
ote:
> >
> > 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@google.com>
> > ---
> >  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-bu=
ildid-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), fal=
se) &&
> > -           filename__read_build_id(filename, &bid) =3D=3D -1) {
> > +       if (!dso__build_id_filename(dso, filename, sizeof(filename), fa=
lse))
> > +               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) =3D=3D -1) {
> >                 if (errno =3D=3D ENOENT)
> >                         return false;
> >
> > --
> > 2.42.0.609.gbb76f46606-goog
> >