[RFC] perf: Prevent potential null dereference

[RFC] perf: Prevent potential null dereference

[Cyrill Gorcunov]

In case if there is no memory we might hit null
dereference on accessing calloc'ed data.

Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
CC: Arnaldo Carvalho de Melo <acme@redhat.com>
CC: Peter Zijlstra <peterz@infradead.org>
CC: Ingo Molnar <mingo@elte.hu>
CC: Frederic Weisbecker <fweisbec@gmail.com>
---

It seems exit right here is more convenient than passing error
handling level up (which would have to exit anyway), thought
if handling it "upper" is preferred -- just say a word.

 tools/perf/builtin-record.c |    4 ++++
 1 file changed, 4 insertions(+)

Index: linux-2.6.git/tools/perf/builtin-record.c
=====================================================================
--- linux-2.6.git.orig/tools/perf/builtin-record.c
+++ linux-2.6.git/tools/perf/builtin-record.c
@@ -524,6 +524,10 @@ static void comm__construct(int argc, co
 		return;
 
 	comm = calloc(1, size);
+	if (!comm) {
+		pr_err("Not enough memory to construct internal command line.\n");
+		exit(-1);
+	}
 
 	tmp = comm;
 	for (i = 0; i < argc; i++) {

Re: [RFC] perf: Prevent potential null dereference

[Frederic Weisbecker]

On Fri, Dec 03, 2010 at 01:26:05AM +0300, Cyrill Gorcunov wrote:
> In case if there is no memory we might hit null
> dereference on accessing calloc'ed data.
> 
> Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
> CC: Arnaldo Carvalho de Melo <acme@redhat.com>
> CC: Peter Zijlstra <peterz@infradead.org>
> CC: Ingo Molnar <mingo@elte.hu>
> CC: Frederic Weisbecker <fweisbec@gmail.com>
> ---
> 
> It seems exit right here is more convenient than passing error
> handling level up (which would have to exit anyway), thought
> if handling it "upper" is preferred -- just say a word.
> 
>  tools/perf/builtin-record.c |    4 ++++
>  1 file changed, 4 insertions(+)
> 
> Index: linux-2.6.git/tools/perf/builtin-record.c
> =====================================================================
> --- linux-2.6.git.orig/tools/perf/builtin-record.c
> +++ linux-2.6.git/tools/perf/builtin-record.c
> @@ -524,6 +524,10 @@ static void comm__construct(int argc, co
>  		return;
>  
>  	comm = calloc(1, size);
> +	if (!comm) {
> +		pr_err("Not enough memory to construct internal command line.\n");
> +		exit(-1);
> +	}
>  
>  	tmp = comm;
>  	for (i = 0; i < argc; i++) {


Good.

As a nit, not that it matters that much because we are very close to the starting code
anyway, but it would be better to propagate the error to the callers.

Thanks.

Re: [RFC] perf: Prevent potential null dereference

[Arnaldo Carvalho de Melo]

Em Thu, Dec 02, 2010 at 11:41:08PM +0100, Frederic Weisbecker escreveu:
> On Fri, Dec 03, 2010 at 01:26:05AM +0300, Cyrill Gorcunov wrote:
> > In case if there is no memory we might hit null
> > dereference on accessing calloc'ed data.
> > 
> > Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
> > CC: Arnaldo Carvalho de Melo <acme@redhat.com>
> > CC: Peter Zijlstra <peterz@infradead.org>
> > CC: Ingo Molnar <mingo@elte.hu>
> > CC: Frederic Weisbecker <fweisbec@gmail.com>
> > ---
> > 
> > It seems exit right here is more convenient than passing error
> > handling level up (which would have to exit anyway), thought
> > if handling it "upper" is preferred -- just say a word.
> > 
> >  tools/perf/builtin-record.c |    4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > Index: linux-2.6.git/tools/perf/builtin-record.c
> > =====================================================================
> > --- linux-2.6.git.orig/tools/perf/builtin-record.c
> > +++ linux-2.6.git/tools/perf/builtin-record.c
> > @@ -524,6 +524,10 @@ static void comm__construct(int argc, co
> >  		return;
> >  
> >  	comm = calloc(1, size);
> > +	if (!comm) {
> > +		pr_err("Not enough memory to construct internal command line.\n");
> > +		exit(-1);
> > +	}
> >  
> >  	tmp = comm;
> >  	for (i = 0; i < argc; i++) {
> 
> 
> Good.
> 
> As a nit, not that it matters that much because we are very close to the starting code
> anyway, but it would be better to propagate the error to the callers.

I'm of the opinion that main() should be where exit() is allowed, and
even there... return would be better. 8-)

- Arnaldo

Re: [RFC] perf: Prevent potential null dereference

[Cyrill Gorcunov]

On Thu, Dec 02, 2010 at 08:46:10PM -0200, Arnaldo Carvalho de Melo wrote:
...
> > > It seems exit right here is more convenient than passing error
> > > handling level up (which would have to exit anyway), thought
> > > if handling it "upper" is preferred -- just say a word.
> > > 
> > 
> > Good.
> > 
> > As a nit, not that it matters that much because we are very close to the starting code
> > anyway, but it would be better to propagate the error to the callers.
> 
> I'm of the opinion that main() should be where exit() is allowed, and
> even there... return would be better. 8-)
> 
> - Arnaldo
> 

ok, i'll update tomorrow (almost off).

  Cyrill

Re: [RFC] perf: Prevent potential null dereference

[Cyrill Gorcunov]

On Thu, Dec 02, 2010 at 08:46:10PM -0200, Arnaldo Carvalho de Melo wrote:
> Em Thu, Dec 02, 2010 at 11:41:08PM +0100, Frederic Weisbecker escreveu:
> > On Fri, Dec 03, 2010 at 01:26:05AM +0300, Cyrill Gorcunov wrote:
> > > In case if there is no memory we might hit null
> > > dereference on accessing calloc'ed data.
> > > 
> > > Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
> > > CC: Arnaldo Carvalho de Melo <acme@redhat.com>
> > > CC: Peter Zijlstra <peterz@infradead.org>
> > > CC: Ingo Molnar <mingo@elte.hu>
> > > CC: Frederic Weisbecker <fweisbec@gmail.com>
> > > ---
...
> > 
> > Good.
> > 
> > As a nit, not that it matters that much because we are very close to the starting code
> > anyway, but it would be better to propagate the error to the callers.
> 
> I'm of the opinion that main() should be where exit() is allowed, and
> even there... return would be better. 8-)
> 
> - Arnaldo
> 

ok, sorry for delay, it seems the following would be liked
more than first version ;)

  Cyrill
---
[PATCH] perf: Prevent potential null dereference v2

In case if there is no memory we might hit null
dereference on accessing calloc'ed data.

v2: propagate error to a caller

Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
CC: Arnaldo Carvalho de Melo <acme@redhat.com>,
CC: Peter Zijlstra <peterz@infradead.org>
CC: Ingo Molnar <mingo@elte.hu>
CC: Frederic Weisbecker <fweisbec@gmail.com>
---

NB it's unclear for me why don't we yield any message on
too long command line, but anyway even then it should not
be messed with this particular patch.

Arnaldo, I'll check builtin-kmem.c next time i be able to,
though if there anyone would like to beat me on this -- feel
free ;)

 tools/perf/builtin-record.c |   14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

Index: linux-2.6.git/tools/perf/builtin-record.c
=====================================================================
--- linux-2.6.git.orig/tools/perf/builtin-record.c
+++ linux-2.6.git/tools/perf/builtin-record.c
@@ -507,7 +507,7 @@ static void mmap_read_all(void)
 		write_output(&finished_round_event, sizeof(finished_round_event));
 }
 
-static void comm__construct(int argc, const char **argv)
+static int comm__construct(int argc, const char **argv)
 {
 	char *comm, *tmp;
 	size_t size;
@@ -521,9 +521,13 @@ static void comm__construct(int argc, co
 	}
 
 	if ((long)size < 0)
-		return;
+		return 0;
 
 	comm = calloc(1, size);
+	if (!comm) {
+		pr_err("Not enough memory to construct internal command line.\n");
+		return -ENOMEM;
+	}
 
 	tmp = comm;
 	for (i = 0; i < argc; i++) {
@@ -533,6 +537,7 @@ static void comm__construct(int argc, co
 	}
 
 	session->command_line = comm;
+	return 0;
 }
 
 static int __cmd_record(int argc, const char **argv)
@@ -597,7 +602,10 @@ static int __cmd_record(int argc, const 
 	if (!no_buildid)
 		perf_header__set_feat(&session->header, HEADER_BUILD_ID);
 
-	comm__construct(argc, argv);
+	err = comm__construct(argc, argv);
+	if (err < 0)
+		goto out_delete_session;
+
 
 	if (!file_new) {
 		err = perf_header__read(session, output);

Re: [RFC] perf: Prevent potential null dereference

[Arnaldo Carvalho de Melo]

Em Mon, Dec 06, 2010 at 01:13:21AM +0300, Cyrill Gorcunov escreveu:
> On Thu, Dec 02, 2010 at 08:46:10PM -0200, Arnaldo Carvalho de Melo wrote:
> > Em Thu, Dec 02, 2010 at 11:41:08PM +0100, Frederic Weisbecker escreveu:
> > > On Fri, Dec 03, 2010 at 01:26:05AM +0300, Cyrill Gorcunov wrote:
> > > > In case if there is no memory we might hit null
> > > > dereference on accessing calloc'ed data.
> > > > 
> > > > Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
> > > > CC: Arnaldo Carvalho de Melo <acme@redhat.com>
> > > > CC: Peter Zijlstra <peterz@infradead.org>
> > > > CC: Ingo Molnar <mingo@elte.hu>
> > > > CC: Frederic Weisbecker <fweisbec@gmail.com>
> > > > ---
> ...
> > > 
> > > Good.
> > > 
> > > As a nit, not that it matters that much because we are very close to the starting code
> > > anyway, but it would be better to propagate the error to the callers.
> > 
> > I'm of the opinion that main() should be where exit() is allowed, and
> > even there... return would be better. 8-)
> > 
> > - Arnaldo
> > 
> 
> ok, sorry for delay, it seems the following would be liked
> more than first version ;)
> 

Hey, what tree is this agains? I guess tip/master, right? If so, I still
didn't got there :-\

- Arnaldo

Re: [RFC] perf: Prevent potential null dereference

[Cyrill Gorcunov]

On Mon, Dec 06, 2010 at 12:49:55PM -0200, Arnaldo Carvalho de Melo wrote:
...
> 
> Hey, what tree is this agains? I guess tip/master, right? If so, I still
> didn't got there :-\

yup, it was -tip/master

 | commit 93edb4532cbdbc174a64ec4ab347a345159bc93e
 | Merge: 913b7a9 11e8896
 | Author: Ingo Molnar <mingo@elte.hu>
 | Date:   Sat Dec 4 11:28:13 2010 +0100
 |
 |   Merge branch 'linus'

> 
> - Arnaldo
> 
  Cyrill