[PATCH] tools lib traceevent: Handle dynamic array's element size properly

[PATCH] tools lib traceevent: Handle dynamic array's element size properly

[Jiri Olsa]

Fixing the dynamic array format field parsing.

Currently the event_read_fields function could segfault while parsing
dynamic array other than string type. The reason is the event->pevent
does not need to be set and gets dereferenced unconditionaly.

Also adding proper initialization of field->elementsize based on the
parsed dynamic type.

Signed-off-by: Jiri Olsa <jolsa@redhat.com>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Corey Ashford <cjashfor@linux.vnet.ibm.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
---
 tools/lib/traceevent/event-parse.c | 40 +++++++++++++++++++++++++++++++++++---
 tools/lib/traceevent/event-parse.h |  1 +
 2 files changed, 38 insertions(+), 3 deletions(-)

diff --git a/tools/lib/traceevent/event-parse.c b/tools/lib/traceevent/event-parse.c
index f504619..d682df2 100644
--- a/tools/lib/traceevent/event-parse.c
+++ b/tools/lib/traceevent/event-parse.c
@@ -1223,6 +1223,34 @@ static int field_is_long(struct format_field *field)
 	return 0;
 }
 
+static unsigned int field_dynamic_elem_size(struct format_field *field)
+{
+	/* This covers all FIELD_IS_STRING types. */
+	static struct {
+		char *type;
+		unsigned int size;
+	} table[] = {
+		{ "u8",   1 },
+		{ "u16",  2 },
+		{ "u32",  4 },
+		{ "u64",  8 },
+		{ "s8",   1 },
+		{ "s16",  2 },
+		{ "s32",  4 },
+		{ "s64",  8 },
+		{ "char", 1 },
+		{ },
+	};
+	int i;
+
+	for (i = 0; table[i].type; i++) {
+		if (!strcmp(table[i].type, field->type_dyn))
+			return table[i].size;
+	}
+
+	return 0;
+}
+
 static int event_read_fields(struct event_format *event, struct format_field **fields)
 {
 	struct format_field *field = NULL;
@@ -1390,7 +1418,7 @@ static int event_read_fields(struct event_format *event, struct format_field **f
 				field->type = new_type;
 				strcat(field->type, " ");
 				strcat(field->type, field->name);
-				free_token(field->name);
+				field->type_dyn = field->name;
 				strcat(field->type, brackets);
 				field->name = token;
 				type = read_token(&token);
@@ -1477,10 +1505,14 @@ static int event_read_fields(struct event_format *event, struct format_field **f
 		if (field->flags & FIELD_IS_ARRAY) {
 			if (field->arraylen)
 				field->elementsize = field->size / field->arraylen;
+			else if (field->flags & FIELD_IS_DYNAMIC)
+				field->elementsize = field_dynamic_elem_size(field);
 			else if (field->flags & FIELD_IS_STRING)
 				field->elementsize = 1;
-			else
-				field->elementsize = event->pevent->long_size;
+			else if (field->flags & FIELD_IS_LONG)
+				field->elementsize = event->pevent ?
+						     event->pevent->long_size :
+						     sizeof(long);
 		} else
 			field->elementsize = field->size;
 
@@ -1496,6 +1528,7 @@ fail:
 fail_expect:
 	if (field) {
 		free(field->type);
+		free(field->type_dyn);
 		free(field->name);
 		free(field);
 	}
@@ -5500,6 +5533,7 @@ static void free_format_fields(struct format_field *field)
 	while (field) {
 		next = field->next;
 		free(field->type);
+		free(field->type_dyn);
 		free(field->name);
 		free(field);
 		field = next;
diff --git a/tools/lib/traceevent/event-parse.h b/tools/lib/traceevent/event-parse.h
index 7be7e89..4d54af2 100644
--- a/tools/lib/traceevent/event-parse.h
+++ b/tools/lib/traceevent/event-parse.h
@@ -174,6 +174,7 @@ struct format_field {
 	struct format_field	*next;
 	struct event_format	*event;
 	char			*type;
+	char			*type_dyn;
 	char			*name;
 	int			offset;
 	int			size;
-- 
1.7.11.7

Re: [PATCH] tools lib traceevent: Handle dynamic array's element size properly

[Steven Rostedt]

On Mon, 2013-01-21 at 13:44 +0100, Jiri Olsa wrote:
> Fixing the dynamic array format field parsing.
> 
> Currently the event_read_fields function could segfault while parsing
> dynamic array other than string type. The reason is the event->pevent
> does not need to be set and gets dereferenced unconditionaly.
> 
> Also adding proper initialization of field->elementsize based on the
> parsed dynamic type.
> 
> Signed-off-by: Jiri Olsa <jolsa@redhat.com>
> Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Cc: Corey Ashford <cjashfor@linux.vnet.ibm.com>
> Cc: Frederic Weisbecker <fweisbec@gmail.com>
> Cc: Ingo Molnar <mingo@elte.hu>
> Cc: Namhyung Kim <namhyung@kernel.org>
> Cc: Paul Mackerras <paulus@samba.org>
> Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
> ---
>  tools/lib/traceevent/event-parse.c | 40 +++++++++++++++++++++++++++++++++++---
>  tools/lib/traceevent/event-parse.h |  1 +
>  2 files changed, 38 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/lib/traceevent/event-parse.c b/tools/lib/traceevent/event-parse.c
> index f504619..d682df2 100644
> --- a/tools/lib/traceevent/event-parse.c
> +++ b/tools/lib/traceevent/event-parse.c
> @@ -1223,6 +1223,34 @@ static int field_is_long(struct format_field *field)
>  	return 0;
>  }
>  
> +static unsigned int field_dynamic_elem_size(struct format_field *field)
> +{
> +	/* This covers all FIELD_IS_STRING types. */
> +	static struct {
> +		char *type;
> +		unsigned int size;
> +	} table[] = {
> +		{ "u8",   1 },
> +		{ "u16",  2 },
> +		{ "u32",  4 },
> +		{ "u64",  8 },
> +		{ "s8",   1 },
> +		{ "s16",  2 },
> +		{ "s32",  4 },
> +		{ "s64",  8 },
> +		{ "char", 1 },
> +		{ },
> +	};
> +	int i;
> +
> +	for (i = 0; table[i].type; i++) {
> +		if (!strcmp(table[i].type, field->type_dyn))
> +			return table[i].size;
> +	}
> +
> +	return 0;
> +}
> +
>  static int event_read_fields(struct event_format *event, struct format_field **fields)
>  {
>  	struct format_field *field = NULL;
> @@ -1390,7 +1418,7 @@ static int event_read_fields(struct event_format *event, struct format_field **f
>  				field->type = new_type;
>  				strcat(field->type, " ");
>  				strcat(field->type, field->name);
> -				free_token(field->name);
> +				field->type_dyn = field->name;

This is only used in this function (the field_dynamic_elem_size() is
only called here). Can we not add the field->type_dyn, and just use a
local variable here. You just need to make sure you free it correctly.

-- Steve

>  				strcat(field->type, brackets);
>  				field->name = token;
>  				type = read_token(&token);
> @@ -1477,10 +1505,14 @@ static int event_read_fields(struct event_format *event, struct format_field **f
>  		if (field->flags & FIELD_IS_ARRAY) {
>  			if (field->arraylen)
>  				field->elementsize = field->size / field->arraylen;
> +			else if (field->flags & FIELD_IS_DYNAMIC)
> +				field->elementsize = field_dynamic_elem_size(field);
>  			else if (field->flags & FIELD_IS_STRING)
>  				field->elementsize = 1;
> -			else
> -				field->elementsize = event->pevent->long_size;
> +			else if (field->flags & FIELD_IS_LONG)
> +				field->elementsize = event->pevent ?
> +						     event->pevent->long_size :
> +						     sizeof(long);
>  		} else
>  			field->elementsize = field->size;
>  
> @@ -1496,6 +1528,7 @@ fail:
>  fail_expect:
>  	if (field) {
>  		free(field->type);
> +		free(field->type_dyn);
>  		free(field->name);
>  		free(field);
>  	}
> @@ -5500,6 +5533,7 @@ static void free_format_fields(struct format_field *field)
>  	while (field) {
>  		next = field->next;
>  		free(field->type);
> +		free(field->type_dyn);
>  		free(field->name);
>  		free(field);
>  		field = next;
> diff --git a/tools/lib/traceevent/event-parse.h b/tools/lib/traceevent/event-parse.h
> index 7be7e89..4d54af2 100644
> --- a/tools/lib/traceevent/event-parse.h
> +++ b/tools/lib/traceevent/event-parse.h
> @@ -174,6 +174,7 @@ struct format_field {
>  	struct format_field	*next;
>  	struct event_format	*event;
>  	char			*type;
> +	char			*type_dyn;
>  	char			*name;
>  	int			offset;
>  	int			size;

Re: [PATCH] tools lib traceevent: Handle dynamic array's element size properly

[Jiri Olsa]

On Mon, Jan 21, 2013 at 11:45:42PM -0500, Steven Rostedt wrote:
> On Mon, 2013-01-21 at 13:44 +0100, Jiri Olsa wrote:

SNIP

> > +	for (i = 0; table[i].type; i++) {
> > +		if (!strcmp(table[i].type, field->type_dyn))
> > +			return table[i].size;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> >  static int event_read_fields(struct event_format *event, struct format_field **fields)
> >  {
> >  	struct format_field *field = NULL;
> > @@ -1390,7 +1418,7 @@ static int event_read_fields(struct event_format *event, struct format_field **f
> >  				field->type = new_type;
> >  				strcat(field->type, " ");
> >  				strcat(field->type, field->name);
> > -				free_token(field->name);
> > +				field->type_dyn = field->name;
> 
> This is only used in this function (the field_dynamic_elem_size() is
> only called here). Can we not add the field->type_dyn, and just use a
> local variable here. You just need to make sure you free it correctly.

ook, will send v2

thanks,
jirka

Re: [PATCH] tools lib traceevent: Handle dynamic array's element size properly

[Arnaldo Carvalho de Melo]

Em Tue, Jan 22, 2013 at 02:16:47PM +0100, Jiri Olsa escreveu:
> On Mon, Jan 21, 2013 at 11:45:42PM -0500, Steven Rostedt wrote:
> > This is only used in this function (the field_dynamic_elem_size() is
> > only called here). Can we not add the field->type_dyn, and just use a
> > local variable here. You just need to make sure you free it correctly.
> 
> ook, will send v2

Waiting for this fix, just got 'perf test' crashing on it again :-)

Till then I implemented 'perf test --skip':


[root@sandy ~]# perf test 
 1: vmlinux symtab matches kallsyms                        : Ok
 2: detect open syscall event                              : Ok
 3: detect open syscall event on all cpus                  : Ok
 4: read samples using the mmap interface                  : Ok
 5: parse events tests                                     :  Warning:
bad op token {
  Warning: bad op token {
  Warning: bad op token {
  Warning: bad op token {
  Warning: bad op token {
  Warning: function is_writable_pte not defined
Segmentation fault (core dumped)
[root@sandy ~]# perf test -s 5
 1: vmlinux symtab matches kallsyms                        : Ok
 2: detect open syscall event                              : Ok
 3: detect open syscall event on all cpus                  : Ok
 4: read samples using the mmap interface                  : Ok
 5: parse events tests                                     : Skip (user override)
 6: x86 rdpmc test                                         : Ok
 7: Validate PERF_RECORD_* events & perf_sample fields     : Ok
 8: Test perf pmu format parsing                           : Ok
 9: Test dso data interface                                : Ok
10: roundtrip evsel->name check                            : Ok
11: Check parsing of sched tracepoints fields              : Ok
12: Generate and check syscalls:sys_enter_open event fields: Ok
13: struct perf_event_attr setup                           : Ok
14: Test matching and linking mutliple hists               : Ok
15: Try 'use perf' in python, checking link problems       : Ok
[root@sandy ~]#

:-)

- Arnaldo

Re: [PATCH] tools lib traceevent: Handle dynamic array's element size properly

[Ingo Molnar]

* Arnaldo Carvalho de Melo <acme@redhat.com> wrote:

> Em Tue, Jan 22, 2013 at 02:16:47PM +0100, Jiri Olsa escreveu:
> > On Mon, Jan 21, 2013 at 11:45:42PM -0500, Steven Rostedt wrote:
> > > This is only used in this function (the field_dynamic_elem_size() is
> > > only called here). Can we not add the field->type_dyn, and just use a
> > > local variable here. You just need to make sure you free it correctly.
> > 
> > ook, will send v2
> 
> Waiting for this fix, just got 'perf test' crashing on it again :-)
> 
> Till then I implemented 'perf test --skip':

Hm, I think perf test should fork() a testing thread and be 
robust against a child catching a SIGSEGV?

That would also protect against a test corrupting perf test 
itself.

Thanks,

	Ingo