2019-07-24 19:26:39

by Numfor Mbiziwo-Tiapo

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

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 <[email protected]>
---
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);
--
2.22.0.657.g960e92d24f-goog

2019-07-25 14:08:30

by David Laight

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

From: Numfor Mbiziwo-Tiapo
> Sent: 24 July 2019 19:45
>
> 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.

Get the compiler fixed so that it knows the domain of the value can never
exceed the compile-time constant NR_ITERS.

> Signed-off-by: Numfor Mbiziwo-Tiapo <[email protected]>
> ---
> 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];

At least use [16]

>
> snprintf(proc_name, sizeof(proc_name), "p:%d\n", i);
> prctl(PR_SET_NAME, proc_name);
> --
> 2.22.0.657.g960e92d24f-goog

David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

2019-07-26 20:15:31

by Arnaldo Carvalho de Melo

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

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 <[email protected]>
> ---
> 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 <[email protected]>
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 <[email protected]>
Reviewed-by: Jiri Olsa <[email protected]>

2019-07-29 20:59:27

by Numfor Mbiziwo-Tiapo

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

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 <[email protected]>
---
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

2019-08-07 11:35:55

by Jiri Olsa

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

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 <[email protected]>

Acked-by: Jiri Olsa <[email protected]>

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
>

2019-10-25 22:40:46

by Ian Rogers

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

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 <[email protected]> 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 <[email protected]>
>
> Acked-by: Jiri Olsa <[email protected]>
>
> 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
> >

2020-07-09 00:58:14

by Ian Rogers

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

On Fri, Oct 25, 2019 at 3:11 PM Ian Rogers <[email protected]> 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 <[email protected]> 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 <[email protected]>
> >
> > Acked-by: Jiri Olsa <[email protected]>

Acked-by: Ian Rogers <[email protected]>

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

2020-07-09 15:39:47

by Arnaldo Carvalho de Melo

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

Em Wed, Jul 08, 2020 at 05:54:47PM -0700, Ian Rogers escreveu:
> On Fri, Oct 25, 2019 at 3:11 PM Ian Rogers <[email protected]> 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 <[email protected]> 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 <[email protected]>
> > >
> > > Acked-by: Jiri Olsa <[email protected]>
>
> Acked-by: Ian Rogers <[email protected]>
>
> 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