[PATCH 0/3] Perf uninitialized value fixes

[PATCH 0/3] Perf uninitialized value fixes

[Numfor Mbiziwo-Tiapo]

These patches are all warnings that the MSAN (Memory Sanitizer) build
of perf has caught.

To build perf with MSAN enabled run:
make -C tools/perf CLANG=1 CC=clang EXTRA_CFLAGS="-fsanitize=memory\
 -fsanitize-memory-track-origins"

(The -fsanitizer-memory-track-origins makes the bugs clearer but
isn't strictly necessary.)

(Additionally, llvm might have to be installed and clang might have to
be specified as the compiler - export CC=/usr/bin/clang).

The patches "Fix util.c use of uninitialized value warning" and "Fix 
annotate.c use of uninitialized value error" build on top of each other
(the changes in Fix util.c use of uninitialized value warning must be
made first).

When running the commands provided in the repro instructions, MSAN will
generate false positive uninitialized memory errors. This is happening
because libc is not MSAN-instrumented. Finding a way to build libc with
MSAN will get rid of these false positives and allow the real warnings
mentioned in the patches to be shown. 

Numfor Mbiziwo-Tiapo (3):
  Fix util.c use of uninitialized value warning
  Fix annotate.c use of uninitialized value error
  Fix sched-messaging.c use of uninitialized value errors

 tools/perf/bench/sched-messaging.c |  3 ++-
 tools/perf/util/annotate.c         | 15 +++++++++++----
 tools/perf/util/header.c           |  2 +-
 3 files changed, 14 insertions(+), 6 deletions(-)

-- 
2.22.0.657.g960e92d24f-goog

[PATCH 1/3] Fix util.c use of unitialized value warning

[Numfor Mbiziwo-Tiapo]

When building our local version of perf with MSAN (Memory Sanitizer)
and running the perf record command, MSAN throws a use of uninitialized
value warning in "tools/perf/util/util.c:333:6".

This warning stems from the "buf" variable being passed into "write".
It originated as the variable "ev" with the type union perf_event*
defined in the "perf_event__synthesize_attr" function in
"tools/perf/util/header.c".

In the "perf_event__synthesize_attr" function they allocate space with
a malloc call using ev, then go on to only assign some of the member
variables before passing "ev" on as a parameter to the "process" function
therefore "ev" contains uninitialized memory. Changing the malloc call
to calloc initializes all the members of "ev" which gets rid of the
warning.

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/header.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
index dec6d218c31c..b9c71fc45ac1 100644
--- a/tools/perf/util/header.c
+++ b/tools/perf/util/header.c
@@ -3427,7 +3427,7 @@ int perf_event__synthesize_attr(struct perf_tool *tool,
 	size += sizeof(struct perf_event_header);
 	size += ids * sizeof(u64);
 
-	ev = malloc(size);
+	ev = calloc(1, size);
 
 	if (ev == NULL)
 		return -ENOMEM;
-- 
2.22.0.657.g960e92d24f-goog

Re: [PATCH 1/3] Fix util.c use of unitialized value warning

[Arnaldo Carvalho de Melo]

Em Wed, Jul 24, 2019 at 04:44:58PM -0700, Numfor Mbiziwo-Tiapo escreveu:
> When building our local version of perf with MSAN (Memory Sanitizer)
> and running the perf record command, MSAN throws a use of uninitialized
> value warning in "tools/perf/util/util.c:333:6".
> 
> This warning stems from the "buf" variable being passed into "write".
> It originated as the variable "ev" with the type union perf_event*
> defined in the "perf_event__synthesize_attr" function in
> "tools/perf/util/header.c".
> 
> In the "perf_event__synthesize_attr" function they allocate space with
> a malloc call using ev, then go on to only assign some of the member
> variables before passing "ev" on as a parameter to the "process" function
> therefore "ev" contains uninitialized memory. Changing the malloc call
> to calloc initializes all the members of "ev" which gets rid of the
> warning.

I'm applying, but changing the calloc call to zalloc() that is just a
shorter wrapper to calloc(1, size).

Thanks,

- Arnaldo
 
> 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/header.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
> index dec6d218c31c..b9c71fc45ac1 100644
> --- a/tools/perf/util/header.c
> +++ b/tools/perf/util/header.c
> @@ -3427,7 +3427,7 @@ int perf_event__synthesize_attr(struct perf_tool *tool,
>  	size += sizeof(struct perf_event_header);
>  	size += ids * sizeof(u64);
>  
> -	ev = malloc(size);
> +	ev = calloc(1, size);
>  
>  	if (ev == NULL)
>  		return -ENOMEM;
> -- 
> 2.22.0.657.g960e92d24f-goog

-- 

- Arnaldo

[tip:perf/urgent] perf header: Fix use of unitialized value warning

[tip-bot for Numfor Mbiziwo-Tiapo]

Commit-ID:  20f9781f491360e7459c589705a2e4b1f136bee9
Gitweb:     https://git.kernel.org/tip/20f9781f491360e7459c589705a2e4b1f136bee9
Author:     Numfor Mbiziwo-Tiapo <nums@google.com>
AuthorDate: Wed, 24 Jul 2019 16:44:58 -0700
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Mon, 29 Jul 2019 09:03:43 -0300

perf header: Fix use of unitialized value warning

When building our local version of perf with MSAN (Memory Sanitizer) and
running the perf record command, MSAN throws a use of uninitialized
value warning in "tools/perf/util/util.c:333:6".

This warning stems from the "buf" variable being passed into "write".
It originated as the variable "ev" with the type union perf_event*
defined in the "perf_event__synthesize_attr" function in
"tools/perf/util/header.c".

In the "perf_event__synthesize_attr" function they allocate space with a malloc
call using ev, then go on to only assign some of the member variables before
passing "ev" on as a parameter to the "process" function therefore "ev"
contains uninitialized memory. Changing the malloc call to zalloc to initialize
all the members of "ev" which gets rid of the warning.

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>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Ian Rogers <irogers@google.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Mark Drayton <mbd@fb.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Song Liu <songliubraving@fb.com>
Cc: Stephane Eranian <eranian@google.com>
Link: http://lkml.kernel.org/r/20190724234500.253358-2-nums@google.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/header.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
index 47877f0f6667..1903d7ec9797 100644
--- a/tools/perf/util/header.c
+++ b/tools/perf/util/header.c
@@ -3646,7 +3646,7 @@ int perf_event__synthesize_attr(struct perf_tool *tool,
 	size += sizeof(struct perf_event_header);
 	size += ids * sizeof(u64);
 
-	ev = malloc(size);
+	ev = zalloc(size);
 
 	if (ev == NULL)
 		return -ENOMEM;

[PATCH 2/3] Fix annotate.c use of uninitialized value error

[Numfor Mbiziwo-Tiapo]

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;
+
+	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

Re: [PATCH 2/3] Fix annotate.c use of uninitialized value error

[Arnaldo Carvalho de Melo]

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

[PATCH v2] Fix annotate.c use of uninitialized value error

[Numfor Mbiziwo-Tiapo]

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..e1b075b52dce 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) - 1);
+	if (len < 0)
+		goto fallback;
+
+	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.709.g102302147b-goog

[PATCH 3/3] Fix sched-messaging.c use of uninitialized value errors

[Numfor Mbiziwo-Tiapo]

Our local MSAN (Memory Sanitizer) build of perf throws use of
uninitialized value warnings in "tools/perf/bench/sched-messaging.c"
when running perf bench.

The first warning comes from the "ready" function where the "dummy" char
is declared and then passed into "write" without being initialized.
Initializing "dummy" to any character silences the warning.

The second warning comes from the "sender" function where a "write" call
is made to write the contents from the "data" char array when it has not
yet been initialized. Calling memset on "data" silences the warning.

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 bench sched all

Please see the cover letter for why false positive warnings may be
generated.

Signed-off-by: Numfor Mbiziwo-Tiapo <nums@google.com>
---
 tools/perf/bench/sched-messaging.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tools/perf/bench/sched-messaging.c b/tools/perf/bench/sched-messaging.c
index f9d7641ae833..d22d7b7b591d 100644
--- a/tools/perf/bench/sched-messaging.c
+++ b/tools/perf/bench/sched-messaging.c
@@ -69,7 +69,7 @@ static void fdpair(int fds[2])
 /* Block until we're ready to go */
 static void ready(int ready_out, int wakefd)
 {
-	char dummy;
+	char dummy = 'N';
 	struct pollfd pollfd = { .fd = wakefd, .events = POLLIN };
 
 	/* Tell them we're ready. */
@@ -87,6 +87,7 @@ static void *sender(struct sender_context *ctx)
 	char data[DATASIZE];
 	unsigned int i, j;
 
+	memset(data, 'N', DATASIZE);
 	ready(ctx->ready_out, ctx->wakefd);
 
 	/* Now pump to every receiver. */
-- 
2.22.0.657.g960e92d24f-goog

Re: [PATCH 3/3] Fix sched-messaging.c use of uninitialized value errors

[Arnaldo Carvalho de Melo]

Em Wed, Jul 24, 2019 at 04:45:00PM -0700, Numfor Mbiziwo-Tiapo escreveu:
> Our local MSAN (Memory Sanitizer) build of perf throws use of
> uninitialized value warnings in "tools/perf/bench/sched-messaging.c"
> when running perf bench.
> 
> The first warning comes from the "ready" function where the "dummy" char
> is declared and then passed into "write" without being initialized.
> Initializing "dummy" to any character silences the warning.
> 
> The second warning comes from the "sender" function where a "write" call
> is made to write the contents from the "data" char array when it has not
> yet been initialized. Calling memset on "data" silences the warning.

So, this is just to silence MSAN, as it doesn't matter what is sent,
whatever values are in those variables is ok, as it will not be used,
right?

- Arnaldo
 
> 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 bench sched all
> 
> Please see the cover letter for why false positive warnings may be
> generated.
> 
> Signed-off-by: Numfor Mbiziwo-Tiapo <nums@google.com>
> ---
>  tools/perf/bench/sched-messaging.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/perf/bench/sched-messaging.c b/tools/perf/bench/sched-messaging.c
> index f9d7641ae833..d22d7b7b591d 100644
> --- a/tools/perf/bench/sched-messaging.c
> +++ b/tools/perf/bench/sched-messaging.c
> @@ -69,7 +69,7 @@ static void fdpair(int fds[2])
>  /* Block until we're ready to go */
>  static void ready(int ready_out, int wakefd)
>  {
> -	char dummy;
> +	char dummy = 'N';
>  	struct pollfd pollfd = { .fd = wakefd, .events = POLLIN };
>  
>  	/* Tell them we're ready. */
> @@ -87,6 +87,7 @@ static void *sender(struct sender_context *ctx)
>  	char data[DATASIZE];
>  	unsigned int i, j;
>  
> +	memset(data, 'N', DATASIZE);
>  	ready(ctx->ready_out, ctx->wakefd);
>  
>  	/* Now pump to every receiver. */
> -- 
> 2.22.0.657.g960e92d24f-goog

-- 

- Arnaldo

Re: [PATCH 3/3] Fix sched-messaging.c use of uninitialized value errors

[Ian Rogers]

On Fri, Jul 26, 2019 at 12:32 PM Arnaldo Carvalho de Melo
<arnaldo.melo@gmail.com> wrote:
>
> Em Wed, Jul 24, 2019 at 04:45:00PM -0700, Numfor Mbiziwo-Tiapo escreveu:
> > Our local MSAN (Memory Sanitizer) build of perf throws use of
> > uninitialized value warnings in "tools/perf/bench/sched-messaging.c"
> > when running perf bench.
> >
> > The first warning comes from the "ready" function where the "dummy" char
> > is declared and then passed into "write" without being initialized.
> > Initializing "dummy" to any character silences the warning.
> >
> > The second warning comes from the "sender" function where a "write" call
> > is made to write the contents from the "data" char array when it has not
> > yet been initialized. Calling memset on "data" silences the warning.
>
> So, this is just to silence MSAN, as it doesn't matter what is sent,
> whatever values are in those variables is ok, as it will not be used,
> right?

That's right.

Thanks,
Ian Rogers

> - Arnaldo
>
> > 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 bench sched all
> >
> > Please see the cover letter for why false positive warnings may be
> > generated.
> >
> > Signed-off-by: Numfor Mbiziwo-Tiapo <nums@google.com>
> > ---
> >  tools/perf/bench/sched-messaging.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/tools/perf/bench/sched-messaging.c b/tools/perf/bench/sched-messaging.c
> > index f9d7641ae833..d22d7b7b591d 100644
> > --- a/tools/perf/bench/sched-messaging.c
> > +++ b/tools/perf/bench/sched-messaging.c
> > @@ -69,7 +69,7 @@ static void fdpair(int fds[2])
> >  /* Block until we're ready to go */
> >  static void ready(int ready_out, int wakefd)
> >  {
> > -     char dummy;
> > +     char dummy = 'N';
> >       struct pollfd pollfd = { .fd = wakefd, .events = POLLIN };
> >
> >       /* Tell them we're ready. */
> > @@ -87,6 +87,7 @@ static void *sender(struct sender_context *ctx)
> >       char data[DATASIZE];
> >       unsigned int i, j;
> >
> > +     memset(data, 'N', DATASIZE);
> >       ready(ctx->ready_out, ctx->wakefd);
> >
> >       /* Now pump to every receiver. */
> > --
> > 2.22.0.657.g960e92d24f-goog
>
> --
>
> - Arnaldo

Re: [PATCH 0/3] Perf uninitialized value fixes

[Arnaldo Carvalho de Melo]

Em Wed, Jul 24, 2019 at 04:44:57PM -0700, Numfor Mbiziwo-Tiapo escreveu:
> These patches are all warnings that the MSAN (Memory Sanitizer) build
> of perf has caught.
> 
> To build perf with MSAN enabled run:
> make -C tools/perf CLANG=1 CC=clang EXTRA_CFLAGS="-fsanitize=memory\
>  -fsanitize-memory-track-origins"
> 
> (The -fsanitizer-memory-track-origins makes the bugs clearer but
> isn't strictly necessary.)
> 
> (Additionally, llvm might have to be installed and clang might have to
> be specified as the compiler - export CC=/usr/bin/clang).
> 
> The patches "Fix util.c use of uninitialized value warning" and "Fix 
> annotate.c use of uninitialized value error" build on top of each other
> (the changes in Fix util.c use of uninitialized value warning must be
> made first).
> 
> When running the commands provided in the repro instructions, MSAN will
> generate false positive uninitialized memory errors. This is happening
> because libc is not MSAN-instrumented. Finding a way to build libc with
> MSAN will get rid of these false positives and allow the real warnings
> mentioned in the patches to be shown. 

So this is because I'm not running a glibc linked with MSAN? Do you have
any pointer to help building glibc with MSAN? I want to do that inside a
container so that I can use these sanitizers, thanks,

[root@quaco ~]# perf record -o - ls / | perf --no-pager annotate -i -  --stdio
==29732==WARNING: MemorySanitizer: use-of-uninitialized-value
==29733==WARNING: MemorySanitizer: use-of-uninitialized-value
    #0 0xcc136d in add_path /home/acme/git/perf/tools/lib/subcmd/exec-cmd.c:130:6
    #1 0xcc075e in setup_path /home/acme/git/perf/tools/lib/subcmd/exec-cmd.c:146:2
    #2 0x71298d in main /home/acme/git/perf/tools/perf/perf.c:512:2
    #0 0xcc136d in add_path /home/acme/git/perf/tools/lib/subcmd/exec-cmd.c:130:6
    #1 0xcc075e in setup_path /home/acme/git/perf/tools/lib/subcmd/exec-cmd.c:146:2
    #2 0x71298d in main /home/acme/git/perf/tools/perf/perf.c:512:2
    #3 0x7f45b9e29f32 in __libc_start_main (/lib64/libc.so.6+0x23f32)
    #4 0x447dcd in _start (/home/acme/bin/perf+0x447dcd)

  Uninitialized value was created by a heap allocation
    #3 0x7fd6433cff32 in __libc_start_main (/lib64/libc.so.6+0x23f32)
    #4 0x447dcd in _start (/home/acme/bin/perf+0x447dcd)

  Uninitialized value was created by a heap allocation
    #0 0x4507d2 in malloc /home/acme/git/llvm/projects/compiler-rt/lib/msan/msan_interceptors.cc:916:3
    #1 0x7f45b9e7fc47 in __vasprintf_internal (/lib64/libc.so.6+0x79c47)

SUMMARY: MemorySanitizer: use-of-uninitialized-value /home/acme/git/perf/tools/lib/subcmd/exec-cmd.c:130:6 in add_path
Exiting
    #0 0x4507d2 in malloc /home/acme/git/llvm/projects/compiler-rt/lib/msan/msan_interceptors.cc:916:3
    #1 0x7fd643425c47 in __vasprintf_internal (/lib64/libc.so.6+0x79c47)

SUMMARY: MemorySanitizer: use-of-uninitialized-value /home/acme/git/perf/tools/lib/subcmd/exec-cmd.c:130:6 in add_path
Exiting
[root@quaco ~]#
 
> Numfor Mbiziwo-Tiapo (3):
>   Fix util.c use of uninitialized value warning
>   Fix annotate.c use of uninitialized value error
>   Fix sched-messaging.c use of uninitialized value errors
> 
>  tools/perf/bench/sched-messaging.c |  3 ++-
>  tools/perf/util/annotate.c         | 15 +++++++++++----
>  tools/perf/util/header.c           |  2 +-
>  3 files changed, 14 insertions(+), 6 deletions(-)
> 
> -- 
> 2.22.0.657.g960e92d24f-goog

-- 

- Arnaldo

Re: [PATCH 0/3] Perf uninitialized value fixes

[Ian Rogers]

On Wed, Aug 7, 2019 at 1:38 PM Arnaldo Carvalho de Melo
<arnaldo.melo@gmail.com> wrote:
>
> Em Wed, Jul 24, 2019 at 04:44:57PM -0700, Numfor Mbiziwo-Tiapo escreveu:
> > These patches are all warnings that the MSAN (Memory Sanitizer) build
> > of perf has caught.
> >
> > To build perf with MSAN enabled run:
> > make -C tools/perf CLANG=1 CC=clang EXTRA_CFLAGS="-fsanitize=memory\
> >  -fsanitize-memory-track-origins"
> >
> > (The -fsanitizer-memory-track-origins makes the bugs clearer but
> > isn't strictly necessary.)
> >
> > (Additionally, llvm might have to be installed and clang might have to
> > be specified as the compiler - export CC=/usr/bin/clang).
> >
> > The patches "Fix util.c use of uninitialized value warning" and "Fix
> > annotate.c use of uninitialized value error" build on top of each other
> > (the changes in Fix util.c use of uninitialized value warning must be
> > made first).
> >
> > When running the commands provided in the repro instructions, MSAN will
> > generate false positive uninitialized memory errors. This is happening
> > because libc is not MSAN-instrumented. Finding a way to build libc with
> > MSAN will get rid of these false positives and allow the real warnings
> > mentioned in the patches to be shown.
>
> So this is because I'm not running a glibc linked with MSAN? Do you have
> any pointer to help building glibc with MSAN? I want to do that inside a
> container so that I can use these sanitizers, thanks,
>
> [root@quaco ~]# perf record -o - ls / | perf --no-pager annotate -i -  --stdio
> ==29732==WARNING: MemorySanitizer: use-of-uninitialized-value
> ==29733==WARNING: MemorySanitizer: use-of-uninitialized-value
>     #0 0xcc136d in add_path /home/acme/git/perf/tools/lib/subcmd/exec-cmd.c:130:6
>     #1 0xcc075e in setup_path /home/acme/git/perf/tools/lib/subcmd/exec-cmd.c:146:2
>     #2 0x71298d in main /home/acme/git/perf/tools/perf/perf.c:512:2
>     #0 0xcc136d in add_path /home/acme/git/perf/tools/lib/subcmd/exec-cmd.c:130:6
>     #1 0xcc075e in setup_path /home/acme/git/perf/tools/lib/subcmd/exec-cmd.c:146:2
>     #2 0x71298d in main /home/acme/git/perf/tools/perf/perf.c:512:2
>     #3 0x7f45b9e29f32 in __libc_start_main (/lib64/libc.so.6+0x23f32)
>     #4 0x447dcd in _start (/home/acme/bin/perf+0x447dcd)
>
>   Uninitialized value was created by a heap allocation
>     #3 0x7fd6433cff32 in __libc_start_main (/lib64/libc.so.6+0x23f32)
>     #4 0x447dcd in _start (/home/acme/bin/perf+0x447dcd)
>
>   Uninitialized value was created by a heap allocation
>     #0 0x4507d2 in malloc /home/acme/git/llvm/projects/compiler-rt/lib/msan/msan_interceptors.cc:916:3
>     #1 0x7f45b9e7fc47 in __vasprintf_internal (/lib64/libc.so.6+0x79c47)
>
> SUMMARY: MemorySanitizer: use-of-uninitialized-value /home/acme/git/perf/tools/lib/subcmd/exec-cmd.c:130:6 in add_path
> Exiting
>     #0 0x4507d2 in malloc /home/acme/git/llvm/projects/compiler-rt/lib/msan/msan_interceptors.cc:916:3
>     #1 0x7fd643425c47 in __vasprintf_internal (/lib64/libc.so.6+0x79c47)
>
> SUMMARY: MemorySanitizer: use-of-uninitialized-value /home/acme/git/perf/tools/lib/subcmd/exec-cmd.c:130:6 in add_path
> Exiting
> [root@quaco ~]#
>
> > Numfor Mbiziwo-Tiapo (3):
> >   Fix util.c use of uninitialized value warning
> >   Fix annotate.c use of uninitialized value error
> >   Fix sched-messaging.c use of uninitialized value errors
> >
> >  tools/perf/bench/sched-messaging.c |  3 ++-
> >  tools/perf/util/annotate.c         | 15 +++++++++++----
> >  tools/perf/util/header.c           |  2 +-
> >  3 files changed, 14 insertions(+), 6 deletions(-)

Thanks Arnaldo! Debugging the issue it isn't down glibc, there are
interceptors in the sanitizers for asprintf to recognize it as a
source of memory allocation. The problem is the sanitizers don't
support _FORTIFY_SOURCE [1] and this is causing the false positives.
The following patch works to resolve the false-positive issue for me:

-----
--- a/tools/lib/subcmd/Makefile
+++ b/tools/lib/subcmd/Makefile
@@ -20,7 +20,13 @@ MAKEFLAGS += --no-print-directory
LIBFILE = $(OUTPUT)libsubcmd.a

CFLAGS := $(EXTRA_WARNINGS) $(EXTRA_CFLAGS)
-CFLAGS += -ggdb3 -Wall -Wextra -std=gnu99 -U_FORTIFY_SOURCE
-D_FORTIFY_SOURCE=2 -fPIC
+CFLAGS += -ggdb3 -Wall -Wextra -std=gnu99 -fPIC
+
+ifeq ($(DEBUG),0)
+  ifeq ($(feature-fortify-source), 1)
+    CFLAGS += -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2
+  endif
+endif

ifeq ($(CC_NO_CLANG), 0)
  CFLAGS += -O3
-----

Thanks,
Ian

[1] https://github.com/google/sanitizers/wiki/AddressSanitizer#faq

> > --
> > 2.22.0.657.g960e92d24f-goog
>
> --
>
> - Arnaldo

Re: [PATCH 1/3] Fix backward-ring-buffer.c format-truncation error

[Arnaldo Carvalho de Melo]

Em Wed, Jul 24, 2019 at 11:45:10AM -0700, Numfor Mbiziwo-Tiapo escreveu:
> Perf does not build with the ubsan (undefined behavior sanitizer)
> and there is an error that says:
> 
> tests/backward-ring-buffer.c:23:45: error: ‘%d’ directive output
> may be truncated writing between 1 and 10 bytes into a region of
> size 8 [-Werror=format-truncation=]
>    snprintf(proc_name, sizeof(proc_name), "p:%d\n", i);
> 
> This can be reproduced by running (from the tip directory):
> make -C tools/perf USE_CLANG=1 EXTRA_CFLAGS="-fsanitize=undefined"
> 
> Th error occurs because they are writing to the 10 byte buffer - the
> index 'i' of the for loop and the 2 byte hardcoded string. If somehow 'i'
> was greater than 8 bytes (10 - 2), then the snprintf function would
> truncate the string. Increasing the size of the buffer fixes the error.
> 
> Signed-off-by: Numfor Mbiziwo-Tiapo <nums@google.com>
> ---
>  tools/perf/tests/backward-ring-buffer.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/perf/tests/backward-ring-buffer.c b/tools/perf/tests/backward-ring-buffer.c
> index 6d598cc071ae..1a9c3becf5ff 100644
> --- a/tools/perf/tests/backward-ring-buffer.c
> +++ b/tools/perf/tests/backward-ring-buffer.c
> @@ -18,7 +18,7 @@ static void testcase(void)
>  	int i;
>  
>  	for (i = 0; i < NR_ITERS; i++) {
> -		char proc_name[10];
> +		char proc_name[15];
>  
>  		snprintf(proc_name, sizeof(proc_name), "p:%d\n", i);
>  		prctl(PR_SET_NAME, proc_name);

This was fixed already by:

commit 11c1ea6f1a9bc97bf857fd12f72eacb6c69794e2
Author: Changbin Du <changbin.du@gmail.com>
Date:   Sat Mar 16 16:05:43 2019 +0800

    perf tools: Fix errors under optimization level '-Og'

    Optimization level '-Og' offers a reasonable level of optimization while
    maintaining fast compilation and a good debugging experience. This patch
    tries to make it work.

      $ make DEBUG=1 EXTRA_CFLAGS='-Og'
      bench/epoll-ctl.c: In function ‘do_threads’:
      bench/epoll-ctl.c:274:9: error: ‘ret’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
        return ret;
               ^~~
      ...

    Signed-off-by: Changbin Du <changbin.du@gmail.com>
    Reviewed-by: Jiri Olsa <jolsa@kernel.org>

[PATCH v2] Fix annotate.c use of uninitialized value error

[Numfor Mbiziwo-Tiapo]

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..e1b075b52dce 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) - 1);
+	if (len < 0)
+		goto fallback;
+
+	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.709.g102302147b-goog

Re: [PATCH v2] Fix annotate.c use of uninitialized value error

[Jiri Olsa]

On Mon, Jul 29, 2019 at 01:57:50PM -0700, Numfor Mbiziwo-Tiapo wrote:
> 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>

Acked-by: Jiri Olsa <jolsa@kernel.org>

thanks,
jirka

> ---
>  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..e1b075b52dce 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) - 1);
> +	if (len < 0)
> +		goto fallback;
> +
> +	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.709.g102302147b-goog
> 

Re: [PATCH v2] Fix annotate.c use of uninitialized value error

[Ian Rogers]

It looks like this wasn't merged to tip. Does anything need addressing
to get it merged?

Thanks,
Ian

On Wed, Aug 7, 2019 at 4:32 AM Jiri Olsa <jolsa@redhat.com> wrote:
>
> On Mon, Jul 29, 2019 at 01:57:50PM -0700, Numfor Mbiziwo-Tiapo wrote:
> > 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>
>
> Acked-by: Jiri Olsa <jolsa@kernel.org>
>
> thanks,
> jirka
>
> > ---
> >  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..e1b075b52dce 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) - 1);
> > +     if (len < 0)
> > +             goto fallback;
> > +
> > +     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.709.g102302147b-goog
> >

Re: [PATCH v2] Fix annotate.c use of uninitialized value error

[Ian Rogers]

On Fri, Oct 25, 2019 at 3:11 PM Ian Rogers <irogers@google.com> wrote:
>
> It looks like this wasn't merged to tip. Does anything need addressing
> to get it merged?
>
> Thanks,
> Ian
>
> On Wed, Aug 7, 2019 at 4:32 AM Jiri Olsa <jolsa@redhat.com> wrote:
> >
> > On Mon, Jul 29, 2019 at 01:57:50PM -0700, Numfor Mbiziwo-Tiapo wrote:
> > > 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>
> >
> > Acked-by: Jiri Olsa <jolsa@kernel.org>

Acked-by: Ian Rogers <irogers@google.com>

Arnaldo, I think this got overlooked. Thanks,

Ian

> > thanks,
> > jirka
> >
> > > ---
> > >  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..e1b075b52dce 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) - 1);
> > > +     if (len < 0)
> > > +             goto fallback;
> > > +
> > > +     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.709.g102302147b-goog
> > >

Re: [PATCH v2] Fix annotate.c use of uninitialized value error

[Arnaldo Carvalho de Melo]

Em Wed, Jul 08, 2020 at 05:54:47PM -0700, Ian Rogers escreveu:
> On Fri, Oct 25, 2019 at 3:11 PM Ian Rogers <irogers@google.com> wrote:
> >
> > It looks like this wasn't merged to tip. Does anything need addressing
> > to get it merged?

Finally processed, thanks for the multiple reminders,

- Arnaldo

> > Thanks,
> > Ian
> >
> > On Wed, Aug 7, 2019 at 4:32 AM Jiri Olsa <jolsa@redhat.com> wrote:
> > >
> > > On Mon, Jul 29, 2019 at 01:57:50PM -0700, Numfor Mbiziwo-Tiapo wrote:
> > > > 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>
> > >
> > > Acked-by: Jiri Olsa <jolsa@kernel.org>
> 
> Acked-by: Ian Rogers <irogers@google.com>
> 
> Arnaldo, I think this got overlooked. Thanks,
> 
> Ian
> 
> > > thanks,
> > > jirka
> > >
> > > > ---
> > > >  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..e1b075b52dce 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) - 1);
> > > > +     if (len < 0)
> > > > +             goto fallback;
> > > > +
> > > > +     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.709.g102302147b-goog
> > > >

-- 

- Arnaldo