[PATCH] perf bench: Fix NULL pointer dereference in "perf bench all"

[PATCH] perf bench: Fix NULL pointer dereference in "perf bench all"

[Patrick Palka]

for_each_bench() must check that the "benchmarks" field of a collection
is not NULL before dereferencing it because the "all" collection in
particular has a NULL "benchmarks" field (signifying that it has no
benchmarks to iterate over).

This fixes a NULL pointer dereference when running "perf bench all".

Signed-off-by: Patrick Palka <patrick@parcs.ath.cx>
---
 tools/perf/builtin-bench.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/builtin-bench.c b/tools/perf/builtin-bench.c
index e47f90c..8a987d2 100644
--- a/tools/perf/builtin-bench.c
+++ b/tools/perf/builtin-bench.c
@@ -76,7 +76,7 @@ static struct collection collections[] = {
 
 /* Iterate over all benchmarks within a collection: */
 #define for_each_bench(coll, bench) \
-	for (bench = coll->benchmarks; bench->name; bench++)
+	for (bench = coll->benchmarks; bench && bench->name; bench++)
 
 static void dump_benchmarks(struct collection *coll)
 {
-- 
1.9.0

Re: [PATCH] perf bench: Fix NULL pointer dereference in "perf bench all"

[Arnaldo Carvalho de Melo]

Em Wed, Mar 12, 2014 at 06:40:51PM -0400, Patrick Palka escreveu:
> for_each_bench() must check that the "benchmarks" field of a collection
> is not NULL before dereferencing it because the "all" collection in
> particular has a NULL "benchmarks" field (signifying that it has no
> benchmarks to iterate over).
> 
> This fixes a NULL pointer dereference when running "perf bench all".

Can you please mention against which tree you're fixing things? I just
tried to reproduce it here on perf/urgent and this problem is not
reproducible by simply running:

  perf bench all

So now I'm going to try on perf/core, tip/master, etc :-\

- Arnaldo
 
> Signed-off-by: Patrick Palka <patrick@parcs.ath.cx>
> ---
>  tools/perf/builtin-bench.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/perf/builtin-bench.c b/tools/perf/builtin-bench.c
> index e47f90c..8a987d2 100644
> --- a/tools/perf/builtin-bench.c
> +++ b/tools/perf/builtin-bench.c
> @@ -76,7 +76,7 @@ static struct collection collections[] = {
>  
>  /* Iterate over all benchmarks within a collection: */
>  #define for_each_bench(coll, bench) \
> -	for (bench = coll->benchmarks; bench->name; bench++)
> +	for (bench = coll->benchmarks; bench && bench->name; bench++)
>  
>  static void dump_benchmarks(struct collection *coll)
>  {
> -- 
> 1.9.0

Re: [PATCH] perf bench: Fix NULL pointer dereference in "perf bench all"

[Patrick Palka]

On Thu, Mar 13, 2014 at 10:34 AM, Arnaldo Carvalho de Melo
<acme@ghostprotocols.net> wrote:
> Em Wed, Mar 12, 2014 at 06:40:51PM -0400, Patrick Palka escreveu:
>> for_each_bench() must check that the "benchmarks" field of a collection
>> is not NULL before dereferencing it because the "all" collection in
>> particular has a NULL "benchmarks" field (signifying that it has no
>> benchmarks to iterate over).
>>
>> This fixes a NULL pointer dereference when running "perf bench all".
>
> Can you please mention against which tree you're fixing things? I just
> tried to reproduce it here on perf/urgent and this problem is not
> reproducible by simply running:
>
>   perf bench all
>
> So now I'm going to try on perf/core, tip/master, etc :-\

I'm fixing this on top on Linus' tree, which is currently on ac9dc67b7.

Without the patch, the output of "./perf bench all" looks like:

# Running sched/messaging benchmark...
# 20 sender and receiver processes per group
# 10 groups == 400 processes run

     Total time: 0.623 [sec]

# Running sched/pipe benchmark...
# Executed 1000000 pipe operations between two processes

     Total time: 32.199 [sec]

      32.199419 usecs/op
          31056 ops/sec

# Running mem/memcpy benchmark...
# Copying 1MB Bytes ...

       1.247206 GB/Sec
       1.268263 GB/Sec (with prefault)

# Running mem/memset benchmark...
# Copying 1MB Bytes ...

       1.429813 GB/Sec
       8.418642 GB/Sec (with prefault)

zsh: segmentation fault  ./perf bench all


Patrick

Re: [PATCH] perf bench: Fix NULL pointer dereference in "perf bench all"

[Arnaldo Carvalho de Melo]

Em Thu, Mar 13, 2014 at 10:47:58AM -0400, Patrick Palka escreveu:
> On Thu, Mar 13, 2014 at 10:34 AM, Arnaldo Carvalho de Melo
> <acme@ghostprotocols.net> wrote:
> > Em Wed, Mar 12, 2014 at 06:40:51PM -0400, Patrick Palka escreveu:
> >> for_each_bench() must check that the "benchmarks" field of a collection
> >> is not NULL before dereferencing it because the "all" collection in
> >> particular has a NULL "benchmarks" field (signifying that it has no
> >> benchmarks to iterate over).

> >> This fixes a NULL pointer dereference when running "perf bench all".

> > Can you please mention against which tree you're fixing things? I just
> > tried to reproduce it here on perf/urgent and this problem is not
> > reproducible by simply running:

> >   perf bench all

> > So now I'm going to try on perf/core, tip/master, etc :-\
 
> I'm fixing this on top on Linus' tree, which is currently on ac9dc67b7.
 
> Without the patch, the output of "./perf bench all" looks like:

That helps, I didn't manage to reproduce it here, I think, because I
have the numa libs installed and then a numa bench is included, and it
seems broken, which stops 'perf bench all' before it hits this problem.

I'm fixing that problem and then will process your patch.

Please add the output, possibly trimmed if long, in your next changelog
messages, that helps in more quickly seeing what is happening and
comparing with results obtained elsewhere.

Thanks,

- Arnaldo
 
> # Running sched/messaging benchmark...
> # 20 sender and receiver processes per group
> # 10 groups == 400 processes run
> 
>      Total time: 0.623 [sec]
> 
> # Running sched/pipe benchmark...
> # Executed 1000000 pipe operations between two processes
> 
>      Total time: 32.199 [sec]
> 
>       32.199419 usecs/op
>           31056 ops/sec
> 
> # Running mem/memcpy benchmark...
> # Copying 1MB Bytes ...
> 
>        1.247206 GB/Sec
>        1.268263 GB/Sec (with prefault)
> 
> # Running mem/memset benchmark...
> # Copying 1MB Bytes ...
> 
>        1.429813 GB/Sec
>        8.418642 GB/Sec (with prefault)
> 
> zsh: segmentation fault  ./perf bench all
> 
> 
> Patrick

[tip:perf/urgent] perf bench: Fix NULL pointer dereference in " perf bench all"

[tip-bot for Patrick Palka]

Commit-ID:  6eeefccdcfc2cc9697562e740bfe6c35fddd4e1c
Gitweb:     http://git.kernel.org/tip/6eeefccdcfc2cc9697562e740bfe6c35fddd4e1c
Author:     Patrick Palka <patrick@parcs.ath.cx>
AuthorDate: Wed, 12 Mar 2014 18:40:51 -0400
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Fri, 14 Mar 2014 13:45:54 -0300

perf bench: Fix NULL pointer dereference in "perf bench all"

The for_each_bench() macro must check that the "benchmarks" field of a
collection is not NULL before dereferencing it because the "all"
collection in particular has a NULL "benchmarks" field (signifying that
it has no benchmarks to iterate over).

This fixes this NULL pointer dereference when running "perf bench all":

  [root@ssdandy ~]# perf bench all
  <SNIP>

  # Running mem/memset benchmark...
  # Copying 1MB Bytes ...

         2.453675 GB/Sec
        12.056327 GB/Sec (with prefault)

  Segmentation fault (core dumped)
  [root@ssdandy ~]#

Signed-off-by: Patrick Palka <patrick@parcs.ath.cx>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Link: http://lkml.kernel.org/r/1394664051-6037-1-git-send-email-patrick@parcs.ath.cx
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/builtin-bench.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/builtin-bench.c b/tools/perf/builtin-bench.c
index e47f90c..8a987d2 100644
--- a/tools/perf/builtin-bench.c
+++ b/tools/perf/builtin-bench.c
@@ -76,7 +76,7 @@ static struct collection collections[] = {
 
 /* Iterate over all benchmarks within a collection: */
 #define for_each_bench(coll, bench) \
-	for (bench = coll->benchmarks; bench->name; bench++)
+	for (bench = coll->benchmarks; bench && bench->name; bench++)
 
 static void dump_benchmarks(struct collection *coll)
 {