Message-ID: <YYfzScy1XicE0/nc@kernel.org>
Date:   Sun, 7 Nov 2021 12:39:53 -0300
From:   Arnaldo Carvalho de Melo <acme@...nel.org>
To:     Ian Rogers <irogers@...gle.com>
Cc:     Peter Zijlstra <peterz@...radead.org>,
        Ingo Molnar <mingo@...hat.com>,
        Mark Rutland <mark.rutland@....com>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        Jiri Olsa <jolsa@...hat.com>,
        Namhyung Kim <namhyung@...nel.org>,
        John Garry <john.garry@...wei.com>,
        linux-perf-users@...r.kernel.org, linux-kernel@...r.kernel.org,
        eranian@...gle.com
Subject: Re: [PATCH v2 2/3] perf parse-event: Add init and exit to
 parse_event_error

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@...gle.com>
> ---
>  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

Powered by blists - more mailing lists