2024-02-05 10:51:56

by Yang Jihong

Subject: [PATCH 2/5] perf sched: Fix memory leak in perf_sched__map()

perf_sched__map() needs to free memory of map_cpus, color_pids and
color_cpus in normal path and rollback allocated memory in error path.

Signed-off-by: Yang Jihong <[email protected]>
---
tools/perf/builtin-sched.c | 41 ++++++++++++++++++++++++--------------
1 file changed, 26 insertions(+), 15 deletions(-)

diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
index 08dec557e6be..26dbfa4aab61 100644
--- a/tools/perf/builtin-sched.c
+++ b/tools/perf/builtin-sched.c
@@ -3208,8 +3208,6 @@ static int perf_sched__lat(struct perf_sched *sched)

static int setup_map_cpus(struct perf_sched *sched)
{
- struct perf_cpu_map *map;
-
sched->max_cpu.cpu = sysconf(_SC_NPROCESSORS_CONF);

if (sched->map.comp) {
@@ -3218,16 +3216,15 @@ static int setup_map_cpus(struct perf_sched *sched)
return -1;
}

- if (!sched->map.cpus_str)
- return 0;
-
- map = perf_cpu_map__new(sched->map.cpus_str);
- if (!map) {
- pr_err("failed to get cpus map from %s\n", sched->map.cpus_str);
- return -1;
+ if (sched->map.cpus_str) {
+ sched->map.cpus = perf_cpu_map__new(sched->map.cpus_str);
+ if (!sched->map.cpus) {
+ pr_err("failed to get cpus map from %s\n", sched->map.cpus_str);
+ free(sched->map.comp_cpus);
+ return -1;
+ }
}

- sched->map.cpus = map;
return 0;
}

@@ -3267,20 +3264,34 @@ static int setup_color_cpus(struct perf_sched *sched)

static int perf_sched__map(struct perf_sched *sched)
{
+ int rc = -1;
+
if (setup_map_cpus(sched))
- return -1;
+ return rc;

if (setup_color_pids(sched))
- return -1;
+ goto out_free_map_cpus;

if (setup_color_cpus(sched))
- return -1;
+ goto out_free_color_pids;

setup_pager();
if (perf_sched__read_events(sched))
- return -1;
+ goto out_free_color_cpus;
+
+ rc = 0;
print_bad_events(sched);
- return 0;
+
+out_free_color_cpus:
+ perf_cpu_map__put(sched->map.color_cpus);
+
+out_free_color_pids:
+ perf_thread_map__put(sched->map.color_pids);
+
+out_free_map_cpus:
+ free(sched->map.comp_cpus);
+ perf_cpu_map__put(sched->map.cpus);
+ return rc;
}

static int perf_sched__replay(struct perf_sched *sched)
--
2.34.1

2024-02-05 19:10:43

by Arnaldo Carvalho de Melo

Subject: Re: [PATCH 2/5] perf sched: Fix memory leak in perf_sched__map()

On Mon, Feb 05, 2024 at 10:46:13AM +0000, Yang Jihong wrote:
> +++ b/tools/perf/builtin-sched.c
> @@ -3267,20 +3264,34 @@ static int setup_color_cpus(struct perf_sched *sched)
>
> static int perf_sched__map(struct perf_sched *sched)
> {
> + int rc = -1;
> +
> if (setup_map_cpus(sched))
> - return -1;
> + return rc;
>
> if (setup_color_pids(sched))
> - return -1;
> + goto out_free_map_cpus;

I think renaming the goto labels to what they will do, dropping a
refcount, is more clear, i.e.:

goto out_put_map_cpus;

>
> if (setup_color_cpus(sched))
> - return -1;
> + goto out_free_color_pids;
>
> setup_pager();
> if (perf_sched__read_events(sched))
> - return -1;
> + goto out_free_color_cpus;
> +
> + rc = 0;
> print_bad_events(sched);
> - return 0;
> +
> +out_free_color_cpus:
> + perf_cpu_map__put(sched->map.color_cpus);
> +
> +out_free_color_pids:
> + perf_thread_map__put(sched->map.color_pids);
> +
> +out_free_map_cpus:
> + free(sched->map.comp_cpus);

Please use:

zfree(&sched->map.comp_cpus);

> + perf_cpu_map__put(sched->map.cpus);
> + return rc;
> }

2024-02-06 07:08:29

by Yang Jihong

Subject: Re: [PATCH 2/5] perf sched: Fix memory leak in perf_sched__map()

Hello,

On 2024/2/6 2:58, Arnaldo Carvalho de Melo wrote:
> On Mon, Feb 05, 2024 at 10:46:13AM +0000, Yang Jihong wrote:
>> +++ b/tools/perf/builtin-sched.c
>> @@ -3267,20 +3264,34 @@ static int setup_color_cpus(struct perf_sched *sched)
>>
>> static int perf_sched__map(struct perf_sched *sched)
>> {
>> + int rc = -1;
>> +
>> if (setup_map_cpus(sched))
>> - return -1;
>> + return rc;
>>
>> if (setup_color_pids(sched))
>> - return -1;
>> + goto out_free_map_cpus;
>
> I think renaming the goto labels to what they will do, dropping a
> refcount, is more clear, i.e.:
>
> goto out_put_map_cpus;
OK, will modify in v2.
>
>>
>> if (setup_color_cpus(sched))
>> - return -1;
>> + goto out_free_color_pids;
>>
>> setup_pager();
>> if (perf_sched__read_events(sched))
>> - return -1;
>> + goto out_free_color_cpus;
>> +
>> + rc = 0;
>> print_bad_events(sched);
>> - return 0;
>> +
>> +out_free_color_cpus:
>> + perf_cpu_map__put(sched->map.color_cpus);
>> +
>> +out_free_color_pids:
>> + perf_thread_map__put(sched->map.color_pids);
>> +
>> +out_free_map_cpus:
>> + free(sched->map.comp_cpus);
>
> Please use:
>
> zfree(&sched->map.comp_cpus);
>
OK, will change to use zfree in the next version.
Other patches where uses free will also be changed to zfree.

Thanks,
Yang