diff mbox series

[v3,3/9] perf tools: ensure config and str in terms are unique

Message ID 20191024190202.109403-4-irogers@google.com
State Not Applicable
Delegated to: BPF Maintainers
Headers show
Series Improvements to memory usage by parse events | expand

Commit Message

Ian Rogers Oct. 24, 2019, 7:01 p.m. UTC 
Make it easier to release memory associated with parse event terms by
duplicating the string for the config name and ensuring the val string
is a duplicate.

Currently the parser may memory leak terms and this is addressed in a
later patch.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/parse-events.c | 51 ++++++++++++++++++++++++++++------
 tools/perf/util/parse-events.y |  4 ++-
 2 files changed, 45 insertions(+), 10 deletions(-)

Comments

Jiri Olsa Oct. 25, 2019, 8:10 a.m. UTC | #1 
On Thu, Oct 24, 2019 at 12:01:56PM -0700, Ian Rogers wrote:
> Make it easier to release memory associated with parse event terms by
> duplicating the string for the config name and ensuring the val string
> is a duplicate.
> 
> Currently the parser may memory leak terms and this is addressed in a
> later patch.

please move that patch before this one 

thanks,
jirka

> 
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/perf/util/parse-events.c | 51 ++++++++++++++++++++++++++++------
>  tools/perf/util/parse-events.y |  4 ++-
>  2 files changed, 45 insertions(+), 10 deletions(-)
> 
> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> index f0d50f079d2f..dc5862a663b5 100644
> --- a/tools/perf/util/parse-events.c
> +++ b/tools/perf/util/parse-events.c
> @@ -1430,7 +1430,6 @@ int parse_events_add_pmu(struct parse_events_state *parse_state,
>  int parse_events_multi_pmu_add(struct parse_events_state *parse_state,
>  			       char *str, struct list_head **listp)
>  {
> -	struct list_head *head;
>  	struct parse_events_term *term;
>  	struct list_head *list;
>  	struct perf_pmu *pmu = NULL;
> @@ -1447,19 +1446,30 @@ int parse_events_multi_pmu_add(struct parse_events_state *parse_state,
>  
>  		list_for_each_entry(alias, &pmu->aliases, list) {
>  			if (!strcasecmp(alias->name, str)) {
> +				struct list_head *head;
> +				char *config;
> +
>  				head = malloc(sizeof(struct list_head));
>  				if (!head)
>  					return -1;
>  				INIT_LIST_HEAD(head);
> -				if (parse_events_term__num(&term, PARSE_EVENTS__TERM_TYPE_USER,
> -							   str, 1, false, &str, NULL) < 0)
> +				config = strdup(str);
> +				if (!config)
> +					return -1;
> +				if (parse_events_term__num(&term,
> +						   PARSE_EVENTS__TERM_TYPE_USER,
> +						   config, 1, false, &config,
> +						   NULL) < 0) {
> +					free(list);
> +					free(config);
>  					return -1;
> +				}
>  				list_add_tail(&term->list, head);
>  
>  				if (!parse_events_add_pmu(parse_state, list,
>  							  pmu->name, head,
>  							  true, true)) {
> -					pr_debug("%s -> %s/%s/\n", str,
> +					pr_debug("%s -> %s/%s/\n", config,
>  						 pmu->name, alias->str);
>  					ok++;
>  				}
> @@ -1468,8 +1478,10 @@ int parse_events_multi_pmu_add(struct parse_events_state *parse_state,
>  			}
>  		}
>  	}
> -	if (!ok)
> +	if (!ok) {
> +		free(list);
>  		return -1;
> +	}
>  	*listp = list;
>  	return 0;
>  }
> @@ -2764,30 +2776,51 @@ int parse_events_term__sym_hw(struct parse_events_term **term,
>  			      char *config, unsigned idx)
>  {
>  	struct event_symbol *sym;
> +	char *str;
>  	struct parse_events_term temp = {
>  		.type_val  = PARSE_EVENTS__TERM_TYPE_STR,
>  		.type_term = PARSE_EVENTS__TERM_TYPE_USER,
> -		.config    = config ?: (char *) "event",
> +		.config    = config,
>  	};
>  
> +	if (!temp.config) {
> +		temp.config = strdup("event");
> +		if (!temp.config)
> +			return -ENOMEM;
> +	}
>  	BUG_ON(idx >= PERF_COUNT_HW_MAX);
>  	sym = &event_symbols_hw[idx];
>  
> -	return new_term(term, &temp, (char *) sym->symbol, 0);
> +	str = strdup(sym->symbol);
> +	if (!str)
> +		return -ENOMEM;
> +	return new_term(term, &temp, str, 0);
>  }
>  
>  int parse_events_term__clone(struct parse_events_term **new,
>  			     struct parse_events_term *term)
>  {
> +	char *str;
>  	struct parse_events_term temp = {
>  		.type_val  = term->type_val,
>  		.type_term = term->type_term,
> -		.config    = term->config,
> +		.config    = NULL,
>  		.err_term  = term->err_term,
>  		.err_val   = term->err_val,
>  	};
>  
> -	return new_term(new, &temp, term->val.str, term->val.num);
> +	if (term->config) {
> +		temp.config = strdup(term->config);
> +		if (!temp.config)
> +			return -ENOMEM;
> +	}
> +	if (term->type_val == PARSE_EVENTS__TERM_TYPE_NUM)
> +		return new_term(new, &temp, NULL, term->val.num);
> +
> +	str = strdup(term->val.str);
> +	if (!str)
> +		return -ENOMEM;
> +	return new_term(new, &temp, str, 0);
>  }
>  
>  int parse_events_copy_term_list(struct list_head *old,
> diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
> index 48126ae4cd13..27d6b187c9b1 100644
> --- a/tools/perf/util/parse-events.y
> +++ b/tools/perf/util/parse-events.y
> @@ -644,9 +644,11 @@ PE_NAME array '=' PE_VALUE
>  PE_DRV_CFG_TERM
>  {
>  	struct parse_events_term *term;
> +	char *config = strdup($1);
>  
> +	ABORT_ON(!config);
>  	ABORT_ON(parse_events_term__str(&term, PARSE_EVENTS__TERM_TYPE_DRV_CFG,
> -					$1, $1, &@1, NULL));
> +					config, $1, &@1, NULL));
>  	$$ = term;
>  }
>  
> -- 
> 2.23.0.866.gb869b98d4c-goog
>
Ian Rogers Oct. 25, 2019, 3:52 p.m. UTC | #2 
On Fri, Oct 25, 2019 at 1:10 AM Jiri Olsa <jolsa@redhat.com> wrote:
>
> On Thu, Oct 24, 2019 at 12:01:56PM -0700, Ian Rogers wrote:
> > Make it easier to release memory associated with parse event terms by
> > duplicating the string for the config name and ensuring the val string
> > is a duplicate.
> >
> > Currently the parser may memory leak terms and this is addressed in a
> > later patch.
>
> please move that patch before this one

Doing that causes a use after free, or freeing of stack or .rodata.
The strings need to be on the heap before they can be cleaned up,
hence the order the patches appear here. There are already memory
leaks here and so while this does make more of them, it is a temporary
state until the later freeing patch is added. I wanted to avoid a
large monolithic patch.

Thanks,
Ian

> thanks,
> jirka
>
> >
> > Signed-off-by: Ian Rogers <irogers@google.com>
> > ---
> >  tools/perf/util/parse-events.c | 51 ++++++++++++++++++++++++++++------
> >  tools/perf/util/parse-events.y |  4 ++-
> >  2 files changed, 45 insertions(+), 10 deletions(-)
> >
> > diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> > index f0d50f079d2f..dc5862a663b5 100644
> > --- a/tools/perf/util/parse-events.c
> > +++ b/tools/perf/util/parse-events.c
> > @@ -1430,7 +1430,6 @@ int parse_events_add_pmu(struct parse_events_state *parse_state,
> >  int parse_events_multi_pmu_add(struct parse_events_state *parse_state,
> >                              char *str, struct list_head **listp)
> >  {
> > -     struct list_head *head;
> >       struct parse_events_term *term;
> >       struct list_head *list;
> >       struct perf_pmu *pmu = NULL;
> > @@ -1447,19 +1446,30 @@ int parse_events_multi_pmu_add(struct parse_events_state *parse_state,
> >
> >               list_for_each_entry(alias, &pmu->aliases, list) {
> >                       if (!strcasecmp(alias->name, str)) {
> > +                             struct list_head *head;
> > +                             char *config;
> > +
> >                               head = malloc(sizeof(struct list_head));
> >                               if (!head)
> >                                       return -1;
> >                               INIT_LIST_HEAD(head);
> > -                             if (parse_events_term__num(&term, PARSE_EVENTS__TERM_TYPE_USER,
> > -                                                        str, 1, false, &str, NULL) < 0)
> > +                             config = strdup(str);
> > +                             if (!config)
> > +                                     return -1;
> > +                             if (parse_events_term__num(&term,
> > +                                                PARSE_EVENTS__TERM_TYPE_USER,
> > +                                                config, 1, false, &config,
> > +                                                NULL) < 0) {
> > +                                     free(list);
> > +                                     free(config);
> >                                       return -1;
> > +                             }
> >                               list_add_tail(&term->list, head);
> >
> >                               if (!parse_events_add_pmu(parse_state, list,
> >                                                         pmu->name, head,
> >                                                         true, true)) {
> > -                                     pr_debug("%s -> %s/%s/\n", str,
> > +                                     pr_debug("%s -> %s/%s/\n", config,
> >                                                pmu->name, alias->str);
> >                                       ok++;
> >                               }
> > @@ -1468,8 +1478,10 @@ int parse_events_multi_pmu_add(struct parse_events_state *parse_state,
> >                       }
> >               }
> >       }
> > -     if (!ok)
> > +     if (!ok) {
> > +             free(list);
> >               return -1;
> > +     }
> >       *listp = list;
> >       return 0;
> >  }
> > @@ -2764,30 +2776,51 @@ int parse_events_term__sym_hw(struct parse_events_term **term,
> >                             char *config, unsigned idx)
> >  {
> >       struct event_symbol *sym;
> > +     char *str;
> >       struct parse_events_term temp = {
> >               .type_val  = PARSE_EVENTS__TERM_TYPE_STR,
> >               .type_term = PARSE_EVENTS__TERM_TYPE_USER,
> > -             .config    = config ?: (char *) "event",
> > +             .config    = config,
> >       };
> >
> > +     if (!temp.config) {
> > +             temp.config = strdup("event");
> > +             if (!temp.config)
> > +                     return -ENOMEM;
> > +     }
> >       BUG_ON(idx >= PERF_COUNT_HW_MAX);
> >       sym = &event_symbols_hw[idx];
> >
> > -     return new_term(term, &temp, (char *) sym->symbol, 0);
> > +     str = strdup(sym->symbol);
> > +     if (!str)
> > +             return -ENOMEM;
> > +     return new_term(term, &temp, str, 0);
> >  }
> >
> >  int parse_events_term__clone(struct parse_events_term **new,
> >                            struct parse_events_term *term)
> >  {
> > +     char *str;
> >       struct parse_events_term temp = {
> >               .type_val  = term->type_val,
> >               .type_term = term->type_term,
> > -             .config    = term->config,
> > +             .config    = NULL,
> >               .err_term  = term->err_term,
> >               .err_val   = term->err_val,
> >       };
> >
> > -     return new_term(new, &temp, term->val.str, term->val.num);
> > +     if (term->config) {
> > +             temp.config = strdup(term->config);
> > +             if (!temp.config)
> > +                     return -ENOMEM;
> > +     }
> > +     if (term->type_val == PARSE_EVENTS__TERM_TYPE_NUM)
> > +             return new_term(new, &temp, NULL, term->val.num);
> > +
> > +     str = strdup(term->val.str);
> > +     if (!str)
> > +             return -ENOMEM;
> > +     return new_term(new, &temp, str, 0);
> >  }
> >
> >  int parse_events_copy_term_list(struct list_head *old,
> > diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
> > index 48126ae4cd13..27d6b187c9b1 100644
> > --- a/tools/perf/util/parse-events.y
> > +++ b/tools/perf/util/parse-events.y
> > @@ -644,9 +644,11 @@ PE_NAME array '=' PE_VALUE
> >  PE_DRV_CFG_TERM
> >  {
> >       struct parse_events_term *term;
> > +     char *config = strdup($1);
> >
> > +     ABORT_ON(!config);
> >       ABORT_ON(parse_events_term__str(&term, PARSE_EVENTS__TERM_TYPE_DRV_CFG,
> > -                                     $1, $1, &@1, NULL));
> > +                                     config, $1, &@1, NULL));
> >       $$ = term;
> >  }
> >
> > --
> > 2.23.0.866.gb869b98d4c-goog
> >
>
diff mbox series

Patch

diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index f0d50f079d2f..dc5862a663b5 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -1430,7 +1430,6 @@  int parse_events_add_pmu(struct parse_events_state *parse_state,
 int parse_events_multi_pmu_add(struct parse_events_state *parse_state,
 			       char *str, struct list_head **listp)
 {
-	struct list_head *head;
 	struct parse_events_term *term;
 	struct list_head *list;
 	struct perf_pmu *pmu = NULL;
@@ -1447,19 +1446,30 @@  int parse_events_multi_pmu_add(struct parse_events_state *parse_state,
 
 		list_for_each_entry(alias, &pmu->aliases, list) {
 			if (!strcasecmp(alias->name, str)) {
+				struct list_head *head;
+				char *config;
+
 				head = malloc(sizeof(struct list_head));
 				if (!head)
 					return -1;
 				INIT_LIST_HEAD(head);
-				if (parse_events_term__num(&term, PARSE_EVENTS__TERM_TYPE_USER,
-							   str, 1, false, &str, NULL) < 0)
+				config = strdup(str);
+				if (!config)
+					return -1;
+				if (parse_events_term__num(&term,
+						   PARSE_EVENTS__TERM_TYPE_USER,
+						   config, 1, false, &config,
+						   NULL) < 0) {
+					free(list);
+					free(config);
 					return -1;
+				}
 				list_add_tail(&term->list, head);
 
 				if (!parse_events_add_pmu(parse_state, list,
 							  pmu->name, head,
 							  true, true)) {
-					pr_debug("%s -> %s/%s/\n", str,
+					pr_debug("%s -> %s/%s/\n", config,
 						 pmu->name, alias->str);
 					ok++;
 				}
@@ -1468,8 +1478,10 @@  int parse_events_multi_pmu_add(struct parse_events_state *parse_state,
 			}
 		}
 	}
-	if (!ok)
+	if (!ok) {
+		free(list);
 		return -1;
+	}
 	*listp = list;
 	return 0;
 }
@@ -2764,30 +2776,51 @@  int parse_events_term__sym_hw(struct parse_events_term **term,
 			      char *config, unsigned idx)
 {
 	struct event_symbol *sym;
+	char *str;
 	struct parse_events_term temp = {
 		.type_val  = PARSE_EVENTS__TERM_TYPE_STR,
 		.type_term = PARSE_EVENTS__TERM_TYPE_USER,
-		.config    = config ?: (char *) "event",
+		.config    = config,
 	};
 
+	if (!temp.config) {
+		temp.config = strdup("event");
+		if (!temp.config)
+			return -ENOMEM;
+	}
 	BUG_ON(idx >= PERF_COUNT_HW_MAX);
 	sym = &event_symbols_hw[idx];
 
-	return new_term(term, &temp, (char *) sym->symbol, 0);
+	str = strdup(sym->symbol);
+	if (!str)
+		return -ENOMEM;
+	return new_term(term, &temp, str, 0);
 }
 
 int parse_events_term__clone(struct parse_events_term **new,
 			     struct parse_events_term *term)
 {
+	char *str;
 	struct parse_events_term temp = {
 		.type_val  = term->type_val,
 		.type_term = term->type_term,
-		.config    = term->config,
+		.config    = NULL,
 		.err_term  = term->err_term,
 		.err_val   = term->err_val,
 	};
 
-	return new_term(new, &temp, term->val.str, term->val.num);
+	if (term->config) {
+		temp.config = strdup(term->config);
+		if (!temp.config)
+			return -ENOMEM;
+	}
+	if (term->type_val == PARSE_EVENTS__TERM_TYPE_NUM)
+		return new_term(new, &temp, NULL, term->val.num);
+
+	str = strdup(term->val.str);
+	if (!str)
+		return -ENOMEM;
+	return new_term(new, &temp, str, 0);
 }
 
 int parse_events_copy_term_list(struct list_head *old,
diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
index 48126ae4cd13..27d6b187c9b1 100644
--- a/tools/perf/util/parse-events.y
+++ b/tools/perf/util/parse-events.y
@@ -644,9 +644,11 @@  PE_NAME array '=' PE_VALUE
 PE_DRV_CFG_TERM
 {
 	struct parse_events_term *term;
+	char *config = strdup($1);
 
+	ABORT_ON(!config);
 	ABORT_ON(parse_events_term__str(&term, PARSE_EVENTS__TERM_TYPE_DRV_CFG,
-					$1, $1, &@1, NULL));
+					config, $1, &@1, NULL));
 	$$ = term;
 }