* [PATCH] tools lib traceevent: Handle dynamic array's element size properly
@ 2013-01-21 12:44 Jiri Olsa
2013-01-22 4:45 ` Steven Rostedt
0 siblings, 1 reply; 5+ messages in thread
From: Jiri Olsa @ 2013-01-21 12:44 UTC (permalink / raw)
To: linux-kernel
Cc: Jiri Olsa, Arnaldo Carvalho de Melo, Steven Rostedt,
Corey Ashford, Frederic Weisbecker, Ingo Molnar, Namhyung Kim,
Paul Mackerras, Peter Zijlstra
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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] tools lib traceevent: Handle dynamic array's element size properly
2013-01-21 12:44 [PATCH] tools lib traceevent: Handle dynamic array's element size properly Jiri Olsa
@ 2013-01-22 4:45 ` Steven Rostedt
2013-01-22 13:16 ` Jiri Olsa
0 siblings, 1 reply; 5+ messages in thread
From: Steven Rostedt @ 2013-01-22 4:45 UTC (permalink / raw)
To: Jiri Olsa
Cc: linux-kernel, Arnaldo Carvalho de Melo, Corey Ashford,
Frederic Weisbecker, Ingo Molnar, Namhyung Kim, Paul Mackerras,
Peter Zijlstra
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;
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] tools lib traceevent: Handle dynamic array's element size properly
2013-01-22 4:45 ` Steven Rostedt
@ 2013-01-22 13:16 ` Jiri Olsa
2013-01-24 19:28 ` Arnaldo Carvalho de Melo
0 siblings, 1 reply; 5+ messages in thread
From: Jiri Olsa @ 2013-01-22 13:16 UTC (permalink / raw)
To: Steven Rostedt
Cc: linux-kernel, Arnaldo Carvalho de Melo, Corey Ashford,
Frederic Weisbecker, Ingo Molnar, Namhyung Kim, Paul Mackerras,
Peter Zijlstra
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] tools lib traceevent: Handle dynamic array's element size properly
2013-01-22 13:16 ` Jiri Olsa
@ 2013-01-24 19:28 ` Arnaldo Carvalho de Melo
2013-01-25 7:53 ` Ingo Molnar
0 siblings, 1 reply; 5+ messages in thread
From: Arnaldo Carvalho de Melo @ 2013-01-24 19:28 UTC (permalink / raw)
To: Jiri Olsa
Cc: Steven Rostedt, linux-kernel, Corey Ashford, Frederic Weisbecker,
Ingo Molnar, Namhyung Kim, Paul Mackerras, Peter Zijlstra
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] tools lib traceevent: Handle dynamic array's element size properly
2013-01-24 19:28 ` Arnaldo Carvalho de Melo
@ 2013-01-25 7:53 ` Ingo Molnar
0 siblings, 0 replies; 5+ messages in thread
From: Ingo Molnar @ 2013-01-25 7:53 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo
Cc: Jiri Olsa, Steven Rostedt, linux-kernel, Corey Ashford,
Frederic Weisbecker, Ingo Molnar, Namhyung Kim, Paul Mackerras,
Peter Zijlstra
* 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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2013-01-25 7:53 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-01-21 12:44 [PATCH] tools lib traceevent: Handle dynamic array's element size properly Jiri Olsa
2013-01-22 4:45 ` Steven Rostedt
2013-01-22 13:16 ` Jiri Olsa
2013-01-24 19:28 ` Arnaldo Carvalho de Melo
2013-01-25 7:53 ` Ingo Molnar
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.