Messages in this thread | | | From | Arnaldo Carvalho de Melo <> | Date | Fri, 26 Jul 2019 16:28:59 -0300 | Subject | Re: [PATCH 2/3] Fix annotate.c use of uninitialized value error |
| |
Em Wed, Jul 24, 2019 at 04:44:59PM -0700, Numfor Mbiziwo-Tiapo escreveu: > Our local MSAN (Memory Sanitizer) build of perf throws a warning > that comes from the "dso__disassemble_filename" function in > "tools/perf/util/annotate.c" when running perf record. > > The warning stems from the call to readlink, in which "build_id_path" > was being read into "linkname". Since readlink does not null terminate, > an uninitialized memory access would later occur when "linkname" is > passed into the strstr function. This is simply fixed by null-terminating > "linkname" after the call to readlink. > > To reproduce this warning, build perf by running: > make -C tools/perf CLANG=1 CC=clang EXTRA_CFLAGS="-fsanitize=memory\ > -fsanitize-memory-track-origins" > > (Additionally, llvm might have to be installed and clang might have to > be specified as the compiler - export CC=/usr/bin/clang) > > then running: > tools/perf/perf record -o - ls / | tools/perf/perf --no-pager annotate\ > -i - --stdio > > Please see the cover letter for why false positive warnings may be > generated. > > Signed-off-by: Numfor Mbiziwo-Tiapo <nums@google.com> > --- > tools/perf/util/annotate.c | 15 +++++++++++---- > 1 file changed, 11 insertions(+), 4 deletions(-) > > diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c > index 70de8f6b3aee..d8bfb561bc35 100644 > --- a/tools/perf/util/annotate.c > +++ b/tools/perf/util/annotate.c > @@ -1627,6 +1627,7 @@ static int dso__disassemble_filename(struct dso *dso, char *filename, size_t fil > char *build_id_filename; > char *build_id_path = NULL; > char *pos; > + int len; > > if (dso->symtab_type == DSO_BINARY_TYPE__KALLSYMS && > !dso__is_kcore(dso)) > @@ -1655,10 +1656,16 @@ static int dso__disassemble_filename(struct dso *dso, char *filename, size_t fil > if (pos && strlen(pos) < SBUILD_ID_SIZE - 2) > dirname(build_id_path); > > - if (dso__is_kcore(dso) || > - readlink(build_id_path, linkname, sizeof(linkname)) < 0 || > - strstr(linkname, DSO__NAME_KALLSYMS) || > - access(filename, R_OK)) { > + if (dso__is_kcore(dso)) > + goto fallback; > + > + len = readlink(build_id_path, linkname, sizeof(linkname)); > + if (len < 0) > + goto fallback;
If the buffer has N bytes and the linkname has more than that, then it will truncate at N bytes and return N, then if we access linkname[N] we will be accessing one byte after the end of the buffer, no?
> + > + linkname[len] = '\0'; > + if (strstr(linkname, DSO__NAME_KALLSYMS) || > + access(filename, R_OK)) { > fallback: > /* > * If we don't have build-ids or the build-id file isn't in the > -- > 2.22.0.657.g960e92d24f-goog
--
- Arnaldo
| |