* [PATCH 1/3] perf, evlist: Fix access of freed id arrays
@ 2019-09-23 23:33 Andi Kleen
2019-09-23 23:33 ` [PATCH 2/3] perf, expr: Remove assert usage Andi Kleen
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Andi Kleen @ 2019-09-23 23:33 UTC (permalink / raw)
To: acme; +Cc: jolsa, linux-kernel, Andi Kleen
From: Andi Kleen <ak@linux.intel.com>
I'm not fully sure if this is the correct fix, but without this
I get crashes on more complex perf stat metric usages. The problem
is that part of the state gets freed when a weak group fails,
but then is later still used. Just don't free the ids, we're
going to reuse them anyways on the weak group retry.
For example:
% perf stat -M IpB,IpCall,IpTB,IPC,Retiring_SMT,Frontend_Bound_SMT,Kernel_Utilization,CPU_Utilization --metric-only -a -I 1000 sleep 2
crashes and gives in valgrind:
=21527== Invalid write of size 8
==21527== at 0x4EE582: hlist_add_head (list.h:644)
==21527== by 0x4EFD3C: perf_evlist__id_hash (evlist.c:477)
==21527== by 0x4EFD99: perf_evlist__id_add (evlist.c:483)
==21527== by 0x4EFF15: perf_evlist__id_add_fd (evlist.c:524)
==21527== by 0x4FC693: store_evsel_ids (evsel.c:2969)
==21527== by 0x4FC76C: perf_evsel__store_ids (evsel.c:2986)
==21527== by 0x450DA7: __run_perf_stat (builtin-stat.c:519)
==21527== by 0x451285: run_perf_stat (builtin-stat.c:636)
==21527== by 0x454619: cmd_stat (builtin-stat.c:1966)
==21527== by 0x4D557D: run_builtin (perf.c:310)
==21527== by 0x4D57EA: handle_internal_command (perf.c:362)
==21527== by 0x4D5931: run_argv (perf.c:406)
==21527== Address 0x12e3f008 is 104 bytes inside a block of size 2,056 free'd
==21527== at 0x4839A0C: free (vg_replace_malloc.c:540)
==21527== by 0x627139: xyarray__delete (xyarray.c:32)
==21527== by 0x4F6BE4: perf_evsel__free_id (evsel.c:1253)
==21527== by 0x4FA11F: evsel__close (evsel.c:1994)
==21527== by 0x4F30A3: perf_evlist__reset_weak_group (evlist.c:1783)
==21527== by 0x450B47: __run_perf_stat (builtin-stat.c:466)
==21527== by 0x451285: run_perf_stat (builtin-stat.c:636)
==21527== by 0x454619: cmd_stat (builtin-stat.c:1966)
==21527== by 0x4D557D: run_builtin (perf.c:310)
==21527== by 0x4D57EA: handle_internal_command (perf.c:362)
==21527== by 0x4D5931: run_argv (perf.c:406)
==21527== by 0x4D5CAE: main (perf.c:531)
==21527== Block was alloc'd at
==21527== at 0x483AB1A: calloc (vg_replace_malloc.c:762)
==21527== by 0x627024: zalloc (zalloc.c:8)
==21527== by 0x627088: xyarray__new (xyarray.c:10)
==21527== by 0x4F6B20: perf_evsel__alloc_id (evsel.c:1237)
==21527== by 0x4FC74E: perf_evsel__store_ids (evsel.c:2983)
==21527== by 0x450DA7: __run_perf_stat (builtin-stat.c:519)
==21527== by 0x451285: run_perf_stat (builtin-stat.c:636)
==21527== by 0x454619: cmd_stat (builtin-stat.c:1966)
==21527== by 0x4D557D: run_builtin (perf.c:310)
==21527== by 0x4D57EA: handle_internal_command (perf.c:362)
==21527== by 0x4D5931: run_argv (perf.c:406)
==21527== by 0x4D5CAE: main (perf.c:531)
Signed-off-by: Andi Kleen <ak@linux.intel.com>
---
tools/perf/util/evlist.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
index 095924aa186b..765303553041 100644
--- a/tools/perf/util/evlist.c
+++ b/tools/perf/util/evlist.c
@@ -1780,7 +1780,7 @@ struct evsel *perf_evlist__reset_weak_group(struct evlist *evsel_list,
is_open = false;
if (c2->leader == leader) {
if (is_open)
- evsel__close(c2);
+ perf_evsel__close(&evsel->core);
c2->leader = c2;
c2->core.nr_members = 0;
}
--
2.21.0
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 2/3] perf, expr: Remove assert usage
2019-09-23 23:33 [PATCH 1/3] perf, evlist: Fix access of freed id arrays Andi Kleen
@ 2019-09-23 23:33 ` Andi Kleen
2019-09-24 7:44 ` Jiri Olsa
2019-09-23 23:33 ` [PATCH 3/3] perf, stat: Fix free memory access / memory leaks in metrics Andi Kleen
2019-09-24 7:44 ` [PATCH 1/3] perf, evlist: Fix access of freed id arrays Jiri Olsa
2 siblings, 1 reply; 10+ messages in thread
From: Andi Kleen @ 2019-09-23 23:33 UTC (permalink / raw)
To: acme; +Cc: jolsa, linux-kernel, Andi Kleen
From: Andi Kleen <ak@linux.intel.com>
My "compile perf statically" setup doesn't like this assert
for unknown reasons. Replace it with a standard BUG_ON
Signed-off-by: Andi Kleen <ak@linux.intel.com>
---
tools/perf/util/expr.y | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/tools/perf/util/expr.y b/tools/perf/util/expr.y
index f9a20a39b64a..5086a941295a 100644
--- a/tools/perf/util/expr.y
+++ b/tools/perf/util/expr.y
@@ -6,7 +6,6 @@
#define IN_EXPR_Y 1
#include "expr.h"
#include "smt.h"
-#include <assert.h>
#include <string.h>
#define MAXIDLEN 256
@@ -172,7 +171,8 @@ static int expr__lex(YYSTYPE *res, const char **pp)
void expr__add_id(struct parse_ctx *ctx, const char *name, double val)
{
int idx;
- assert(ctx->num_ids < MAX_PARSE_ID);
+
+ BUG_ON(ctx->num_ids >= MAX_PARSE_ID);
idx = ctx->num_ids++;
ctx->ids[idx].name = name;
ctx->ids[idx].val = val;
--
2.21.0
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 3/3] perf, stat: Fix free memory access / memory leaks in metrics
2019-09-23 23:33 [PATCH 1/3] perf, evlist: Fix access of freed id arrays Andi Kleen
2019-09-23 23:33 ` [PATCH 2/3] perf, expr: Remove assert usage Andi Kleen
@ 2019-09-23 23:33 ` Andi Kleen
2019-09-24 7:50 ` Jiri Olsa
2019-09-24 7:44 ` [PATCH 1/3] perf, evlist: Fix access of freed id arrays Jiri Olsa
2 siblings, 1 reply; 10+ messages in thread
From: Andi Kleen @ 2019-09-23 23:33 UTC (permalink / raw)
To: acme; +Cc: jolsa, linux-kernel, Andi Kleen
From: Andi Kleen <ak@linux.intel.com>
Make sure to not free the name passed in by the caller, but free
all the allocated ids when parsing expressions.
The loop at the end knows that the first entry shouldn't be freed,
so make sure the caller name is the first entry.
Fixes
% perf stat -M IpB,IpCall,IpTB,IPC,Retiring_SMT,Frontend_Bound_SMT,Kernel_Utilization,CPU_Utilization --metric-only -a -I 1000 sleep 2
valgrind:
1.009943231 ==21527== Invalid read of size 1
==21527== at 0x483CB74: strcmp (vg_replace_strmem.c:849)
==21527== by 0x582CF8: collect_all_aliases (stat-display.c:554)
==21527== by 0x582EB3: collect_data (stat-display.c:577)
==21527== by 0x583A32: print_counter_aggr (stat-display.c:806)
==21527== by 0x584FAD: perf_evlist__print_counters (stat-display.c:1200)
==21527== by 0x45133A: print_counters (builtin-stat.c:655)
==21527== by 0x450629: process_interval (builtin-stat.c:353)
==21527== by 0x450FBD: __run_perf_stat (builtin-stat.c:564)
==21527== by 0x451285: run_perf_stat (builtin-stat.c:636)
==21527== by 0x454619: cmd_stat (builtin-stat.c:1966)
==21527== by 0x4D557D: run_builtin (perf.c:310)
==21527== by 0x4D57EA: handle_internal_command (perf.c:362)
==21527== Address 0x12826cd0 is 0 bytes inside a block of size 25 free'd
==21527== at 0x4839A0C: free (vg_replace_malloc.c:540)
==21527== by 0x627041: __zfree (zalloc.c:13)
==21527== by 0x57F66A: generic_metric (stat-shadow.c:814)
==21527== by 0x580B21: perf_stat__print_shadow_stats (stat-shadow.c:1057)
==21527== by 0x58418E: print_metric_headers (stat-display.c:943)
==21527== by 0x5844BC: print_interval (stat-display.c:1004)
==21527== by 0x584DEB: perf_evlist__print_counters (stat-display.c:1172)
==21527== by 0x45133A: print_counters (builtin-stat.c:655)
==21527== by 0x450629: process_interval (builtin-stat.c:353)
==21527== by 0x450FBD: __run_perf_stat (builtin-stat.c:564)
==21527== by 0x451285: run_perf_stat (builtin-stat.c:636)
==21527== by 0x454619: cmd_stat (builtin-stat.c:1966)
==21527== Block was alloc'd at
==21527== at 0x483880B: malloc (vg_replace_malloc.c:309)
==21527== by 0x51677DE: strdup (in /usr/lib64/libc-2.29.so)
==21527== by 0x506457: parse_events_name (parse-events.c:1754)
==21527== by 0x5550BB: parse_events_parse (parse-events.y:214)
==21527== by 0x50694D: parse_events__scanner (parse-events.c:1887)
==21527== by 0x506AEF: parse_events (parse-events.c:1927)
==21527== by 0x521D8B: metricgroup__parse_groups (metricgroup.c:527)
==21527== by 0x45156F: parse_metric_groups (builtin-stat.c:721)
==21527== by 0x6228A9: get_value (parse-options.c:243)
==21527== by 0x62363F: parse_short_opt (parse-options.c:348)
==21527== by 0x62363F: parse_options_step (parse-options.c:536)
==21527== by 0x62363F: parse_options_subcommand (parse-options.c:651)
==21527== by 0x453C1D: cmd_stat (builtin-stat.c:1718)
==21527== by 0x4D557D: run_builtin (perf.c:310)
and also a leak report.
Signed-off-by: Andi Kleen <ak@linux.intel.com>
---
tools/perf/util/stat-shadow.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/tools/perf/util/stat-shadow.c b/tools/perf/util/stat-shadow.c
index 70c87fdb2a43..2c41d47f6f83 100644
--- a/tools/perf/util/stat-shadow.c
+++ b/tools/perf/util/stat-shadow.c
@@ -738,6 +738,8 @@ static void generic_metric(struct perf_stat_config *config,
char *n, *pn;
expr__ctx_init(&pctx);
+ /* Must be first id entry */
+ expr__add_id(&pctx, name, avg);
for (i = 0; metric_events[i]; i++) {
struct saved_value *v;
struct stats *stats;
@@ -776,8 +778,6 @@ static void generic_metric(struct perf_stat_config *config,
expr__add_id(&pctx, n, avg_stats(stats)*scale);
}
- expr__add_id(&pctx, name, avg);
-
if (!metric_events[i]) {
const char *p = metric_expr;
--
2.21.0
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 1/3] perf, evlist: Fix access of freed id arrays
2019-09-23 23:33 [PATCH 1/3] perf, evlist: Fix access of freed id arrays Andi Kleen
2019-09-23 23:33 ` [PATCH 2/3] perf, expr: Remove assert usage Andi Kleen
2019-09-23 23:33 ` [PATCH 3/3] perf, stat: Fix free memory access / memory leaks in metrics Andi Kleen
@ 2019-09-24 7:44 ` Jiri Olsa
2019-09-24 16:05 ` Andi Kleen
2 siblings, 1 reply; 10+ messages in thread
From: Jiri Olsa @ 2019-09-24 7:44 UTC (permalink / raw)
To: Andi Kleen; +Cc: acme, jolsa, linux-kernel, Andi Kleen
On Mon, Sep 23, 2019 at 04:33:37PM -0700, Andi Kleen wrote:
> From: Andi Kleen <ak@linux.intel.com>
>
> I'm not fully sure if this is the correct fix, but without this
> I get crashes on more complex perf stat metric usages. The problem
> is that part of the state gets freed when a weak group fails,
> but then is later still used. Just don't free the ids, we're
> going to reuse them anyways on the weak group retry.
>
> For example:
>
> % perf stat -M IpB,IpCall,IpTB,IPC,Retiring_SMT,Frontend_Bound_SMT,Kernel_Utilization,CPU_Utilization --metric-only -a -I 1000 sleep 2
>
> crashes and gives in valgrind:
>
> =21527== Invalid write of size 8
> ==21527== at 0x4EE582: hlist_add_head (list.h:644)
> ==21527== by 0x4EFD3C: perf_evlist__id_hash (evlist.c:477)
> ==21527== by 0x4EFD99: perf_evlist__id_add (evlist.c:483)
> ==21527== by 0x4EFF15: perf_evlist__id_add_fd (evlist.c:524)
> ==21527== by 0x4FC693: store_evsel_ids (evsel.c:2969)
> ==21527== by 0x4FC76C: perf_evsel__store_ids (evsel.c:2986)
> ==21527== by 0x450DA7: __run_perf_stat (builtin-stat.c:519)
> ==21527== by 0x451285: run_perf_stat (builtin-stat.c:636)
> ==21527== by 0x454619: cmd_stat (builtin-stat.c:1966)
> ==21527== by 0x4D557D: run_builtin (perf.c:310)
> ==21527== by 0x4D57EA: handle_internal_command (perf.c:362)
> ==21527== by 0x4D5931: run_argv (perf.c:406)
> ==21527== Address 0x12e3f008 is 104 bytes inside a block of size 2,056 free'd
> ==21527== at 0x4839A0C: free (vg_replace_malloc.c:540)
> ==21527== by 0x627139: xyarray__delete (xyarray.c:32)
> ==21527== by 0x4F6BE4: perf_evsel__free_id (evsel.c:1253)
> ==21527== by 0x4FA11F: evsel__close (evsel.c:1994)
> ==21527== by 0x4F30A3: perf_evlist__reset_weak_group (evlist.c:1783)
> ==21527== by 0x450B47: __run_perf_stat (builtin-stat.c:466)
> ==21527== by 0x451285: run_perf_stat (builtin-stat.c:636)
> ==21527== by 0x454619: cmd_stat (builtin-stat.c:1966)
> ==21527== by 0x4D557D: run_builtin (perf.c:310)
> ==21527== by 0x4D57EA: handle_internal_command (perf.c:362)
> ==21527== by 0x4D5931: run_argv (perf.c:406)
> ==21527== by 0x4D5CAE: main (perf.c:531)
> ==21527== Block was alloc'd at
> ==21527== at 0x483AB1A: calloc (vg_replace_malloc.c:762)
> ==21527== by 0x627024: zalloc (zalloc.c:8)
> ==21527== by 0x627088: xyarray__new (xyarray.c:10)
> ==21527== by 0x4F6B20: perf_evsel__alloc_id (evsel.c:1237)
> ==21527== by 0x4FC74E: perf_evsel__store_ids (evsel.c:2983)
> ==21527== by 0x450DA7: __run_perf_stat (builtin-stat.c:519)
> ==21527== by 0x451285: run_perf_stat (builtin-stat.c:636)
> ==21527== by 0x454619: cmd_stat (builtin-stat.c:1966)
> ==21527== by 0x4D557D: run_builtin (perf.c:310)
> ==21527== by 0x4D57EA: handle_internal_command (perf.c:362)
> ==21527== by 0x4D5931: run_argv (perf.c:406)
> ==21527== by 0x4D5CAE: main (perf.c:531)
>
> Signed-off-by: Andi Kleen <ak@linux.intel.com>
> ---
> tools/perf/util/evlist.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
> index 095924aa186b..765303553041 100644
> --- a/tools/perf/util/evlist.c
> +++ b/tools/perf/util/evlist.c
> @@ -1780,7 +1780,7 @@ struct evsel *perf_evlist__reset_weak_group(struct evlist *evsel_list,
> is_open = false;
> if (c2->leader == leader) {
> if (is_open)
> - evsel__close(c2);
> + perf_evsel__close(&evsel->core);
id/sample_id arrays are not created when evsel is open but
we free it at close
for now this fix seems correct to me.. we are moving id/sample_id
arrays under libperf, I'll make a note to check on close and reopen
of evsel and add some tests for that
Acked-by: Jiri Olsa <jolsa@redhat.com>
thanks,
jirka
> c2->leader = c2;
> c2->core.nr_members = 0;
> }
> --
> 2.21.0
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/3] perf, expr: Remove assert usage
2019-09-23 23:33 ` [PATCH 2/3] perf, expr: Remove assert usage Andi Kleen
@ 2019-09-24 7:44 ` Jiri Olsa
0 siblings, 0 replies; 10+ messages in thread
From: Jiri Olsa @ 2019-09-24 7:44 UTC (permalink / raw)
To: Andi Kleen; +Cc: acme, jolsa, linux-kernel, Andi Kleen
On Mon, Sep 23, 2019 at 04:33:38PM -0700, Andi Kleen wrote:
> From: Andi Kleen <ak@linux.intel.com>
>
> My "compile perf statically" setup doesn't like this assert
> for unknown reasons. Replace it with a standard BUG_ON
>
> Signed-off-by: Andi Kleen <ak@linux.intel.com>
> ---
> tools/perf/util/expr.y | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/tools/perf/util/expr.y b/tools/perf/util/expr.y
> index f9a20a39b64a..5086a941295a 100644
> --- a/tools/perf/util/expr.y
> +++ b/tools/perf/util/expr.y
> @@ -6,7 +6,6 @@
> #define IN_EXPR_Y 1
> #include "expr.h"
> #include "smt.h"
> -#include <assert.h>
> #include <string.h>
>
> #define MAXIDLEN 256
> @@ -172,7 +171,8 @@ static int expr__lex(YYSTYPE *res, const char **pp)
> void expr__add_id(struct parse_ctx *ctx, const char *name, double val)
> {
> int idx;
> - assert(ctx->num_ids < MAX_PARSE_ID);
> +
> + BUG_ON(ctx->num_ids >= MAX_PARSE_ID);
hi,
getting compilation fail
LINK perf
/usr/bin/ld: perf-in.o: in function `expr__add_id':
/home/jolsa/kernel/linux-perf/tools/perf/util/expr.y:175: undefined reference to `BUG_ON'
collect2: error: ld returned 1 exit status
make[2]: *** [Makefile.perf:609: perf] Error 1
make[1]: *** [Makefile.perf:221: sub-make] Error 2
make: *** [Makefile:70: all] Error 2
jirka
> idx = ctx->num_ids++;
> ctx->ids[idx].name = name;
> ctx->ids[idx].val = val;
> --
> 2.21.0
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 3/3] perf, stat: Fix free memory access / memory leaks in metrics
2019-09-23 23:33 ` [PATCH 3/3] perf, stat: Fix free memory access / memory leaks in metrics Andi Kleen
@ 2019-09-24 7:50 ` Jiri Olsa
2019-09-24 14:08 ` Andi Kleen
0 siblings, 1 reply; 10+ messages in thread
From: Jiri Olsa @ 2019-09-24 7:50 UTC (permalink / raw)
To: Andi Kleen; +Cc: acme, jolsa, linux-kernel, Andi Kleen
On Mon, Sep 23, 2019 at 04:33:39PM -0700, Andi Kleen wrote:
> From: Andi Kleen <ak@linux.intel.com>
>
> Make sure to not free the name passed in by the caller, but free
> all the allocated ids when parsing expressions.
>
> The loop at the end knows that the first entry shouldn't be freed,
> so make sure the caller name is the first entry.
>
> Fixes
>
> % perf stat -M IpB,IpCall,IpTB,IPC,Retiring_SMT,Frontend_Bound_SMT,Kernel_Utilization,CPU_Utilization --metric-only -a -I 1000 sleep 2
>
> valgrind:
> 1.009943231 ==21527== Invalid read of size 1
> ==21527== at 0x483CB74: strcmp (vg_replace_strmem.c:849)
> ==21527== by 0x582CF8: collect_all_aliases (stat-display.c:554)
> ==21527== by 0x582EB3: collect_data (stat-display.c:577)
> ==21527== by 0x583A32: print_counter_aggr (stat-display.c:806)
> ==21527== by 0x584FAD: perf_evlist__print_counters (stat-display.c:1200)
> ==21527== by 0x45133A: print_counters (builtin-stat.c:655)
> ==21527== by 0x450629: process_interval (builtin-stat.c:353)
> ==21527== by 0x450FBD: __run_perf_stat (builtin-stat.c:564)
> ==21527== by 0x451285: run_perf_stat (builtin-stat.c:636)
> ==21527== by 0x454619: cmd_stat (builtin-stat.c:1966)
> ==21527== by 0x4D557D: run_builtin (perf.c:310)
> ==21527== by 0x4D57EA: handle_internal_command (perf.c:362)
> ==21527== Address 0x12826cd0 is 0 bytes inside a block of size 25 free'd
> ==21527== at 0x4839A0C: free (vg_replace_malloc.c:540)
> ==21527== by 0x627041: __zfree (zalloc.c:13)
> ==21527== by 0x57F66A: generic_metric (stat-shadow.c:814)
> ==21527== by 0x580B21: perf_stat__print_shadow_stats (stat-shadow.c:1057)
> ==21527== by 0x58418E: print_metric_headers (stat-display.c:943)
> ==21527== by 0x5844BC: print_interval (stat-display.c:1004)
> ==21527== by 0x584DEB: perf_evlist__print_counters (stat-display.c:1172)
> ==21527== by 0x45133A: print_counters (builtin-stat.c:655)
> ==21527== by 0x450629: process_interval (builtin-stat.c:353)
> ==21527== by 0x450FBD: __run_perf_stat (builtin-stat.c:564)
> ==21527== by 0x451285: run_perf_stat (builtin-stat.c:636)
> ==21527== by 0x454619: cmd_stat (builtin-stat.c:1966)
> ==21527== Block was alloc'd at
> ==21527== at 0x483880B: malloc (vg_replace_malloc.c:309)
> ==21527== by 0x51677DE: strdup (in /usr/lib64/libc-2.29.so)
> ==21527== by 0x506457: parse_events_name (parse-events.c:1754)
> ==21527== by 0x5550BB: parse_events_parse (parse-events.y:214)
> ==21527== by 0x50694D: parse_events__scanner (parse-events.c:1887)
> ==21527== by 0x506AEF: parse_events (parse-events.c:1927)
> ==21527== by 0x521D8B: metricgroup__parse_groups (metricgroup.c:527)
> ==21527== by 0x45156F: parse_metric_groups (builtin-stat.c:721)
> ==21527== by 0x6228A9: get_value (parse-options.c:243)
> ==21527== by 0x62363F: parse_short_opt (parse-options.c:348)
> ==21527== by 0x62363F: parse_options_step (parse-options.c:536)
> ==21527== by 0x62363F: parse_options_subcommand (parse-options.c:651)
> ==21527== by 0x453C1D: cmd_stat (builtin-stat.c:1718)
> ==21527== by 0x4D557D: run_builtin (perf.c:310)
>
> and also a leak report.
>
> Signed-off-by: Andi Kleen <ak@linux.intel.com>
> ---
> tools/perf/util/stat-shadow.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/tools/perf/util/stat-shadow.c b/tools/perf/util/stat-shadow.c
> index 70c87fdb2a43..2c41d47f6f83 100644
> --- a/tools/perf/util/stat-shadow.c
> +++ b/tools/perf/util/stat-shadow.c
> @@ -738,6 +738,8 @@ static void generic_metric(struct perf_stat_config *config,
> char *n, *pn;
>
> expr__ctx_init(&pctx);
> + /* Must be first id entry */
> + expr__add_id(&pctx, name, avg);
hum, shouldn't u instead use strdup(name) instead of name?
jirka
> for (i = 0; metric_events[i]; i++) {
> struct saved_value *v;
> struct stats *stats;
> @@ -776,8 +778,6 @@ static void generic_metric(struct perf_stat_config *config,
> expr__add_id(&pctx, n, avg_stats(stats)*scale);
> }
>
> - expr__add_id(&pctx, name, avg);
> -
> if (!metric_events[i]) {
> const char *p = metric_expr;
>
> --
> 2.21.0
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 3/3] perf, stat: Fix free memory access / memory leaks in metrics
2019-09-24 7:50 ` Jiri Olsa
@ 2019-09-24 14:08 ` Andi Kleen
2019-09-24 14:44 ` Jiri Olsa
0 siblings, 1 reply; 10+ messages in thread
From: Andi Kleen @ 2019-09-24 14:08 UTC (permalink / raw)
To: Jiri Olsa; +Cc: Andi Kleen, acme, jolsa, linux-kernel
> > expr__ctx_init(&pctx);
> > + /* Must be first id entry */
> > + expr__add_id(&pctx, name, avg);
>
> hum, shouldn't u instead use strdup(name) instead of name?
The cleanup loop later skips freeing the first entry.
-Andi
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 3/3] perf, stat: Fix free memory access / memory leaks in metrics
2019-09-24 14:08 ` Andi Kleen
@ 2019-09-24 14:44 ` Jiri Olsa
2019-09-24 19:20 ` Arnaldo Carvalho de Melo
0 siblings, 1 reply; 10+ messages in thread
From: Jiri Olsa @ 2019-09-24 14:44 UTC (permalink / raw)
To: Andi Kleen; +Cc: Andi Kleen, acme, jolsa, linux-kernel
On Tue, Sep 24, 2019 at 07:08:56AM -0700, Andi Kleen wrote:
> > > expr__ctx_init(&pctx);
> > > + /* Must be first id entry */
> > > + expr__add_id(&pctx, name, avg);
> >
> > hum, shouldn't u instead use strdup(name) instead of name?
>
> The cleanup loop later skips freeing the first entry.
aaah, nice ;-)
Acked-by: Jiri Olsa <jolsa@kernel.org>
thanks,
jirka
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/3] perf, evlist: Fix access of freed id arrays
2019-09-24 7:44 ` [PATCH 1/3] perf, evlist: Fix access of freed id arrays Jiri Olsa
@ 2019-09-24 16:05 ` Andi Kleen
0 siblings, 0 replies; 10+ messages in thread
From: Andi Kleen @ 2019-09-24 16:05 UTC (permalink / raw)
To: Jiri Olsa; +Cc: Andi Kleen, acme, jolsa, linux-kernel
> id/sample_id arrays are not created when evsel is open but
> we free it at close
>
> for now this fix seems correct to me.. we are moving id/sample_id
> arrays under libperf, I'll make a note to check on close and reopen
> of evsel and add some tests for that
>
> Acked-by: Jiri Olsa <jolsa@redhat.com>
It looks like there are still some bogus closes in such a case, at least valgrind
complains about some close(-1). But these should be harmless, so I guess
we can leave them for now.
-Andi
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 3/3] perf, stat: Fix free memory access / memory leaks in metrics
2019-09-24 14:44 ` Jiri Olsa
@ 2019-09-24 19:20 ` Arnaldo Carvalho de Melo
0 siblings, 0 replies; 10+ messages in thread
From: Arnaldo Carvalho de Melo @ 2019-09-24 19:20 UTC (permalink / raw)
To: Jiri Olsa; +Cc: Andi Kleen, Andi Kleen, jolsa, linux-kernel
Em Tue, Sep 24, 2019 at 04:44:18PM +0200, Jiri Olsa escreveu:
> On Tue, Sep 24, 2019 at 07:08:56AM -0700, Andi Kleen wrote:
> > > > expr__ctx_init(&pctx);
> > > > + /* Must be first id entry */
> > > > + expr__add_id(&pctx, name, avg);
> > >
> > > hum, shouldn't u instead use strdup(name) instead of name?
> >
> > The cleanup loop later skips freeing the first entry.
>
> aaah, nice ;-)
>
> Acked-by: Jiri Olsa <jolsa@kernel.org>
Thanks, reproduced and applied, before the patch:
# perf stat -M IpB,IpCall,IpTB,IPC,Retiring_SMT,Frontend_Bound_SMT,Kernel_Utilization,CPU_Utilization --metric-only -a -I 1000 sleep 2
# time CPU_Utilization
1.000470810 free(): double free detected in tcache 2
Aborted (core dumped)
#
- Arnaldo
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2019-09-24 19:21 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-23 23:33 [PATCH 1/3] perf, evlist: Fix access of freed id arrays Andi Kleen
2019-09-23 23:33 ` [PATCH 2/3] perf, expr: Remove assert usage Andi Kleen
2019-09-24 7:44 ` Jiri Olsa
2019-09-23 23:33 ` [PATCH 3/3] perf, stat: Fix free memory access / memory leaks in metrics Andi Kleen
2019-09-24 7:50 ` Jiri Olsa
2019-09-24 14:08 ` Andi Kleen
2019-09-24 14:44 ` Jiri Olsa
2019-09-24 19:20 ` Arnaldo Carvalho de Melo
2019-09-24 7:44 ` [PATCH 1/3] perf, evlist: Fix access of freed id arrays Jiri Olsa
2019-09-24 16:05 ` Andi Kleen
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).