Re: [PATCH v3 1/9] perf tools: add parse events append error

[Jiri Olsa <jolsa@redhat.com>]

On Thu, Oct 24, 2019 at 12:01:54PM -0700, Ian Rogers wrote:
> Parse event error handling may overwrite one error string with another
> creating memory leaks and masking errors. Introduce a helper routine
> that appends error messages and avoids the memory leak.
> 
> A reproduction of this problem can be seen with:
>   perf stat -e c/c/
> After this change this produces:
> event syntax error: 'c/c/'
>                        \___ unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: Cannot find PMU `c'. Missing kernel support?)(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,pc,in_tx,edge,any,offcore_rsp,in_tx_cp,ldlat,inv,umask,frontend,cmask,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))


hum... I'd argue that the previous state was better:

[jolsa@krava perf]$ ./perf stat -e c/c/
event syntax error: 'c/c/'
                       \___ unknown term


jirka

> 
> valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore
> Run 'perf list' for a list of valid events
> 
>  Usage: perf stat [<options>] [<command>]
> 
>     -e, --event <event>   event selector. use 'perf list' to list available events
> 
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/perf/util/parse-events.c | 100 +++++++++++++++++++++++----------
>  tools/perf/util/parse-events.h |   2 +
>  tools/perf/util/pmu.c          |  30 ++++++----
>  3 files changed, 89 insertions(+), 43 deletions(-)
> 
> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> index db882f630f7e..edb3ae76777d 100644
> --- a/tools/perf/util/parse-events.c
> +++ b/tools/perf/util/parse-events.c
> @@ -182,6 +182,38 @@ static int tp_event_has_id(const char *dir_path, struct dirent *evt_dir)
>  
>  #define MAX_EVENT_LENGTH 512
>  
> +void parse_events__append_error(struct parse_events_error *err, int idx,
> +				char *str, char *help)
> +{
> +	char *new_str = NULL;
> +
> +	if (WARN(!str, "WARNING: failed to provide error string")) {
> +		free(help);
> +		return;
> +	}
> +	if (err->str) {
> +		int ret;
> +
> +		if (err->help) {
> +			ret = asprintf(&new_str,
> +				"%s (previous error: %s(help: %s))",
> +				str, err->str, err->help);
> +		} else {
> +			ret = asprintf(&new_str,
> +				"%s (previous error: %s)",
> +				str, err->str);
> +		}
> +		if (ret < 0)
> +			new_str = NULL;
> +		else
> +			zfree(&str);
> +	}
> +	err->idx = idx;
> +	free(err->str);
> +	err->str = new_str ?: str;
> +	free(err->help);
> +	err->help = help;
> +}
>  
>  struct tracepoint_path *tracepoint_id_to_path(u64 config)
>  {
> @@ -932,11 +964,11 @@ static int check_type_val(struct parse_events_term *term,
>  		return 0;
>  
>  	if (err) {
> -		err->idx = term->err_val;
> -		if (type == PARSE_EVENTS__TERM_TYPE_NUM)
> -			err->str = strdup("expected numeric value");
> -		else
> -			err->str = strdup("expected string value");
> +		parse_events__append_error(err, term->err_val,
> +					type == PARSE_EVENTS__TERM_TYPE_NUM
> +					? strdup("expected numeric value")
> +					: strdup("expected string value"),
> +					NULL);
>  	}
>  	return -EINVAL;
>  }
> @@ -972,8 +1004,11 @@ static bool config_term_shrinked;
>  static bool
>  config_term_avail(int term_type, struct parse_events_error *err)
>  {
> +	char *err_str;
> +
>  	if (term_type < 0 || term_type >= __PARSE_EVENTS__TERM_TYPE_NR) {
> -		err->str = strdup("Invalid term_type");
> +		parse_events__append_error(err, -1,
> +					strdup("Invalid term_type"), NULL);
>  		return false;
>  	}
>  	if (!config_term_shrinked)
> @@ -992,9 +1027,9 @@ config_term_avail(int term_type, struct parse_events_error *err)
>  			return false;
>  
>  		/* term_type is validated so indexing is safe */
> -		if (asprintf(&err->str, "'%s' is not usable in 'perf stat'",
> -			     config_term_names[term_type]) < 0)
> -			err->str = NULL;
> +		if (asprintf(&err_str, "'%s' is not usable in 'perf stat'",
> +				config_term_names[term_type]) >= 0)
> +			parse_events__append_error(err, -1, err_str, NULL);
>  		return false;
>  	}
>  }
> @@ -1036,17 +1071,20 @@ do {									   \
>  	case PARSE_EVENTS__TERM_TYPE_BRANCH_SAMPLE_TYPE:
>  		CHECK_TYPE_VAL(STR);
>  		if (strcmp(term->val.str, "no") &&
> -		    parse_branch_str(term->val.str, &attr->branch_sample_type)) {
> -			err->str = strdup("invalid branch sample type");
> -			err->idx = term->err_val;
> +		    parse_branch_str(term->val.str,
> +				    &attr->branch_sample_type)) {
> +			parse_events__append_error(err, term->err_val,
> +					strdup("invalid branch sample type"),
> +					NULL);
>  			return -EINVAL;
>  		}
>  		break;
>  	case PARSE_EVENTS__TERM_TYPE_TIME:
>  		CHECK_TYPE_VAL(NUM);
>  		if (term->val.num > 1) {
> -			err->str = strdup("expected 0 or 1");
> -			err->idx = term->err_val;
> +			parse_events__append_error(err, term->err_val,
> +						strdup("expected 0 or 1"),
> +						NULL);
>  			return -EINVAL;
>  		}
>  		break;
> @@ -1080,8 +1118,9 @@ do {									   \
>  	case PARSE_EVENTS__TERM_TYPE_PERCORE:
>  		CHECK_TYPE_VAL(NUM);
>  		if ((unsigned int)term->val.num > 1) {
> -			err->str = strdup("expected 0 or 1");
> -			err->idx = term->err_val;
> +			parse_events__append_error(err, term->err_val,
> +						strdup("expected 0 or 1"),
> +						NULL);
>  			return -EINVAL;
>  		}
>  		break;
> @@ -1089,9 +1128,9 @@ do {									   \
>  		CHECK_TYPE_VAL(NUM);
>  		break;
>  	default:
> -		err->str = strdup("unknown term");
> -		err->idx = term->err_term;
> -		err->help = parse_events_formats_error_string(NULL);
> +		parse_events__append_error(err, term->err_term,
> +				strdup("unknown term"),
> +				parse_events_formats_error_string(NULL));
>  		return -EINVAL;
>  	}
>  
> @@ -1142,9 +1181,9 @@ static int config_term_tracepoint(struct perf_event_attr *attr,
>  		return config_term_common(attr, term, err);
>  	default:
>  		if (err) {
> -			err->idx = term->err_term;
> -			err->str = strdup("unknown term");
> -			err->help = strdup("valid terms: call-graph,stack-size\n");
> +			parse_events__append_error(err, term->err_term,
> +				strdup("unknown term"),
> +				strdup("valid terms: call-graph,stack-size\n"));
>  		}
>  		return -EINVAL;
>  	}
> @@ -1323,10 +1362,12 @@ int parse_events_add_pmu(struct parse_events_state *parse_state,
>  
>  	pmu = perf_pmu__find(name);
>  	if (!pmu) {
> -		if (asprintf(&err->str,
> +		char *err_str;
> +
> +		if (asprintf(&err_str,
>  				"Cannot find PMU `%s'. Missing kernel support?",
> -				name) < 0)
> -			err->str = NULL;
> +				name) >= 0)
> +			parse_events__append_error(err, -1, err_str, NULL);
>  		return -EINVAL;
>  	}
>  
> @@ -2797,13 +2838,10 @@ void parse_events__clear_array(struct parse_events_array *a)
>  void parse_events_evlist_error(struct parse_events_state *parse_state,
>  			       int idx, const char *str)
>  {
> -	struct parse_events_error *err = parse_state->error;
> -
> -	if (!err)
> +	if (!parse_state->error)
>  		return;
> -	err->idx = idx;
> -	err->str = strdup(str);
> -	WARN_ONCE(!err->str, "WARNING: failed to allocate error string");
> +
> +	parse_events__append_error(parse_state->error, idx, strdup(str), NULL);
>  }
>  
>  static void config_terms_list(char *buf, size_t buf_sz)
> diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h
> index 769e07cddaa2..a7d42faeab0c 100644
> --- a/tools/perf/util/parse-events.h
> +++ b/tools/perf/util/parse-events.h
> @@ -124,6 +124,8 @@ struct parse_events_state {
>  	struct list_head	  *terms;
>  };
>  
> +void parse_events__append_error(struct parse_events_error *err, int idx,
> +				char *str, char *help);
>  void parse_events__shrink_config_terms(void);
>  int parse_events__is_hardcoded_term(struct parse_events_term *term);
>  int parse_events_term__num(struct parse_events_term **term,
> diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
> index adbe97e941dd..4015ec11944a 100644
> --- a/tools/perf/util/pmu.c
> +++ b/tools/perf/util/pmu.c
> @@ -1050,9 +1050,9 @@ static int pmu_config_term(struct list_head *formats,
>  		if (err) {
>  			char *pmu_term = pmu_formats_string(formats);
>  
> -			err->idx  = term->err_term;
> -			err->str  = strdup("unknown term");
> -			err->help = parse_events_formats_error_string(pmu_term);
> +			parse_events__append_error(err, term->err_term,
> +				strdup("unknown term"),
> +				parse_events_formats_error_string(pmu_term));
>  			free(pmu_term);
>  		}
>  		return -EINVAL;
> @@ -1080,8 +1080,9 @@ static int pmu_config_term(struct list_head *formats,
>  		if (term->no_value &&
>  		    bitmap_weight(format->bits, PERF_PMU_FORMAT_BITS) > 1) {
>  			if (err) {
> -				err->idx = term->err_val;
> -				err->str = strdup("no value assigned for term");
> +				parse_events__append_error(err, term->err_val,
> +					   strdup("no value assigned for term"),
> +					   NULL);
>  			}
>  			return -EINVAL;
>  		}
> @@ -1094,8 +1095,9 @@ static int pmu_config_term(struct list_head *formats,
>  						term->config, term->val.str);
>  			}
>  			if (err) {
> -				err->idx = term->err_val;
> -				err->str = strdup("expected numeric value");
> +				parse_events__append_error(err, term->err_val,
> +					strdup("expected numeric value"),
> +					NULL);
>  			}
>  			return -EINVAL;
>  		}
> @@ -1108,11 +1110,15 @@ static int pmu_config_term(struct list_head *formats,
>  	max_val = pmu_format_max_value(format->bits);
>  	if (val > max_val) {
>  		if (err) {
> -			err->idx = term->err_val;
> -			if (asprintf(&err->str,
> -				     "value too big for format, maximum is %llu",
> -				     (unsigned long long)max_val) < 0)
> -				err->str = strdup("value too big for format");
> +			char *err_str;
> +
> +			parse_events__append_error(err, term->err_val,
> +				asprintf(&err_str,
> +				    "value too big for format, maximum is %llu",
> +				    (unsigned long long)max_val) < 0
> +				    ? strdup("value too big for format")
> +				    : err_str,
> +				    NULL);
>  			return -EINVAL;
>  		}
>  		/*
> -- 
> 2.23.0.866.gb869b98d4c-goog
>