linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/3] Perf uninitialized value fixes
@ 2019-07-24 23:44 Numfor Mbiziwo-Tiapo
  2019-07-24 23:44 ` [PATCH 1/3] Fix util.c use of unitialized value warning Numfor Mbiziwo-Tiapo
                   ` (3 more replies)
  0 siblings, 4 replies; 12+ messages in thread
From: Numfor Mbiziwo-Tiapo @ 2019-07-24 23:44 UTC (permalink / raw)
  To: peterz, mingo, acme, alexander.shishkin, jolsa, namhyung,
	songliubraving, mbd
  Cc: linux-kernel, irogers, eranian, 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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH 1/3] Fix util.c use of unitialized value warning
  2019-07-24 23:44 [PATCH 0/3] Perf uninitialized value fixes Numfor Mbiziwo-Tiapo
@ 2019-07-24 23:44 ` Numfor Mbiziwo-Tiapo
  2019-07-26 19:19   ` Arnaldo Carvalho de Melo
  2019-07-29 21:35   ` [tip:perf/urgent] perf header: Fix " tip-bot for Numfor Mbiziwo-Tiapo
  2019-07-24 23:44 ` [PATCH 2/3] Fix annotate.c use of uninitialized value error Numfor Mbiziwo-Tiapo
                   ` (2 subsequent siblings)
  3 siblings, 2 replies; 12+ messages in thread
From: Numfor Mbiziwo-Tiapo @ 2019-07-24 23:44 UTC (permalink / raw)
  To: peterz, mingo, acme, alexander.shishkin, jolsa, namhyung,
	songliubraving, mbd
  Cc: linux-kernel, irogers, eranian, 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


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 2/3] Fix annotate.c use of uninitialized value error
  2019-07-24 23:44 [PATCH 0/3] Perf uninitialized value fixes Numfor Mbiziwo-Tiapo
  2019-07-24 23:44 ` [PATCH 1/3] Fix util.c use of unitialized value warning Numfor Mbiziwo-Tiapo
@ 2019-07-24 23:44 ` Numfor Mbiziwo-Tiapo
  2019-07-26 19:28   ` Arnaldo Carvalho de Melo
  2019-07-24 23:45 ` [PATCH 3/3] Fix sched-messaging.c use of uninitialized value errors Numfor Mbiziwo-Tiapo
  2019-08-07 20:38 ` [PATCH 0/3] Perf uninitialized value fixes Arnaldo Carvalho de Melo
  3 siblings, 1 reply; 12+ messages in thread
From: Numfor Mbiziwo-Tiapo @ 2019-07-24 23:44 UTC (permalink / raw)
  To: peterz, mingo, acme, alexander.shishkin, jolsa, namhyung,
	songliubraving, mbd
  Cc: linux-kernel, irogers, eranian, 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


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 3/3] Fix sched-messaging.c use of uninitialized value errors
  2019-07-24 23:44 [PATCH 0/3] Perf uninitialized value fixes Numfor Mbiziwo-Tiapo
  2019-07-24 23:44 ` [PATCH 1/3] Fix util.c use of unitialized value warning Numfor Mbiziwo-Tiapo
  2019-07-24 23:44 ` [PATCH 2/3] Fix annotate.c use of uninitialized value error Numfor Mbiziwo-Tiapo
@ 2019-07-24 23:45 ` Numfor Mbiziwo-Tiapo
  2019-07-26 19:32   ` Arnaldo Carvalho de Melo
  2019-08-07 20:38 ` [PATCH 0/3] Perf uninitialized value fixes Arnaldo Carvalho de Melo
  3 siblings, 1 reply; 12+ messages in thread
From: Numfor Mbiziwo-Tiapo @ 2019-07-24 23:45 UTC (permalink / raw)
  To: peterz, mingo, acme, alexander.shishkin, jolsa, namhyung,
	songliubraving, mbd
  Cc: linux-kernel, irogers, eranian, 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


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/3] Fix util.c use of unitialized value warning
  2019-07-24 23:44 ` [PATCH 1/3] Fix util.c use of unitialized value warning Numfor Mbiziwo-Tiapo
@ 2019-07-26 19:19   ` Arnaldo Carvalho de Melo
  2019-07-29 21:35   ` [tip:perf/urgent] perf header: Fix " tip-bot for Numfor Mbiziwo-Tiapo
  1 sibling, 0 replies; 12+ messages in thread
From: Arnaldo Carvalho de Melo @ 2019-07-26 19:19 UTC (permalink / raw)
  To: Numfor Mbiziwo-Tiapo
  Cc: peterz, mingo, alexander.shishkin, jolsa, namhyung,
	songliubraving, mbd, linux-kernel, irogers, eranian

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/3] Fix annotate.c use of uninitialized value error
  2019-07-24 23:44 ` [PATCH 2/3] Fix annotate.c use of uninitialized value error Numfor Mbiziwo-Tiapo
@ 2019-07-26 19:28   ` Arnaldo Carvalho de Melo
  2019-07-31  0:40     ` [PATCH v2] " Numfor Mbiziwo-Tiapo
  0 siblings, 1 reply; 12+ messages in thread
From: Arnaldo Carvalho de Melo @ 2019-07-26 19:28 UTC (permalink / raw)
  To: Numfor Mbiziwo-Tiapo
  Cc: peterz, mingo, alexander.shishkin, jolsa, namhyung,
	songliubraving, mbd, linux-kernel, irogers, eranian

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 3/3] Fix sched-messaging.c use of uninitialized value errors
  2019-07-24 23:45 ` [PATCH 3/3] Fix sched-messaging.c use of uninitialized value errors Numfor Mbiziwo-Tiapo
@ 2019-07-26 19:32   ` Arnaldo Carvalho de Melo
  2019-07-26 23:52     ` Ian Rogers
  0 siblings, 1 reply; 12+ messages in thread
From: Arnaldo Carvalho de Melo @ 2019-07-26 19:32 UTC (permalink / raw)
  To: Numfor Mbiziwo-Tiapo
  Cc: peterz, mingo, alexander.shishkin, jolsa, namhyung,
	songliubraving, mbd, linux-kernel, irogers, eranian

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 3/3] Fix sched-messaging.c use of uninitialized value errors
  2019-07-26 19:32   ` Arnaldo Carvalho de Melo
@ 2019-07-26 23:52     ` Ian Rogers
  0 siblings, 0 replies; 12+ messages in thread
From: Ian Rogers @ 2019-07-26 23:52 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: Numfor Mbiziwo-Tiapo, Peter Zijlstra, Ingo Molnar,
	Alexander Shishkin, Jiri Olsa, Namhyung Kim, Song Liu, mbd, LKML,
	Stephane Eranian

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [tip:perf/urgent] perf header: Fix use of unitialized value warning
  2019-07-24 23:44 ` [PATCH 1/3] Fix util.c use of unitialized value warning Numfor Mbiziwo-Tiapo
  2019-07-26 19:19   ` Arnaldo Carvalho de Melo
@ 2019-07-29 21:35   ` tip-bot for Numfor Mbiziwo-Tiapo
  1 sibling, 0 replies; 12+ messages in thread
From: tip-bot for Numfor Mbiziwo-Tiapo @ 2019-07-29 21:35 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: tglx, acme, jolsa, linux-kernel, eranian, alexander.shishkin,
	mingo, irogers, nums, namhyung, songliubraving, hpa, mbd, peterz

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;

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH v2] Fix annotate.c use of uninitialized value error
  2019-07-26 19:28   ` Arnaldo Carvalho de Melo
@ 2019-07-31  0:40     ` Numfor Mbiziwo-Tiapo
  0 siblings, 0 replies; 12+ messages in thread
From: Numfor Mbiziwo-Tiapo @ 2019-07-31  0:40 UTC (permalink / raw)
  To: peterz, mingo, acme, alexander.shishkin, jolsa, namhyung,
	songliubraving, mbd
  Cc: linux-kernel, irogers, eranian, 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


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH 0/3] Perf uninitialized value fixes
  2019-07-24 23:44 [PATCH 0/3] Perf uninitialized value fixes Numfor Mbiziwo-Tiapo
                   ` (2 preceding siblings ...)
  2019-07-24 23:45 ` [PATCH 3/3] Fix sched-messaging.c use of uninitialized value errors Numfor Mbiziwo-Tiapo
@ 2019-08-07 20:38 ` Arnaldo Carvalho de Melo
  2019-08-22 21:29   ` Ian Rogers
  3 siblings, 1 reply; 12+ messages in thread
From: Arnaldo Carvalho de Melo @ 2019-08-07 20:38 UTC (permalink / raw)
  To: Numfor Mbiziwo-Tiapo
  Cc: peterz, mingo, alexander.shishkin, jolsa, namhyung,
	songliubraving, mbd, linux-kernel, irogers, eranian

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 0/3] Perf uninitialized value fixes
  2019-08-07 20:38 ` [PATCH 0/3] Perf uninitialized value fixes Arnaldo Carvalho de Melo
@ 2019-08-22 21:29   ` Ian Rogers
  0 siblings, 0 replies; 12+ messages in thread
From: Ian Rogers @ 2019-08-22 21:29 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: Numfor Mbiziwo-Tiapo, Peter Zijlstra, Ingo Molnar,
	Alexander Shishkin, Jiri Olsa, Namhyung Kim, Song Liu, mbd, LKML,
	Stephane Eranian

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2019-08-22 21:29 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-24 23:44 [PATCH 0/3] Perf uninitialized value fixes Numfor Mbiziwo-Tiapo
2019-07-24 23:44 ` [PATCH 1/3] Fix util.c use of unitialized value warning Numfor Mbiziwo-Tiapo
2019-07-26 19:19   ` Arnaldo Carvalho de Melo
2019-07-29 21:35   ` [tip:perf/urgent] perf header: Fix " tip-bot for Numfor Mbiziwo-Tiapo
2019-07-24 23:44 ` [PATCH 2/3] Fix annotate.c use of uninitialized value error Numfor Mbiziwo-Tiapo
2019-07-26 19:28   ` Arnaldo Carvalho de Melo
2019-07-31  0:40     ` [PATCH v2] " Numfor Mbiziwo-Tiapo
2019-07-24 23:45 ` [PATCH 3/3] Fix sched-messaging.c use of uninitialized value errors Numfor Mbiziwo-Tiapo
2019-07-26 19:32   ` Arnaldo Carvalho de Melo
2019-07-26 23:52     ` Ian Rogers
2019-08-07 20:38 ` [PATCH 0/3] Perf uninitialized value fixes Arnaldo Carvalho de Melo
2019-08-22 21:29   ` Ian Rogers

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).