hi,
there's a way to crash perf top command by specifying multiple
events. Attached is the fix and change to eliminate the events
output in case many events are specified.
attached patches:
1/2 - perf,top: fix events overflow in top command
2/2 - perf,top: dont let events to eat up whole header line
wbr,
jirka
---
tools/perf/util/top.c | 48 ++++++++++++++++++++++++++++++++++--------------
1 files changed, 34 insertions(+), 14 deletions(-)
Passing multiple events might force out information about pid/tid/cpu.
Attached patch leaves 30 characters for this info at the expense of
the events' names.
wbr,
jirka
Signed-off-by: Jiri Olsa <[email protected]>
---
tools/perf/util/top.c | 20 +++++++++++++++++---
1 files changed, 17 insertions(+), 3 deletions(-)
diff --git a/tools/perf/util/top.c b/tools/perf/util/top.c
index 4f869da..75cfe4d 100644
--- a/tools/perf/util/top.c
+++ b/tools/perf/util/top.c
@@ -115,9 +115,23 @@ size_t perf_top__header_snprintf(struct perf_top *top, char *bf, size_t size)
if (!top->display_weighted) {
ret += SNPRINTF(bf + ret, size - ret, "%s",
event_name(top->sym_evsel));
- } else list_for_each_entry(counter, &top->evlist->entries, node) {
- ret += SNPRINTF(bf + ret, size - ret, "%s%s",
- counter->idx ? "/" : "", event_name(counter));
+ } else {
+ /*
+ * Don't let events eat all the space. Leaving 30 bytes
+ * for the rest should be enough.
+ */
+ size_t last_pos = size - 30;
+
+ list_for_each_entry(counter, &top->evlist->entries, node) {
+ ret += SNPRINTF(bf + ret, size - ret, "%s%s",
+ counter->idx ? "/" : "",
+ event_name(counter));
+ if (ret > last_pos) {
+ sprintf(bf + last_pos - 3, "..");
+ ret = last_pos - 1;
+ break;
+ }
+ }
}
ret += SNPRINTF(bf + ret, size - ret, "], ");
--
1.7.4
The snprintf function returns number of printed characters even
if it cross the size parameter. So passing enough events via '-e'
parameter will cause segmentation fault.
It's reproduced by following command:
perf top -e `perf list | grep Tracepoint | awk -F'[' '\
{gsub(/[[:space:]]+/,"",$1);array[FNR]=$1}END{outputs=array[1];\
for (i=2;i<=FNR;i++){ outputs=outputs "," array[i];};print outputs}'`
Attached patch is adding SNPRINTF macro that provides the
overflow check and returns actuall number of printed characters.
wbr,
jirka
Reported-by: Han Pingtian <[email protected]>
Signed-off-by: Jiri Olsa <[email protected]>
---
tools/perf/util/top.c | 30 ++++++++++++++++++------------
1 files changed, 18 insertions(+), 12 deletions(-)
diff --git a/tools/perf/util/top.c b/tools/perf/util/top.c
index 70a9c13..4f869da 100644
--- a/tools/perf/util/top.c
+++ b/tools/perf/util/top.c
@@ -61,6 +61,12 @@ static void rb_insert_active_sym(struct rb_root *tree, struct sym_entry *se)
rb_insert_color(&se->rb_node, tree);
}
+#define SNPRINTF(buf, size, fmt, args...) \
+({ \
+ size_t r = snprintf(buf, size, fmt, ## args); \
+ r > size ? size : r; \
+})
+
size_t perf_top__header_snprintf(struct perf_top *top, char *bf, size_t size)
{
struct perf_evsel *counter;
@@ -70,7 +76,7 @@ size_t perf_top__header_snprintf(struct perf_top *top, char *bf, size_t size)
size_t ret = 0;
if (!perf_guest) {
- ret = snprintf(bf, size,
+ ret = SNPRINTF(bf, size,
" PerfTop:%8.0f irqs/sec kernel:%4.1f%%"
" exact: %4.1f%% [", samples_per_sec,
100.0 - (100.0 * ((samples_per_sec - ksamples_per_sec) /
@@ -81,7 +87,7 @@ size_t perf_top__header_snprintf(struct perf_top *top, char *bf, size_t size)
float guest_kernel_samples_per_sec = top->guest_kernel_samples / top->delay_secs;
float guest_us_samples_per_sec = top->guest_us_samples / top->delay_secs;
- ret = snprintf(bf, size,
+ ret = SNPRINTF(bf, size,
" PerfTop:%8.0f irqs/sec kernel:%4.1f%% us:%4.1f%%"
" guest kernel:%4.1f%% guest us:%4.1f%%"
" exact: %4.1f%% [", samples_per_sec,
@@ -101,38 +107,38 @@ size_t perf_top__header_snprintf(struct perf_top *top, char *bf, size_t size)
if (top->evlist->nr_entries == 1 || !top->display_weighted) {
struct perf_evsel *first;
first = list_entry(top->evlist->entries.next, struct perf_evsel, node);
- ret += snprintf(bf + ret, size - ret, "%" PRIu64 "%s ",
+ ret += SNPRINTF(bf + ret, size - ret, "%" PRIu64 "%s ",
(uint64_t)first->attr.sample_period,
top->freq ? "Hz" : "");
}
if (!top->display_weighted) {
- ret += snprintf(bf + ret, size - ret, "%s",
+ ret += SNPRINTF(bf + ret, size - ret, "%s",
event_name(top->sym_evsel));
} else list_for_each_entry(counter, &top->evlist->entries, node) {
- ret += snprintf(bf + ret, size - ret, "%s%s",
+ ret += SNPRINTF(bf + ret, size - ret, "%s%s",
counter->idx ? "/" : "", event_name(counter));
}
- ret += snprintf(bf + ret, size - ret, "], ");
+ ret += SNPRINTF(bf + ret, size - ret, "], ");
if (top->target_pid != -1)
- ret += snprintf(bf + ret, size - ret, " (target_pid: %d",
+ ret += SNPRINTF(bf + ret, size - ret, " (target_pid: %d",
top->target_pid);
else if (top->target_tid != -1)
- ret += snprintf(bf + ret, size - ret, " (target_tid: %d",
+ ret += SNPRINTF(bf + ret, size - ret, " (target_tid: %d",
top->target_tid);
else
- ret += snprintf(bf + ret, size - ret, " (all");
+ ret += SNPRINTF(bf + ret, size - ret, " (all");
if (top->cpu_list)
- ret += snprintf(bf + ret, size - ret, ", CPU%s: %s)",
+ ret += SNPRINTF(bf + ret, size - ret, ", CPU%s: %s)",
top->evlist->cpus->nr > 1 ? "s" : "", top->cpu_list);
else {
if (top->target_tid != -1)
- ret += snprintf(bf + ret, size - ret, ")");
+ ret += SNPRINTF(bf + ret, size - ret, ")");
else
- ret += snprintf(bf + ret, size - ret, ", %d CPU%s)",
+ ret += SNPRINTF(bf + ret, size - ret, ", %d CPU%s)",
top->evlist->cpus->nr,
top->evlist->cpus->nr > 1 ? "s" : "");
}
--
1.7.4
Em Mon, Mar 07, 2011 at 09:13:40PM +0100, Jiri Olsa escreveu:
> The snprintf function returns number of printed characters even
> if it cross the size parameter. So passing enough events via '-e'
> parameter will cause segmentation fault.
>
> It's reproduced by following command:
>
> perf top -e `perf list | grep Tracepoint | awk -F'[' '\
> {gsub(/[[:space:]]+/,"",$1);array[FNR]=$1}END{outputs=array[1];\
> for (i=2;i<=FNR;i++){ outputs=outputs "," array[i];};print outputs}'`
>
> Attached patch is adding SNPRINTF macro that provides the
> overflow check and returns actuall number of printed characters.
Good catch, applying to perf/urgent.
- Arnaldo
Em Wed, Mar 09, 2011 at 10:59:25AM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Mon, Mar 07, 2011 at 09:13:40PM +0100, Jiri Olsa escreveu:
> > The snprintf function returns number of printed characters even
> > if it cross the size parameter. So passing enough events via '-e'
> > parameter will cause segmentation fault.
> >
> > It's reproduced by following command:
> >
> > perf top -e `perf list | grep Tracepoint | awk -F'[' '\
> > {gsub(/[[:space:]]+/,"",$1);array[FNR]=$1}END{outputs=array[1];\
> > for (i=2;i<=FNR;i++){ outputs=outputs "," array[i];};print outputs}'`
> >
> > Attached patch is adding SNPRINTF macro that provides the
> > overflow check and returns actuall number of printed characters.
>
> Good catch, applying to perf/urgent.
Tried but it doesn't apply nor can I reproduce the overflow (albeit
granted the long line of events is annoying).
So I'm applying both patches to perf/core instead.
- Arnaldo
On Wed, Mar 09, 2011 at 11:06:10AM -0300, Arnaldo Carvalho de Melo wrote:
> Em Wed, Mar 09, 2011 at 10:59:25AM -0300, Arnaldo Carvalho de Melo escreveu:
> > Em Mon, Mar 07, 2011 at 09:13:40PM +0100, Jiri Olsa escreveu:
> > > The snprintf function returns number of printed characters even
> > > if it cross the size parameter. So passing enough events via '-e'
> > > parameter will cause segmentation fault.
> > >
> > > It's reproduced by following command:
> > >
> > > perf top -e `perf list | grep Tracepoint | awk -F'[' '\
> > > {gsub(/[[:space:]]+/,"",$1);array[FNR]=$1}END{outputs=array[1];\
> > > for (i=2;i<=FNR;i++){ outputs=outputs "," array[i];};print outputs}'`
> > >
> > > Attached patch is adding SNPRINTF macro that provides the
> > > overflow check and returns actuall number of printed characters.
> >
> > Good catch, applying to perf/urgent.
>
> Tried but it doesn't apply nor can I reproduce the overflow (albeit
> granted the long line of events is annoying).
patches were based on tip tree
hm, it crashes for me on tip tree,
but I haven't checked the perf/core tree..
not sure if it's any help, but while debugging I could see the sigsegv happened
when the pointer was far beyond the 160 chars buffer size,
maybe the buffer got bigger in the perf tree..
jirka
>
> So I'm applying both patches to perf/core instead.
>
> - Arnaldo
Em Wed, Mar 09, 2011 at 03:19:10PM +0100, Jiri Olsa escreveu:
> On Wed, Mar 09, 2011 at 11:06:10AM -0300, Arnaldo Carvalho de Melo wrote:
> > Em Wed, Mar 09, 2011 at 10:59:25AM -0300, Arnaldo Carvalho de Melo escreveu:
> > > Good catch, applying to perf/urgent.
> >
> > Tried but it doesn't apply nor can I reproduce the overflow (albeit
> > granted the long line of events is annoying).
>
> patches were based on tip tree
>
> hm, it crashes for me on tip tree,
> but I haven't checked the perf/core tree..
>
> not sure if it's any help, but while debugging I could see the sigsegv happened
> when the pointer was far beyond the 160 chars buffer size,
> maybe the buffer got bigger in the perf tree..
Anyway, using tip/master you are using perf/core so the patch was made
against it, I already merged it, will be on my next pull request to
Ingo.
Thanks,
- Arnaldo
Commit-ID: b9a46bba88001504235459c8410f17e6a7e38008
Gitweb: http://git.kernel.org/tip/b9a46bba88001504235459c8410f17e6a7e38008
Author: Jiri Olsa <[email protected]>
AuthorDate: Mon, 7 Mar 2011 21:13:40 +0100
Committer: Arnaldo Carvalho de Melo <[email protected]>
CommitDate: Thu, 10 Mar 2011 10:54:13 -0300
perf top: Fix events overflow in top command
The snprintf function returns number of printed characters even if it
cross the size parameter. So passing enough events via '-e' parameter
will cause segmentation fault.
It's reproduced by following command:
perf top -e `perf list | grep Tracepoint | awk -F'[' '\
{gsub(/[[:space:]]+/,"",$1);array[FNR]=$1}END{outputs=array[1];\
for (i=2;i<=FNR;i++){ outputs=outputs "," array[i];};print outputs}'`
Attached patch is adding SNPRINTF macro that provides the overflow check
and returns actuall number of printed characters.
Reported-by: Han Pingtian <[email protected]>
Cc: Han Pingtian <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: Paul Mackerras <[email protected]>
Cc: Peter Zijlstra <[email protected]>
LKML-Reference: <[email protected]>
Signed-off-by: Jiri Olsa <[email protected]>
Signed-off-by: Arnaldo Carvalho de Melo <[email protected]>
---
tools/perf/util/top.c | 30 ++++++++++++++++++------------
1 files changed, 18 insertions(+), 12 deletions(-)
diff --git a/tools/perf/util/top.c b/tools/perf/util/top.c
index 70a9c13..4f869da 100644
--- a/tools/perf/util/top.c
+++ b/tools/perf/util/top.c
@@ -61,6 +61,12 @@ static void rb_insert_active_sym(struct rb_root *tree, struct sym_entry *se)
rb_insert_color(&se->rb_node, tree);
}
+#define SNPRINTF(buf, size, fmt, args...) \
+({ \
+ size_t r = snprintf(buf, size, fmt, ## args); \
+ r > size ? size : r; \
+})
+
size_t perf_top__header_snprintf(struct perf_top *top, char *bf, size_t size)
{
struct perf_evsel *counter;
@@ -70,7 +76,7 @@ size_t perf_top__header_snprintf(struct perf_top *top, char *bf, size_t size)
size_t ret = 0;
if (!perf_guest) {
- ret = snprintf(bf, size,
+ ret = SNPRINTF(bf, size,
" PerfTop:%8.0f irqs/sec kernel:%4.1f%%"
" exact: %4.1f%% [", samples_per_sec,
100.0 - (100.0 * ((samples_per_sec - ksamples_per_sec) /
@@ -81,7 +87,7 @@ size_t perf_top__header_snprintf(struct perf_top *top, char *bf, size_t size)
float guest_kernel_samples_per_sec = top->guest_kernel_samples / top->delay_secs;
float guest_us_samples_per_sec = top->guest_us_samples / top->delay_secs;
- ret = snprintf(bf, size,
+ ret = SNPRINTF(bf, size,
" PerfTop:%8.0f irqs/sec kernel:%4.1f%% us:%4.1f%%"
" guest kernel:%4.1f%% guest us:%4.1f%%"
" exact: %4.1f%% [", samples_per_sec,
@@ -101,38 +107,38 @@ size_t perf_top__header_snprintf(struct perf_top *top, char *bf, size_t size)
if (top->evlist->nr_entries == 1 || !top->display_weighted) {
struct perf_evsel *first;
first = list_entry(top->evlist->entries.next, struct perf_evsel, node);
- ret += snprintf(bf + ret, size - ret, "%" PRIu64 "%s ",
+ ret += SNPRINTF(bf + ret, size - ret, "%" PRIu64 "%s ",
(uint64_t)first->attr.sample_period,
top->freq ? "Hz" : "");
}
if (!top->display_weighted) {
- ret += snprintf(bf + ret, size - ret, "%s",
+ ret += SNPRINTF(bf + ret, size - ret, "%s",
event_name(top->sym_evsel));
} else list_for_each_entry(counter, &top->evlist->entries, node) {
- ret += snprintf(bf + ret, size - ret, "%s%s",
+ ret += SNPRINTF(bf + ret, size - ret, "%s%s",
counter->idx ? "/" : "", event_name(counter));
}
- ret += snprintf(bf + ret, size - ret, "], ");
+ ret += SNPRINTF(bf + ret, size - ret, "], ");
if (top->target_pid != -1)
- ret += snprintf(bf + ret, size - ret, " (target_pid: %d",
+ ret += SNPRINTF(bf + ret, size - ret, " (target_pid: %d",
top->target_pid);
else if (top->target_tid != -1)
- ret += snprintf(bf + ret, size - ret, " (target_tid: %d",
+ ret += SNPRINTF(bf + ret, size - ret, " (target_tid: %d",
top->target_tid);
else
- ret += snprintf(bf + ret, size - ret, " (all");
+ ret += SNPRINTF(bf + ret, size - ret, " (all");
if (top->cpu_list)
- ret += snprintf(bf + ret, size - ret, ", CPU%s: %s)",
+ ret += SNPRINTF(bf + ret, size - ret, ", CPU%s: %s)",
top->evlist->cpus->nr > 1 ? "s" : "", top->cpu_list);
else {
if (top->target_tid != -1)
- ret += snprintf(bf + ret, size - ret, ")");
+ ret += SNPRINTF(bf + ret, size - ret, ")");
else
- ret += snprintf(bf + ret, size - ret, ", %d CPU%s)",
+ ret += SNPRINTF(bf + ret, size - ret, ", %d CPU%s)",
top->evlist->cpus->nr,
top->evlist->cpus->nr > 1 ? "s" : "");
}
Commit-ID: 6547250381eb315acff3d52b4872ad775359407c
Gitweb: http://git.kernel.org/tip/6547250381eb315acff3d52b4872ad775359407c
Author: Jiri Olsa <[email protected]>
AuthorDate: Mon, 7 Mar 2011 21:13:41 +0100
Committer: Arnaldo Carvalho de Melo <[email protected]>
CommitDate: Thu, 10 Mar 2011 10:55:00 -0300
perf top: Don't let events to eat up whole header line
Passing multiple events might force out information about pid/tid/cpu.
Attached patch leaves 30 characters for this info at the expense of the
events' names.
Cc: Ingo Molnar <[email protected]>
Cc: Paul Mackerras <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Han Pingtian <[email protected]>
LKML-Reference: <[email protected]>
Signed-off-by: Jiri Olsa <[email protected]>
Signed-off-by: Arnaldo Carvalho de Melo <[email protected]>
---
tools/perf/util/top.c | 20 +++++++++++++++++---
1 files changed, 17 insertions(+), 3 deletions(-)
diff --git a/tools/perf/util/top.c b/tools/perf/util/top.c
index 4f869da..75cfe4d 100644
--- a/tools/perf/util/top.c
+++ b/tools/perf/util/top.c
@@ -115,9 +115,23 @@ size_t perf_top__header_snprintf(struct perf_top *top, char *bf, size_t size)
if (!top->display_weighted) {
ret += SNPRINTF(bf + ret, size - ret, "%s",
event_name(top->sym_evsel));
- } else list_for_each_entry(counter, &top->evlist->entries, node) {
- ret += SNPRINTF(bf + ret, size - ret, "%s%s",
- counter->idx ? "/" : "", event_name(counter));
+ } else {
+ /*
+ * Don't let events eat all the space. Leaving 30 bytes
+ * for the rest should be enough.
+ */
+ size_t last_pos = size - 30;
+
+ list_for_each_entry(counter, &top->evlist->entries, node) {
+ ret += SNPRINTF(bf + ret, size - ret, "%s%s",
+ counter->idx ? "/" : "",
+ event_name(counter));
+ if (ret > last_pos) {
+ sprintf(bf + last_pos - 3, "..");
+ ret = last_pos - 1;
+ break;
+ }
+ }
}
ret += SNPRINTF(bf + ret, size - ret, "], ");