Re: [PATCH v2 2/3] perf parse-event: Add init and exit to parse_event_error

From: Arnaldo Carvalho de Melo
Date: Sun Nov 07 2021 - 10:39:59 EST

Next message: Arnaldo Carvalho de Melo: "Re: [PATCH v2 3/3] perf metric: Fix memory leaks."
Previous message: Sergio Paracuellos: "Re: drivers/pci/controller/pcie-mt7621.c:224:6: error: implicit declaration of function 'mips_cps_numiocu'"
In reply to: Ian Rogers: "[PATCH v2 2/3] perf parse-event: Add init and exit to parse_event_error"
Next in thread: Arnaldo Carvalho de Melo: "Re: [PATCH v2 1/3] perf parse-events: Rename parse_events_error functions"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Em Sun, Nov 07, 2021 at 01:00:01AM -0800, Ian Rogers escreveu:
> 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.

A bit big, could've been split in more patches, but I couldn't find
problems, so I'm applying.

- Arnaldo

> Signed-off-by: Ian Rogers <irogers@xxxxxxxxxx>
> ---
> 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 16510686c138..2a74bec15a3e 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_print_error(&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 3f9518936367..482738e9bdad 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 af447a179d84..7974933dbc77 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 @@ static int add_default_attributes(void)
>
> 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 @@ static int add_default_attributes(void)
> 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 7f0acc94e9ac..624ea12ce5ca 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 b4b9a9488d51..7447a4478991 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 fa03ff0dc083..2bf146e49ce8 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 57b4c5f30324..80cff8a3558c 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 e200af986613..6af94639b14a 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 50b1299fe643..9ae894c406d8 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 b9028e304ddd..4574c46260d9 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 edd7180b24e4..1b43cbc1961d 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 75cafb9a0720..5bfb6f892489 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 52ac26b3720a..c7fc93f54577 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.34.0.rc0.344.g81b53c2807-goog

--

- Arnaldo

Next message: Arnaldo Carvalho de Melo: "Re: [PATCH v2 3/3] perf metric: Fix memory leaks."
Previous message: Sergio Paracuellos: "Re: drivers/pci/controller/pcie-mt7621.c:224:6: error: implicit declaration of function 'mips_cps_numiocu'"
In reply to: Ian Rogers: "[PATCH v2 2/3] perf parse-event: Add init and exit to parse_event_error"
Next in thread: Arnaldo Carvalho de Melo: "Re: [PATCH v2 1/3] perf parse-events: Rename parse_events_error functions"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]