From 07eafd4e053a41d72611848b8758df0752b53ee4 Mon Sep 17 00:00:00 2001 From: Ian Rogers Date: Sun, 7 Nov 2021 01:00:01 -0800 Subject: [PATCH] perf parse-event: Add init and exit to parse_event_error parse_events() may succeed but leave string memory allocations reachable in the error. Add an init/exit that must be called to initialize and clean up the error. This fixes a leak in metricgroup parse_ids. Signed-off-by: Ian Rogers Cc: Alexander Shishkin Cc: Jiri Olsa Cc: John Garry Cc: Mark Rutland Cc: Namhyung Kim Cc: Peter Zijlstra Cc: Stephane Eranian Link: http://lore.kernel.org/lkml/20211107090002.3784612-2-irogers@google.com Signed-off-by: Arnaldo Carvalho de Melo --- tools/perf/arch/powerpc/util/kvm-stat.c | 3 ++- tools/perf/bench/evlist-open-close.c | 6 ++++-- tools/perf/builtin-stat.c | 38 +++++++++++++++++++-------------- tools/perf/builtin-trace.c | 17 ++++++--------- tools/perf/tests/backward-ring-buffer.c | 3 ++- tools/perf/tests/bpf.c | 3 ++- tools/perf/tests/expand-cgroup.c | 2 ++ tools/perf/tests/parse-events.c | 4 ++-- tools/perf/tests/pmu-events.c | 22 +++++++++---------- tools/perf/tests/topology.c | 2 ++ tools/perf/util/metricgroup.c | 3 ++- tools/perf/util/parse-events.c | 20 ++++++++++++----- tools/perf/util/parse-events.h | 2 ++ 13 files changed, 74 insertions(+), 51 deletions(-) diff --git a/tools/perf/arch/powerpc/util/kvm-stat.c b/tools/perf/arch/powerpc/util/kvm-stat.c index dc644ca..1a9b40ea 100644 --- a/tools/perf/arch/powerpc/util/kvm-stat.c +++ b/tools/perf/arch/powerpc/util/kvm-stat.c @@ -113,10 +113,11 @@ static int is_tracepoint_available(const char *str, struct evlist *evlist) struct parse_events_error err; int ret; - bzero(&err, sizeof(err)); + parse_events_error__init(&err); ret = parse_events(evlist, str, &err); if (err.str) parse_events_error__print(&err, "tracepoint"); + parse_events_error__exit(&err); return ret; } diff --git a/tools/perf/bench/evlist-open-close.c b/tools/perf/bench/evlist-open-close.c index 3f951893..482738e 100644 --- a/tools/perf/bench/evlist-open-close.c +++ b/tools/perf/bench/evlist-open-close.c @@ -78,7 +78,7 @@ static int evlist__count_evsel_fds(struct evlist *evlist) static struct evlist *bench__create_evlist(char *evstr) { - struct parse_events_error err = { .idx = 0, }; + struct parse_events_error err; struct evlist *evlist = evlist__new(); int ret; @@ -87,14 +87,16 @@ static struct evlist *bench__create_evlist(char *evstr) return NULL; } + parse_events_error__init(&err); ret = parse_events(evlist, evstr, &err); if (ret) { parse_events_error__print(&err, evstr); + parse_events_error__exit(&err); pr_err("Run 'perf list' for a list of valid events\n"); ret = 1; goto out_delete_evlist; } - + parse_events_error__exit(&err); ret = evlist__create_maps(evlist, &opts.target); if (ret < 0) { pr_err("Not enough memory to create thread/cpu maps\n"); diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c index af447a1..7974933 100644 --- a/tools/perf/builtin-stat.c +++ b/tools/perf/builtin-stat.c @@ -1750,14 +1750,12 @@ static int add_default_attributes(void) (PERF_COUNT_HW_CACHE_OP_PREFETCH << 8) | (PERF_COUNT_HW_CACHE_RESULT_MISS << 16) }, }; - struct parse_events_error errinfo; - /* Set attrs if no event is selected and !null_run: */ if (stat_config.null_run) return 0; - bzero(&errinfo, sizeof(errinfo)); if (transaction_run) { + struct parse_events_error errinfo; /* Handle -T as -M transaction. Once platform specific metrics * support has been added to the json files, all architectures * will use this approach. To determine transaction support @@ -1772,6 +1770,7 @@ static int add_default_attributes(void) &stat_config.metric_events); } + parse_events_error__init(&errinfo); if (pmu_have_event("cpu", "cycles-ct") && pmu_have_event("cpu", "el-start")) err = parse_events(evsel_list, transaction_attrs, @@ -1783,12 +1782,13 @@ static int add_default_attributes(void) if (err) { fprintf(stderr, "Cannot set up transaction events\n"); parse_events_error__print(&errinfo, transaction_attrs); - return -1; } - return 0; + parse_events_error__exit(&errinfo); + return err ? -1 : 0; } if (smi_cost) { + struct parse_events_error errinfo; int smi; if (sysfs__read_int(FREEZE_ON_SMI_PATH, &smi) < 0) { @@ -1804,23 +1804,23 @@ static int add_default_attributes(void) smi_reset = true; } - if (pmu_have_event("msr", "aperf") && - pmu_have_event("msr", "smi")) { - if (!force_metric_only) - stat_config.metric_only = true; - err = parse_events(evsel_list, smi_cost_attrs, &errinfo); - } else { + if (!pmu_have_event("msr", "aperf") || + !pmu_have_event("msr", "smi")) { fprintf(stderr, "To measure SMI cost, it needs " "msr/aperf/, msr/smi/ and cpu/cycles/ support\n"); - parse_events_error__print(&errinfo, smi_cost_attrs); return -1; } + if (!force_metric_only) + stat_config.metric_only = true; + + parse_events_error__init(&errinfo); + err = parse_events(evsel_list, smi_cost_attrs, &errinfo); if (err) { parse_events_error__print(&errinfo, smi_cost_attrs); fprintf(stderr, "Cannot set up SMI cost events\n"); - return -1; } - return 0; + parse_events_error__exit(&errinfo); + return err ? -1 : 0; } if (topdown_run) { @@ -1875,18 +1875,22 @@ static int add_default_attributes(void) return -1; } if (topdown_attrs[0] && str) { + struct parse_events_error errinfo; if (warn) arch_topdown_group_warn(); setup_metrics: + parse_events_error__init(&errinfo); err = parse_events(evsel_list, str, &errinfo); if (err) { fprintf(stderr, "Cannot set up top down events %s: %d\n", str, err); parse_events_error__print(&errinfo, str); + parse_events_error__exit(&errinfo); free(str); return -1; } + parse_events_error__exit(&errinfo); } else { fprintf(stderr, "System does not support topdown\n"); return -1; @@ -1896,6 +1900,7 @@ setup_metrics: if (!evsel_list->core.nr_entries) { if (perf_pmu__has_hybrid()) { + struct parse_events_error errinfo; const char *hybrid_str = "cycles,instructions,branches,branch-misses"; if (target__has_cpu(&target)) @@ -1906,15 +1911,16 @@ setup_metrics: return -1; } + parse_events_error__init(&errinfo); err = parse_events(evsel_list, hybrid_str, &errinfo); if (err) { fprintf(stderr, "Cannot set up hybrid events %s: %d\n", hybrid_str, err); parse_events_error__print(&errinfo, hybrid_str); - return -1; } - return err; + parse_events_error__exit(&errinfo); + return err ? -1 : 0; } if (target__has_cpu(&target)) diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c index 7f0acc9..624ea12 100644 --- a/tools/perf/builtin-trace.c +++ b/tools/perf/builtin-trace.c @@ -3063,15 +3063,11 @@ static bool evlist__add_vfs_getname(struct evlist *evlist) struct parse_events_error err; int ret; - bzero(&err, sizeof(err)); + parse_events_error__init(&err); ret = parse_events(evlist, "probe:vfs_getname*", &err); - if (ret) { - free(err.str); - free(err.help); - free(err.first_str); - free(err.first_help); + parse_events_error__exit(&err); + if (ret) return false; - } evlist__for_each_entry_safe(evlist, evsel, tmp) { if (!strstarts(evsel__name(evsel), "probe:vfs_getname")) @@ -4925,12 +4921,13 @@ int cmd_trace(int argc, const char **argv) if (trace.perfconfig_events != NULL) { struct parse_events_error parse_err; - bzero(&parse_err, sizeof(parse_err)); + parse_events_error__init(&parse_err); err = parse_events(trace.evlist, trace.perfconfig_events, &parse_err); - if (err) { + if (err) parse_events_error__print(&parse_err, trace.perfconfig_events); + parse_events_error__exit(&parse_err); + if (err) goto out; - } } if ((nr_cgroups || trace.cgroup) && !trace.opts.target.system_wide) { diff --git a/tools/perf/tests/backward-ring-buffer.c b/tools/perf/tests/backward-ring-buffer.c index b4b9a94..7447a44 100644 --- a/tools/perf/tests/backward-ring-buffer.c +++ b/tools/perf/tests/backward-ring-buffer.c @@ -115,12 +115,13 @@ int test__backward_ring_buffer(struct test *test __maybe_unused, int subtest __m goto out_delete_evlist; } - bzero(&parse_error, sizeof(parse_error)); + parse_events_error__init(&parse_error); /* * Set backward bit, ring buffer should be writing from end. Record * it in aux evlist */ err = parse_events(evlist, "syscalls:sys_enter_prctl/overwrite/", &parse_error); + parse_events_error__exit(&parse_error); if (err) { pr_debug("Failed to parse tracepoint event, try use root\n"); ret = TEST_SKIP; diff --git a/tools/perf/tests/bpf.c b/tools/perf/tests/bpf.c index fa03ff0..2bf146e 100644 --- a/tools/perf/tests/bpf.c +++ b/tools/perf/tests/bpf.c @@ -123,12 +123,13 @@ static int do_test(struct bpf_object *obj, int (*func)(void), struct parse_events_state parse_state; struct parse_events_error parse_error; - bzero(&parse_error, sizeof(parse_error)); + parse_events_error__init(&parse_error); bzero(&parse_state, sizeof(parse_state)); parse_state.error = &parse_error; INIT_LIST_HEAD(&parse_state.list); err = parse_events_load_bpf_obj(&parse_state, &parse_state.list, obj, NULL); + parse_events_error__exit(&parse_error); if (err || list_empty(&parse_state.list)) { pr_debug("Failed to add events selected by BPF\n"); return TEST_FAIL; diff --git a/tools/perf/tests/expand-cgroup.c b/tools/perf/tests/expand-cgroup.c index 57b4c5f..80cff8a 100644 --- a/tools/perf/tests/expand-cgroup.c +++ b/tools/perf/tests/expand-cgroup.c @@ -124,6 +124,7 @@ static int expand_group_events(void) evlist = evlist__new(); TEST_ASSERT_VAL("failed to get evlist", evlist); + parse_events_error__init(&err); ret = parse_events(evlist, event_str, &err); if (ret < 0) { pr_debug("failed to parse event '%s', err %d, str '%s'\n", @@ -135,6 +136,7 @@ static int expand_group_events(void) rblist__init(&metric_events); ret = test_expand_events(evlist, &metric_events); out: + parse_events_error__exit(&err); evlist__delete(evlist); return ret; } diff --git a/tools/perf/tests/parse-events.c b/tools/perf/tests/parse-events.c index e200af9..6af9463 100644 --- a/tools/perf/tests/parse-events.c +++ b/tools/perf/tests/parse-events.c @@ -2045,7 +2045,6 @@ static int test_event(struct evlist_test *e) struct evlist *evlist; int ret; - bzero(&err, sizeof(err)); if (e->valid && !e->valid()) { pr_debug("... SKIP"); return 0; @@ -2055,6 +2054,7 @@ static int test_event(struct evlist_test *e) if (evlist == NULL) return -ENOMEM; + parse_events_error__init(&err); ret = parse_events(evlist, e->name, &err); if (ret) { pr_debug("failed to parse event '%s', err %d, str '%s'\n", @@ -2063,7 +2063,7 @@ static int test_event(struct evlist_test *e) } else { ret = e->check(evlist); } - + parse_events_error__exit(&err); evlist__delete(evlist); return ret; diff --git a/tools/perf/tests/pmu-events.c b/tools/perf/tests/pmu-events.c index 50b1299..9ae894c 100644 --- a/tools/perf/tests/pmu-events.c +++ b/tools/perf/tests/pmu-events.c @@ -787,9 +787,11 @@ static int check_parse_id(const char *id, struct parse_events_error *error, static int check_parse_cpu(const char *id, bool same_cpu, const struct pmu_event *pe) { - struct parse_events_error error = { .idx = 0, }; + struct parse_events_error error; + int ret; - int ret = check_parse_id(id, &error, NULL); + parse_events_error__init(&error); + ret = check_parse_id(id, &error, NULL); if (ret && same_cpu) { pr_warning("Parse event failed metric '%s' id '%s' expr '%s'\n", pe->metric_name, id, pe->metric_expr); @@ -800,22 +802,18 @@ static int check_parse_cpu(const char *id, bool same_cpu, const struct pmu_event id, pe->metric_name, pe->metric_expr); ret = 0; } - free(error.str); - free(error.help); - free(error.first_str); - free(error.first_help); + parse_events_error__exit(&error); return ret; } static int check_parse_fake(const char *id) { - struct parse_events_error error = { .idx = 0, }; - int ret = check_parse_id(id, &error, &perf_pmu__fake); + struct parse_events_error error; + int ret; - free(error.str); - free(error.help); - free(error.first_str); - free(error.first_help); + parse_events_error__init(&error); + ret = check_parse_id(id, &error, &perf_pmu__fake); + parse_events_error__exit(&error); return ret; } diff --git a/tools/perf/tests/topology.c b/tools/perf/tests/topology.c index b9028e3..4574c46 100644 --- a/tools/perf/tests/topology.c +++ b/tools/perf/tests/topology.c @@ -49,7 +49,9 @@ static int session_write_header(char *path) session->evlist = evlist__new(); TEST_ASSERT_VAL("can't get evlist", session->evlist); + parse_events_error__init(&err); parse_events(session->evlist, "cpu_core/cycles/", &err); + parse_events_error__exit(&err); } perf_header__set_feat(&session->header, HEADER_CPU_TOPOLOGY); diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c index edd7180..1b43cbc 100644 --- a/tools/perf/util/metricgroup.c +++ b/tools/perf/util/metricgroup.c @@ -1339,7 +1339,7 @@ static int parse_ids(struct perf_pmu *fake_pmu, struct expr_parse_ctx *ids, goto err_out; } pr_debug("Parsing metric events '%s'\n", events.buf); - bzero(&parse_error, sizeof(parse_error)); + parse_events_error__init(&parse_error); ret = __parse_events(parsed_evlist, events.buf, &parse_error, fake_pmu); if (ret) { parse_events_error__print(&parse_error, events.buf); @@ -1352,6 +1352,7 @@ static int parse_ids(struct perf_pmu *fake_pmu, struct expr_parse_ctx *ids, *out_evlist = parsed_evlist; parsed_evlist = NULL; err_out: + parse_events_error__exit(&parse_error); evlist__delete(parsed_evlist); strbuf_release(&events); return ret; diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c index 75cafb9..5bfb6f8 100644 --- a/tools/perf/util/parse-events.c +++ b/tools/perf/util/parse-events.c @@ -2301,6 +2301,19 @@ int __parse_events(struct evlist *evlist, const char *str, return ret; } +void parse_events_error__init(struct parse_events_error *err) +{ + bzero(err, sizeof(*err)); +} + +void parse_events_error__exit(struct parse_events_error *err) +{ + zfree(&err->str); + zfree(&err->help); + zfree(&err->first_str); + zfree(&err->first_help); +} + void parse_events_error__handle(struct parse_events_error *err, int idx, char *str, char *help) { @@ -2405,15 +2418,11 @@ void parse_events_error__print(struct parse_events_error *err, return; __parse_events_error__print(err->idx, err->str, err->help, event); - zfree(&err->str); - zfree(&err->help); if (err->num_errors > 1) { fputs("\nInitial error:\n", stderr); __parse_events_error__print(err->first_idx, err->first_str, err->first_help, event); - zfree(&err->first_str); - zfree(&err->first_help); } } @@ -2426,13 +2435,14 @@ int parse_events_option(const struct option *opt, const char *str, struct parse_events_error err; int ret; - bzero(&err, sizeof(err)); + parse_events_error__init(&err); ret = parse_events(evlist, str, &err); if (ret) { parse_events_error__print(&err, str); fprintf(stderr, "Run 'perf list' for a list of valid events\n"); } + parse_events_error__exit(&err); return ret; } diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h index 52ac26b..c7fc93f5 100644 --- a/tools/perf/util/parse-events.h +++ b/tools/perf/util/parse-events.h @@ -242,6 +242,8 @@ int is_valid_tracepoint(const char *event_string); int valid_event_mount(const char *eventfs); char *parse_events_formats_error_string(char *additional_terms); +void parse_events_error__init(struct parse_events_error *err); +void parse_events_error__exit(struct parse_events_error *err); void parse_events_error__handle(struct parse_events_error *err, int idx, char *str, char *help); void parse_events_error__print(struct parse_events_error *err, -- 2.7.4