[PATCH 0/4] perf: Fix memory errors

[PATCH 0/4] perf: Fix memory errors

[Wei Li]

Fix several memory errors in perf tool.

Hongbo Yao (1):
  perf metrictroup: Fix memory leak of metric_events

Li Bin (2):
  perf svghelper: Fix memory leak in svg_build_topology_map
  perf util: Fix potential segment fault in put_tracepoints_path

Xie XiuQi (1):
  perf util: Fix memory leak of prefix_if_not_in

 tools/perf/util/metricgroup.c      |  3 +++
 tools/perf/util/sort.c             |  2 +-
 tools/perf/util/svghelper.c        | 10 +++++++---
 tools/perf/util/trace-event-info.c |  2 +-
 4 files changed, 12 insertions(+), 5 deletions(-)

-- 
2.17.1

[PATCH 1/4] perf metrictroup: Fix memory leak of metric_events

[Wei Li]

From: Hongbo Yao <yaohongbo@huawei.com>

Fix memory leak of metric_events in function metricgroup__setup_events()

Signed-off-by: Hongbo Yao <yaohongbo@huawei.com>
---
 tools/perf/util/metricgroup.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
index 926449a7cdbf..69bf0f4d646e 100644
--- a/tools/perf/util/metricgroup.c
+++ b/tools/perf/util/metricgroup.c
@@ -184,6 +184,7 @@ static int metricgroup__setup_events(struct list_head *groups,
 		if (!evsel) {
 			pr_debug("Cannot resolve %s: %s\n",
 					eg->metric_name, eg->metric_expr);
+			free(metric_events);
 			continue;
 		}
 		for (i = 0; i < eg->idnum; i++)
@@ -191,11 +192,13 @@ static int metricgroup__setup_events(struct list_head *groups,
 		me = metricgroup__lookup(metric_events_list, evsel, true);
 		if (!me) {
 			ret = -ENOMEM;
+			free(metric_events);
 			break;
 		}
 		expr = malloc(sizeof(struct metric_expr));
 		if (!expr) {
 			ret = -ENOMEM;
+			free(metric_events);
 			break;
 		}
 		expr->metric_expr = eg->metric_expr;
-- 
2.17.1

[PATCH 2/4] perf svghelper: Fix memory leak in svg_build_topology_map

[Wei Li]

From: Li Bin <huawei.libin@huawei.com>

Fix leak of memory pointed to by t.sib_thr and t.sib_core in
svg_build_topology_map.

Signed-off-by: Li Bin <huawei.libin@huawei.com>
---
 tools/perf/util/svghelper.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/tools/perf/util/svghelper.c b/tools/perf/util/svghelper.c
index 96f941e01681..e2b3b0e2fafe 100644
--- a/tools/perf/util/svghelper.c
+++ b/tools/perf/util/svghelper.c
@@ -754,6 +754,7 @@ int svg_build_topology_map(struct perf_env *env)
 	int i, nr_cpus;
 	struct topology t;
 	char *sib_core, *sib_thr;
+	int ret;
 
 	nr_cpus = min(env->nr_cpus_online, MAX_NR_CPUS);
 
@@ -767,12 +768,14 @@ int svg_build_topology_map(struct perf_env *env)
 
 	if (!t.sib_core || !t.sib_thr) {
 		fprintf(stderr, "topology: no memory\n");
+		ret = -1;
 		goto exit;
 	}
 
 	for (i = 0; i < env->nr_sibling_cores; i++) {
 		if (str_to_bitmap(sib_core, &t.sib_core[i], nr_cpus)) {
 			fprintf(stderr, "topology: can't parse siblings map\n");
+			ret = -1;
 			goto exit;
 		}
 
@@ -782,6 +785,7 @@ int svg_build_topology_map(struct perf_env *env)
 	for (i = 0; i < env->nr_sibling_threads; i++) {
 		if (str_to_bitmap(sib_thr, &t.sib_thr[i], nr_cpus)) {
 			fprintf(stderr, "topology: can't parse siblings map\n");
+			ret = -1;
 			goto exit;
 		}
 
@@ -791,6 +795,7 @@ int svg_build_topology_map(struct perf_env *env)
 	topology_map = malloc(sizeof(int) * nr_cpus);
 	if (!topology_map) {
 		fprintf(stderr, "topology: no memory\n");
+		ret = -1;
 		goto exit;
 	}
 
@@ -798,12 +803,11 @@ int svg_build_topology_map(struct perf_env *env)
 		topology_map[i] = -1;
 
 	scan_core_topology(topology_map, &t, nr_cpus);
-
-	return 0;
+	ret = 0;
 
 exit:
 	zfree(&t.sib_core);
 	zfree(&t.sib_thr);
 
-	return -1;
+	return ret;
 }
-- 
2.17.1

[PATCH 3/4] perf util: Fix memory leak of prefix_if_not_in

[Wei Li]

From: Xie XiuQi <xiexiuqi@huawei.com>

Need to free "str" before return when asprintf() failed
to avoid memory leak.

Signed-off-by: Xie XiuQi <xiexiuqi@huawei.com>
---
 tools/perf/util/sort.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
index f14cc728c358..8ed777565c82 100644
--- a/tools/perf/util/sort.c
+++ b/tools/perf/util/sort.c
@@ -2811,7 +2811,7 @@ static char *prefix_if_not_in(const char *pre, char *str)
 		return str;
 
 	if (asprintf(&n, "%s,%s", pre, str) < 0)
-		return NULL;
+		n = NULL;
 
 	free(str);
 	return n;
-- 
2.17.1

[PATCH 4/4] perf util: Fix potential segment fault in put_tracepoints_path

[Wei Li]

From: Li Bin <huawei.libin@huawei.com>

This patch fix potential segment fault triggerd in put_tracepoints_path
when the address of the local variable 'path' be freed in error path
of record_saved_cmdline.

Signed-off-by: Li Bin <huawei.libin@huawei.com>
---
 tools/perf/util/trace-event-info.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/util/trace-event-info.c b/tools/perf/util/trace-event-info.c
index 086e98ff42a3..0e5c4786f296 100644
--- a/tools/perf/util/trace-event-info.c
+++ b/tools/perf/util/trace-event-info.c
@@ -428,7 +428,7 @@ get_tracepoints_path(struct list_head *pattrs)
 		if (!ppath->next) {
 error:
 			pr_debug("No memory to alloc tracepoints list\n");
-			put_tracepoints_path(&path);
+			put_tracepoints_path(path.next);
 			return NULL;
 		}
 next:
-- 
2.17.1

Re: [PATCH 2/4] perf svghelper: Fix memory leak in svg_build_topology_map

[Arnaldo Carvalho de Melo]

Em Thu, May 21, 2020 at 09:32:16PM +0800, Wei Li escreveu:
> From: Li Bin <huawei.libin@huawei.com>
> 
> Fix leak of memory pointed to by t.sib_thr and t.sib_core in
> svg_build_topology_map.
> 
> Signed-off-by: Li Bin <huawei.libin@huawei.com>
> ---
>  tools/perf/util/svghelper.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/perf/util/svghelper.c b/tools/perf/util/svghelper.c
> index 96f941e01681..e2b3b0e2fafe 100644
> --- a/tools/perf/util/svghelper.c
> +++ b/tools/perf/util/svghelper.c
> @@ -754,6 +754,7 @@ int svg_build_topology_map(struct perf_env *env)
>  	int i, nr_cpus;
>  	struct topology t;
>  	char *sib_core, *sib_thr;
> +	int ret;

Please set ret to -1 here

	int ret = -1;
	
So that you don't have to add all those lines below...

>  
>  	nr_cpus = min(env->nr_cpus_online, MAX_NR_CPUS);
>  
> @@ -767,12 +768,14 @@ int svg_build_topology_map(struct perf_env *env)
>  
>  	if (!t.sib_core || !t.sib_thr) {
>  		fprintf(stderr, "topology: no memory\n");
> +		ret = -1;
>  		goto exit;
>  	}
>  
>  	for (i = 0; i < env->nr_sibling_cores; i++) {
>  		if (str_to_bitmap(sib_core, &t.sib_core[i], nr_cpus)) {
>  			fprintf(stderr, "topology: can't parse siblings map\n");
> +			ret = -1;
>  			goto exit;
>  		}
>  
> @@ -782,6 +785,7 @@ int svg_build_topology_map(struct perf_env *env)
>  	for (i = 0; i < env->nr_sibling_threads; i++) {
>  		if (str_to_bitmap(sib_thr, &t.sib_thr[i], nr_cpus)) {
>  			fprintf(stderr, "topology: can't parse siblings map\n");
> +			ret = -1;
>  			goto exit;
>  		}
>  
> @@ -791,6 +795,7 @@ int svg_build_topology_map(struct perf_env *env)
>  	topology_map = malloc(sizeof(int) * nr_cpus);
>  	if (!topology_map) {
>  		fprintf(stderr, "topology: no memory\n");
> +		ret = -1;
>  		goto exit;
>  	}
>  
> @@ -798,12 +803,11 @@ int svg_build_topology_map(struct perf_env *env)
>  		topology_map[i] = -1;
>  
>  	scan_core_topology(topology_map, &t, nr_cpus);
> -
> -	return 0;

... as you'll set it to zero here :-)

> +	ret = 0;
>  
>  exit:
>  	zfree(&t.sib_core);
>  	zfree(&t.sib_thr);
>  
> -	return -1;
> +	return ret;
>  }
> -- 
> 2.17.1
> 

-- 

- Arnaldo

Re: [PATCH 3/4] perf util: Fix memory leak of prefix_if_not_in

[Arnaldo Carvalho de Melo]

Em Thu, May 21, 2020 at 09:32:17PM +0800, Wei Li escreveu:
> From: Xie XiuQi <xiexiuqi@huawei.com>
> 
> Need to free "str" before return when asprintf() failed
> to avoid memory leak.

Thanks, applied.

- Arnaldo
 
> Signed-off-by: Xie XiuQi <xiexiuqi@huawei.com>
> ---
>  tools/perf/util/sort.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
> index f14cc728c358..8ed777565c82 100644
> --- a/tools/perf/util/sort.c
> +++ b/tools/perf/util/sort.c
> @@ -2811,7 +2811,7 @@ static char *prefix_if_not_in(const char *pre, char *str)
>  		return str;
>  
>  	if (asprintf(&n, "%s,%s", pre, str) < 0)
> -		return NULL;
> +		n = NULL;
>  
>  	free(str);
>  	return n;
> -- 
> 2.17.1
> 

-- 

- Arnaldo

Re: [PATCH 1/4] perf metrictroup: Fix memory leak of metric_events

[Arnaldo Carvalho de Melo]

Em Thu, May 21, 2020 at 09:32:15PM +0800, Wei Li escreveu:
> From: Hongbo Yao <yaohongbo@huawei.com>
> 
> Fix memory leak of metric_events in function metricgroup__setup_events()

The leak is there but this code has changed a lot recently, please take
a look at my perf/core branch and keep Ian Rogers in the loop,

- Arnaldo
 
> Signed-off-by: Hongbo Yao <yaohongbo@huawei.com>
> ---
>  tools/perf/util/metricgroup.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
> index 926449a7cdbf..69bf0f4d646e 100644
> --- a/tools/perf/util/metricgroup.c
> +++ b/tools/perf/util/metricgroup.c
> @@ -184,6 +184,7 @@ static int metricgroup__setup_events(struct list_head *groups,
>  		if (!evsel) {
>  			pr_debug("Cannot resolve %s: %s\n",
>  					eg->metric_name, eg->metric_expr);
> +			free(metric_events);
>  			continue;
>  		}
>  		for (i = 0; i < eg->idnum; i++)
> @@ -191,11 +192,13 @@ static int metricgroup__setup_events(struct list_head *groups,
>  		me = metricgroup__lookup(metric_events_list, evsel, true);
>  		if (!me) {
>  			ret = -ENOMEM;
> +			free(metric_events);
>  			break;
>  		}
>  		expr = malloc(sizeof(struct metric_expr));
>  		if (!expr) {
>  			ret = -ENOMEM;
> +			free(metric_events);
>  			break;
>  		}
>  		expr->metric_expr = eg->metric_expr;
> -- 
> 2.17.1
> 

-- 

- Arnaldo

Re: [PATCH 1/4] perf metrictroup: Fix memory leak of metric_events

[Ian Rogers]

On Thu, May 21, 2020 at 8:54 AM Arnaldo Carvalho de Melo
<acme@kernel.org> wrote:
>
> Em Thu, May 21, 2020 at 09:32:15PM +0800, Wei Li escreveu:
> > From: Hongbo Yao <yaohongbo@huawei.com>
> >
> > Fix memory leak of metric_events in function metricgroup__setup_events()
>
> The leak is there but this code has changed a lot recently, please take
> a look at my perf/core branch and keep Ian Rogers in the loop,
>
> - Arnaldo

Thanks, I'd posted the same change here:
https://lore.kernel.org/lkml/20200508053629.210324-10-irogers@google.com/
but it looks like it was mislaid. Arnaldo, I can re-post that patch or
Acked-by this patch. I'm easy either way.

Thanks,
Ian

> > Signed-off-by: Hongbo Yao <yaohongbo@huawei.com>
> > ---
> >  tools/perf/util/metricgroup.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
> > index 926449a7cdbf..69bf0f4d646e 100644
> > --- a/tools/perf/util/metricgroup.c
> > +++ b/tools/perf/util/metricgroup.c
> > @@ -184,6 +184,7 @@ static int metricgroup__setup_events(struct list_head *groups,
> >               if (!evsel) {
> >                       pr_debug("Cannot resolve %s: %s\n",
> >                                       eg->metric_name, eg->metric_expr);
> > +                     free(metric_events);
> >                       continue;
> >               }
> >               for (i = 0; i < eg->idnum; i++)
> > @@ -191,11 +192,13 @@ static int metricgroup__setup_events(struct list_head *groups,
> >               me = metricgroup__lookup(metric_events_list, evsel, true);
> >               if (!me) {
> >                       ret = -ENOMEM;
> > +                     free(metric_events);
> >                       break;
> >               }
> >               expr = malloc(sizeof(struct metric_expr));
> >               if (!expr) {
> >                       ret = -ENOMEM;
> > +                     free(metric_events);
> >                       break;
> >               }
> >               expr->metric_expr = eg->metric_expr;
> > --
> > 2.17.1
> >
>
> --
>
> - Arnaldo

Re: [PATCH 2/4] perf svghelper: Fix memory leak in svg_build_topology_map

[LiBin (Huawei)]

在 2020/5/21 22:15, Arnaldo Carvalho de Melo 写道:
> Em Thu, May 21, 2020 at 09:32:16PM +0800, Wei Li escreveu:
>> From: Li Bin <huawei.libin@huawei.com>
>>
>> Fix leak of memory pointed to by t.sib_thr and t.sib_core in
>> svg_build_topology_map.
>>
>> Signed-off-by: Li Bin <huawei.libin@huawei.com>
>> ---
>>   tools/perf/util/svghelper.c | 10 +++++++---
>>   1 file changed, 7 insertions(+), 3 deletions(-)
>>
>> diff --git a/tools/perf/util/svghelper.c b/tools/perf/util/svghelper.c
>> index 96f941e01681..e2b3b0e2fafe 100644
>> --- a/tools/perf/util/svghelper.c
>> +++ b/tools/perf/util/svghelper.c
>> @@ -754,6 +754,7 @@ int svg_build_topology_map(struct perf_env *env)
>>   	int i, nr_cpus;
>>   	struct topology t;
>>   	char *sib_core, *sib_thr;
>> +	int ret;
> Please set ret to -1 here
>
> 	int ret = -1;
> 	
> So that you don't have to add all those lines below...
>
>>   
>>   	nr_cpus = min(env->nr_cpus_online, MAX_NR_CPUS);
>>   
>> @@ -767,12 +768,14 @@ int svg_build_topology_map(struct perf_env *env)
>>   
>>   	if (!t.sib_core || !t.sib_thr) {
>>   		fprintf(stderr, "topology: no memory\n");
>> +		ret = -1;
>>   		goto exit;
>>   	}
>>   
>>   	for (i = 0; i < env->nr_sibling_cores; i++) {
>>   		if (str_to_bitmap(sib_core, &t.sib_core[i], nr_cpus)) {
>>   			fprintf(stderr, "topology: can't parse siblings map\n");
>> +			ret = -1;
>>   			goto exit;
>>   		}
>>   
>> @@ -782,6 +785,7 @@ int svg_build_topology_map(struct perf_env *env)
>>   	for (i = 0; i < env->nr_sibling_threads; i++) {
>>   		if (str_to_bitmap(sib_thr, &t.sib_thr[i], nr_cpus)) {
>>   			fprintf(stderr, "topology: can't parse siblings map\n");
>> +			ret = -1;
>>   			goto exit;
>>   		}
>>   
>> @@ -791,6 +795,7 @@ int svg_build_topology_map(struct perf_env *env)
>>   	topology_map = malloc(sizeof(int) * nr_cpus);
>>   	if (!topology_map) {
>>   		fprintf(stderr, "topology: no memory\n");
>> +		ret = -1;
>>   		goto exit;
>>   	}
>>   
>> @@ -798,12 +803,11 @@ int svg_build_topology_map(struct perf_env *env)
>>   		topology_map[i] = -1;
>>   
>>   	scan_core_topology(topology_map, &t, nr_cpus);
>> -
>> -	return 0;
> ... as you'll set it to zero here :-)


Thank you for your comments. I will fix it.

Thanks,

Li Bin


>> +	ret = 0;
>>   
>>   exit:
>>   	zfree(&t.sib_core);
>>   	zfree(&t.sib_thr);
>>   
>> -	return -1;
>> +	return ret;
>>   }
>> -- 
>> 2.17.1
>>