* [PATCH next] perf tools: fix potential memleak in perf events parser @ 2020-06-11 1:42 Chen Wandun 2020-06-11 13:16 ` Arnaldo Carvalho de Melo 0 siblings, 1 reply; 3+ messages in thread From: Chen Wandun @ 2020-06-11 1:42 UTC (permalink / raw) To: acme, linux-kernel; +Cc: peterz, mingo, cj.chengjian, chenwandun Fix potential memory leak in function parse_events_term__sym_hw() and parse_events_term__clone(). 1. Free memory when errors occur. 2. Function new_term may return error, so it is need to free memory when the return value is negative. Signed-off-by: Chen Wandun <chenwandun@huawei.com> --- tools/perf/util/parse-events.c | 40 +++++++++++++++++++++++++++++----- 1 file changed, 34 insertions(+), 6 deletions(-) diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c index 3decbb203846..3491c18edd71 100644 --- a/tools/perf/util/parse-events.c +++ b/tools/perf/util/parse-events.c @@ -2947,6 +2947,7 @@ int parse_events_term__sym_hw(struct parse_events_term **term, .type_term = PARSE_EVENTS__TERM_TYPE_USER, .config = config, }; + int ret; if (!temp.config) { temp.config = strdup("event"); @@ -2957,9 +2958,20 @@ int parse_events_term__sym_hw(struct parse_events_term **term, sym = &event_symbols_hw[idx]; str = strdup(sym->symbol); - if (!str) + if (!str) { + if (!config) + free(temp.config); return -ENOMEM; - return new_term(term, &temp, str, 0); + } + + ret = new_term(term, &temp, str, 0); + if (ret < 0) { + free(str); + if (!config) + free(temp.config); + } + + return ret; } int parse_events_term__clone(struct parse_events_term **new, @@ -2973,19 +2985,35 @@ int parse_events_term__clone(struct parse_events_term **new, .err_term = term->err_term, .err_val = term->err_val, }; + int ret; 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); + if (term->type_val == PARSE_EVENTS__TERM_TYPE_NUM) { + ret = new_term(new, &temp, NULL, term->val.num); + if (ret < 0 && term->config) + free(temp.config); + return ret; + } str = strdup(term->val.str); - if (!str) + if (!str) { + if (term->config) + free(temp.config); return -ENOMEM; - return new_term(new, &temp, str, 0); + } + + ret = new_term(new, &temp, str, 0); + if (ret < 0) { + free(str); + if (term->config) + free(temp.config); + } + + return ret; } void parse_events_term__delete(struct parse_events_term *term) -- 2.17.1 ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH next] perf tools: fix potential memleak in perf events parser 2020-06-11 1:42 [PATCH next] perf tools: fix potential memleak in perf events parser Chen Wandun @ 2020-06-11 13:16 ` Arnaldo Carvalho de Melo 2020-06-11 15:05 ` Chen Wandun 0 siblings, 1 reply; 3+ messages in thread From: Arnaldo Carvalho de Melo @ 2020-06-11 13:16 UTC (permalink / raw) To: Chen Wandun; +Cc: linux-kernel, peterz, mingo, cj.chengjian Em Thu, Jun 11, 2020 at 09:42:34AM +0800, Chen Wandun escreveu: > Fix potential memory leak in function parse_events_term__sym_hw() > and parse_events_term__clone(). > > 1. Free memory when errors occur. > 2. Function new_term may return error, so it is need to free memory > when the return value is negative. Try to fix one thing per patch, i.e. first the most obvious one, then the other that requires going thru the new_term() logic, i.e. first this, which is super easy to review: diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c index c4906a6a9f1a..3ada3874a90a 100644 --- a/tools/perf/util/parse-events.c +++ b/tools/perf/util/parse-events.c @@ -2958,8 +2958,10 @@ int parse_events_term__sym_hw(struct parse_events_term **term, sym = &event_symbols_hw[idx]; str = strdup(sym->symbol); - if (!str) + if (!str) { + zfree(&temp.config); return -ENOMEM; + } return new_term(term, &temp, str, 0); } @@ -2984,8 +2986,10 @@ int parse_events_term__clone(struct parse_events_term **new, return new_term(new, &temp, NULL, term->val.num); str = strdup(term->val.str); - if (!str) + if (!str) { + zfree(&temp.config); return -ENOMEM; + } return new_term(new, &temp, str, 0); } Then you go to the one that requires the reviewer (now or in the future, trying to figure out why something broke) to look at the new_term() code, ok? - Arnaldo > Signed-off-by: Chen Wandun <chenwandun@huawei.com> > --- > tools/perf/util/parse-events.c | 40 +++++++++++++++++++++++++++++----- > 1 file changed, 34 insertions(+), 6 deletions(-) > > diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c > index 3decbb203846..3491c18edd71 100644 > --- a/tools/perf/util/parse-events.c > +++ b/tools/perf/util/parse-events.c > @@ -2947,6 +2947,7 @@ int parse_events_term__sym_hw(struct parse_events_term **term, > .type_term = PARSE_EVENTS__TERM_TYPE_USER, > .config = config, > }; > + int ret; > > if (!temp.config) { > temp.config = strdup("event"); > @@ -2957,9 +2958,20 @@ int parse_events_term__sym_hw(struct parse_events_term **term, > sym = &event_symbols_hw[idx]; > > str = strdup(sym->symbol); > - if (!str) > + if (!str) { > + if (!config) > + free(temp.config); > return -ENOMEM; > - return new_term(term, &temp, str, 0); > + } > + > + ret = new_term(term, &temp, str, 0); > + if (ret < 0) { > + free(str); > + if (!config) > + free(temp.config); > + } > + > + return ret; > } > > int parse_events_term__clone(struct parse_events_term **new, > @@ -2973,19 +2985,35 @@ int parse_events_term__clone(struct parse_events_term **new, > .err_term = term->err_term, > .err_val = term->err_val, > }; > + int ret; > > 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); > + if (term->type_val == PARSE_EVENTS__TERM_TYPE_NUM) { > + ret = new_term(new, &temp, NULL, term->val.num); > + if (ret < 0 && term->config) > + free(temp.config); > + return ret; > + } > > str = strdup(term->val.str); > - if (!str) > + if (!str) { > + if (term->config) > + free(temp.config); > return -ENOMEM; > - return new_term(new, &temp, str, 0); > + } > + > + ret = new_term(new, &temp, str, 0); > + if (ret < 0) { > + free(str); > + if (term->config) > + free(temp.config); > + } > + > + return ret; > } > > void parse_events_term__delete(struct parse_events_term *term) > -- > 2.17.1 > -- - Arnaldo ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH next] perf tools: fix potential memleak in perf events parser 2020-06-11 13:16 ` Arnaldo Carvalho de Melo @ 2020-06-11 15:05 ` Chen Wandun 0 siblings, 0 replies; 3+ messages in thread From: Chen Wandun @ 2020-06-11 15:05 UTC (permalink / raw) To: Arnaldo Carvalho de Melo, Markus.Elfring Cc: linux-kernel, peterz, mingo, cj.chengjian, chenwandun 在 2020/6/11 21:16, Arnaldo Carvalho de Melo 写道: > Em Thu, Jun 11, 2020 at 09:42:34AM +0800, Chen Wandun escreveu: >> Fix potential memory leak in function parse_events_term__sym_hw() >> and parse_events_term__clone(). >> >> 1. Free memory when errors occur. >> 2. Function new_term may return error, so it is need to free memory >> when the return value is negative. > Try to fix one thing per patch, i.e. first the most obvious one, then > the other that requires going thru the new_term() logic, i.e. first > this, which is super easy to review: > Would you like to add the tag “Fixes” to the commit message? > Hi, Arnaldo and Markus. Thank you for your reply, I will update in v2 Best Regards, Chen Wandun ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-06-11 15:05 UTC | newest] Thread overview: 3+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2020-06-11 1:42 [PATCH next] perf tools: fix potential memleak in perf events parser Chen Wandun 2020-06-11 13:16 ` Arnaldo Carvalho de Melo 2020-06-11 15:05 ` Chen Wandun
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox