2019-10-16 16:09:39

by Yunfeng Ye

Subject: [PATCH v2] perf jevents: Fix resource leak in process_mapfile() and main()

There are memory leaks and file descriptor resource leaks in
process_mapfile() and main().

Fix this by adding free(), fclose() and free_arch_std_events()
on the error paths.

Fixes: 80eeb67fe577 ("perf jevents: Program to convert JSON file")
Fixes: 3f056b66647b ("perf jevents: Make build fail on JSON parse error")
Fixes: e9d32c1bf0cd ("perf vendor events: Add support for arch standard events")
Signed-off-by: Yunfeng Ye <[email protected]>
---
v1 -> v2:
- add free(eventsfp) to fix eventsfp resource leaks
- add free_arch_std_events() on the error path

tools/perf/pmu-events/jevents.c | 13 +++++++++++--
1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/tools/perf/pmu-events/jevents.c b/tools/perf/pmu-events/jevents.c
index e2837260ca4d..99e3fd04a5cb 100644
--- a/tools/perf/pmu-events/jevents.c
+++ b/tools/perf/pmu-events/jevents.c
@@ -758,6 +758,7 @@ static int process_mapfile(FILE *outfp, char *fpath)
char *line, *p;
int line_num;
char *tblname;
+ int ret = 0;

pr_info("%s: Processing mapfile %s\n", prog, fpath);

@@ -769,6 +770,7 @@ static int process_mapfile(FILE *outfp, char *fpath)
if (!mapfp) {
pr_info("%s: Error %s opening %s\n", prog, strerror(errno),
fpath);
+ free(line);
return -1;
}

@@ -795,7 +797,8 @@ static int process_mapfile(FILE *outfp, char *fpath)
/* TODO Deal with lines longer than 16K */
pr_info("%s: Mapfile %s: line %d too long, aborting\n",
prog, fpath, line_num);
- return -1;
+ ret = -1;
+ goto out;
}
line[strlen(line)-1] = '\0';

@@ -825,7 +828,9 @@ static int process_mapfile(FILE *outfp, char *fpath)

out:
print_mapping_table_suffix(outfp);
- return 0;
+ fclose(mapfp);
+ free(line);
+ return ret;
}

/*
@@ -1122,6 +1127,7 @@ int main(int argc, char *argv[])
goto empty_map;
} else if (rc < 0) {
/* Make build fail */
+ fclose(eventsfp);
free_arch_std_events();
return 1;
} else if (rc) {
@@ -1134,6 +1140,7 @@ int main(int argc, char *argv[])
goto empty_map;
} else if (rc < 0) {
/* Make build fail */
+ fclose(eventsfp);
free_arch_std_events();
return 1;
} else if (rc) {
@@ -1151,6 +1158,8 @@ int main(int argc, char *argv[])
if (process_mapfile(eventsfp, mapfile)) {
pr_info("%s: Error processing mapfile %s\n", prog, mapfile);
/* Make build fail */
+ fclose(eventsfp);
+ free_arch_std_events();
return 1;
}

--
2.7.4.3

2019-10-16 16:22:42

by Arnaldo Carvalho de Melo

Subject: Re: [PATCH v2] perf jevents: Fix resource leak in process_mapfile() and main()

Em Wed, Oct 16, 2019 at 09:50:17PM +0800, Yunfeng Ye escreveu:
> There are memory leaks and file descriptor resource leaks in
> process_mapfile() and main().
>
> Fix this by adding free(), fclose() and free_arch_std_events()
> on the error paths.
>
> Fixes: 80eeb67fe577 ("perf jevents: Program to convert JSON file")
> Fixes: 3f056b66647b ("perf jevents: Make build fail on JSON parse error")
> Fixes: e9d32c1bf0cd ("perf vendor events: Add support for arch standard events")

Nice, thanks for adding the fixes line, I looked at those three patches
and indeed they were leaky, thanks for the fixes, we shouldn't have
those leaks even if that, for now, makes the tool to end anyway.

- Arnaldo

> Signed-off-by: Yunfeng Ye <[email protected]>
> ---
> v1 -> v2:
> - add free(eventsfp) to fix eventsfp resource leaks
> - add free_arch_std_events() on the error path
>
> tools/perf/pmu-events/jevents.c | 13 +++++++++++--
> 1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/tools/perf/pmu-events/jevents.c b/tools/perf/pmu-events/jevents.c
> index e2837260ca4d..99e3fd04a5cb 100644
> --- a/tools/perf/pmu-events/jevents.c
> +++ b/tools/perf/pmu-events/jevents.c
> @@ -758,6 +758,7 @@ static int process_mapfile(FILE *outfp, char *fpath)
> char *line, *p;
> int line_num;
> char *tblname;
> + int ret = 0;
>
> pr_info("%s: Processing mapfile %s\n", prog, fpath);
>
> @@ -769,6 +770,7 @@ static int process_mapfile(FILE *outfp, char *fpath)
> if (!mapfp) {
> pr_info("%s: Error %s opening %s\n", prog, strerror(errno),
> fpath);
> + free(line);
> return -1;
> }
>
> @@ -795,7 +797,8 @@ static int process_mapfile(FILE *outfp, char *fpath)
> /* TODO Deal with lines longer than 16K */
> pr_info("%s: Mapfile %s: line %d too long, aborting\n",
> prog, fpath, line_num);
> - return -1;
> + ret = -1;
> + goto out;
> }
> line[strlen(line)-1] = '\0';
>
> @@ -825,7 +828,9 @@ static int process_mapfile(FILE *outfp, char *fpath)
>
> out:
> print_mapping_table_suffix(outfp);
> - return 0;
> + fclose(mapfp);
> + free(line);
> + return ret;
> }
>
> /*
> @@ -1122,6 +1127,7 @@ int main(int argc, char *argv[])
> goto empty_map;
> } else if (rc < 0) {
> /* Make build fail */
> + fclose(eventsfp);
> free_arch_std_events();
> return 1;
> } else if (rc) {
> @@ -1134,6 +1140,7 @@ int main(int argc, char *argv[])
> goto empty_map;
> } else if (rc < 0) {
> /* Make build fail */
> + fclose(eventsfp);
> free_arch_std_events();
> return 1;
> } else if (rc) {
> @@ -1151,6 +1158,8 @@ int main(int argc, char *argv[])
> if (process_mapfile(eventsfp, mapfile)) {
> pr_info("%s: Error processing mapfile %s\n", prog, mapfile);
> /* Make build fail */
> + fclose(eventsfp);
> + free_arch_std_events();
> return 1;
> }
>
> --
> 2.7.4.3

--

- Arnaldo

2019-10-23 12:33:52

by Yunfeng Ye

Subject: Re: [PATCH v2] perf jevents: Fix resource leak in process_mapfile() and main()

On 2019/10/16 22:25, Arnaldo Carvalho de Melo wrote:
> Em Wed, Oct 16, 2019 at 09:50:17PM +0800, Yunfeng Ye escreveu:
>> There are memory leaks and file descriptor resource leaks in
>> process_mapfile() and main().
>>
>> Fix this by adding free(), fclose() and free_arch_std_events()
>> on the error paths.
>>
>> Fixes: 80eeb67fe577 ("perf jevents: Program to convert JSON file")
>> Fixes: 3f056b66647b ("perf jevents: Make build fail on JSON parse error")
>> Fixes: e9d32c1bf0cd ("perf vendor events: Add support for arch standard events")
>
> Nice, thanks for adding the fixes line, I looked at those three patches
> and indeed they were leaky, thanks for the fixes, we shouldn't have
> those leaks even if that, for now, makes the tool to end anyway.
>
The other 3 patchs have been applied, is this patch applied ? thanks.

> - Arnaldo
>
>> Signed-off-by: Yunfeng Ye <[email protected]>
>> ---
>> v1 -> v2:
>> - add free(eventsfp) to fix eventsfp resource leaks
>> - add free_arch_std_events() on the error path
>>
>> tools/perf/pmu-events/jevents.c | 13 +++++++++++--
>> 1 file changed, 11 insertions(+), 2 deletions(-)
>>
>> diff --git a/tools/perf/pmu-events/jevents.c b/tools/perf/pmu-events/jevents.c
>> index e2837260ca4d..99e3fd04a5cb 100644
>> --- a/tools/perf/pmu-events/jevents.c
>> +++ b/tools/perf/pmu-events/jevents.c
>> @@ -758,6 +758,7 @@ static int process_mapfile(FILE *outfp, char *fpath)
>> char *line, *p;
>> int line_num;
>> char *tblname;
>> + int ret = 0;
>>
>> pr_info("%s: Processing mapfile %s\n", prog, fpath);
>>
>> @@ -769,6 +770,7 @@ static int process_mapfile(FILE *outfp, char *fpath)
>> if (!mapfp) {
>> pr_info("%s: Error %s opening %s\n", prog, strerror(errno),
>> fpath);
>> + free(line);
>> return -1;
>> }
>>
>> @@ -795,7 +797,8 @@ static int process_mapfile(FILE *outfp, char *fpath)
>> /* TODO Deal with lines longer than 16K */
>> pr_info("%s: Mapfile %s: line %d too long, aborting\n",
>> prog, fpath, line_num);
>> - return -1;
>> + ret = -1;
>> + goto out;
>> }
>> line[strlen(line)-1] = '\0';
>>
>> @@ -825,7 +828,9 @@ static int process_mapfile(FILE *outfp, char *fpath)
>>
>> out:
>> print_mapping_table_suffix(outfp);
>> - return 0;
>> + fclose(mapfp);
>> + free(line);
>> + return ret;
>> }
>>
>> /*
>> @@ -1122,6 +1127,7 @@ int main(int argc, char *argv[])
>> goto empty_map;
>> } else if (rc < 0) {
>> /* Make build fail */
>> + fclose(eventsfp);
>> free_arch_std_events();
>> return 1;
>> } else if (rc) {
>> @@ -1134,6 +1140,7 @@ int main(int argc, char *argv[])
>> goto empty_map;
>> } else if (rc < 0) {
>> /* Make build fail */
>> + fclose(eventsfp);
>> free_arch_std_events();
>> return 1;
>> } else if (rc) {
>> @@ -1151,6 +1158,8 @@ int main(int argc, char *argv[])
>> if (process_mapfile(eventsfp, mapfile)) {
>> pr_info("%s: Error processing mapfile %s\n", prog, mapfile);
>> /* Make build fail */
>> + fclose(eventsfp);
>> + free_arch_std_events();
>> return 1;
>> }
>>
>> --
>> 2.7.4.3
>

2019-10-25 13:57:40

by Arnaldo Carvalho de Melo

Subject: Re: [PATCH v2] perf jevents: Fix resource leak in process_mapfile() and main()

Em Wed, Oct 23, 2019 at 04:22:25PM +0800, Yunfeng Ye escreveu:
>
>
> On 2019/10/16 22:25, Arnaldo Carvalho de Melo wrote:
> > Em Wed, Oct 16, 2019 at 09:50:17PM +0800, Yunfeng Ye escreveu:
> >> There are memory leaks and file descriptor resource leaks in
> >> process_mapfile() and main().
> >>
> >> Fix this by adding free(), fclose() and free_arch_std_events()
> >> on the error paths.
> >>
> >> Fixes: 80eeb67fe577 ("perf jevents: Program to convert JSON file")
> >> Fixes: 3f056b66647b ("perf jevents: Make build fail on JSON parse error")
> >> Fixes: e9d32c1bf0cd ("perf vendor events: Add support for arch standard events")
> >
> > Nice, thanks for adding the fixes line, I looked at those three patches
> > and indeed they were leaky, thanks for the fixes, we shouldn't have
> > those leaks even if that, for now, makes the tool to end anyway.
> >
> The other 3 patchs have been applied, is this patch applied ? thanks.

Applied now, thanks for the reminder,

- Arnaldo

> > - Arnaldo
> >
> >> Signed-off-by: Yunfeng Ye <[email protected]>
> >> ---
> >> v1 -> v2:
> >> - add free(eventsfp) to fix eventsfp resource leaks
> >> - add free_arch_std_events() on the error path
> >>
> >> tools/perf/pmu-events/jevents.c | 13 +++++++++++--
> >> 1 file changed, 11 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/tools/perf/pmu-events/jevents.c b/tools/perf/pmu-events/jevents.c
> >> index e2837260ca4d..99e3fd04a5cb 100644
> >> --- a/tools/perf/pmu-events/jevents.c
> >> +++ b/tools/perf/pmu-events/jevents.c
> >> @@ -758,6 +758,7 @@ static int process_mapfile(FILE *outfp, char *fpath)
> >> char *line, *p;
> >> int line_num;
> >> char *tblname;
> >> + int ret = 0;
> >>
> >> pr_info("%s: Processing mapfile %s\n", prog, fpath);
> >>
> >> @@ -769,6 +770,7 @@ static int process_mapfile(FILE *outfp, char *fpath)
> >> if (!mapfp) {
> >> pr_info("%s: Error %s opening %s\n", prog, strerror(errno),
> >> fpath);
> >> + free(line);
> >> return -1;
> >> }
> >>
> >> @@ -795,7 +797,8 @@ static int process_mapfile(FILE *outfp, char *fpath)
> >> /* TODO Deal with lines longer than 16K */
> >> pr_info("%s: Mapfile %s: line %d too long, aborting\n",
> >> prog, fpath, line_num);
> >> - return -1;
> >> + ret = -1;
> >> + goto out;
> >> }
> >> line[strlen(line)-1] = '\0';
> >>
> >> @@ -825,7 +828,9 @@ static int process_mapfile(FILE *outfp, char *fpath)
> >>
> >> out:
> >> print_mapping_table_suffix(outfp);
> >> - return 0;
> >> + fclose(mapfp);
> >> + free(line);
> >> + return ret;
> >> }
> >>
> >> /*
> >> @@ -1122,6 +1127,7 @@ int main(int argc, char *argv[])
> >> goto empty_map;
> >> } else if (rc < 0) {
> >> /* Make build fail */
> >> + fclose(eventsfp);
> >> free_arch_std_events();
> >> return 1;
> >> } else if (rc) {
> >> @@ -1134,6 +1140,7 @@ int main(int argc, char *argv[])
> >> goto empty_map;
> >> } else if (rc < 0) {
> >> /* Make build fail */
> >> + fclose(eventsfp);
> >> free_arch_std_events();
> >> return 1;
> >> } else if (rc) {
> >> @@ -1151,6 +1158,8 @@ int main(int argc, char *argv[])
> >> if (process_mapfile(eventsfp, mapfile)) {
> >> pr_info("%s: Error processing mapfile %s\n", prog, mapfile);
> >> /* Make build fail */
> >> + fclose(eventsfp);
> >> + free_arch_std_events();
> >> return 1;
> >> }
> >>
> >> --
> >> 2.7.4.3
> >

--

- Arnaldo

2019-11-12 11:20:11

by tip-bot2 for Alexey Makhalov

Subject: [tip: perf/core] perf jevents: Fix resource leak in process_mapfile() and main()

The following commit has been merged into the perf/core branch of tip:

Commit-ID: 1785fbb73896dbd9d27a406f0d73047df42db710
Gitweb: https://git.kernel.org/tip/1785fbb73896dbd9d27a406f0d73047df42db710
Author: Yunfeng Ye <[email protected]>
AuthorDate: Wed, 16 Oct 2019 21:50:17 +08:00
Committer: Arnaldo Carvalho de Melo <[email protected]>
CommitterDate: Wed, 06 Nov 2019 15:43:06 -03:00

perf jevents: Fix resource leak in process_mapfile() and main()

There are memory leaks and file descriptor resource leaks in
process_mapfile() and main().

Fix this by adding free(), fclose() and free_arch_std_events() on the
error paths.

Fixes: 80eeb67fe577 ("perf jevents: Program to convert JSON file")
Fixes: 3f056b66647b ("perf jevents: Make build fail on JSON parse error")
Fixes: e9d32c1bf0cd ("perf vendor events: Add support for arch standard events")
Signed-off-by: Yunfeng Ye <[email protected]>
Cc: Alexander Shishkin <[email protected]>
Cc: Andi Kleen <[email protected]>
Cc: Feilong Lin <[email protected]>
Cc: Hu Shiyuan <[email protected]>
Cc: Jiri Olsa <[email protected]>
Cc: John Garry <[email protected]>
Cc: Kan Liang <[email protected]>
Cc: Luke Mujica <[email protected]>
Cc: Mark Rutland <[email protected]>
Cc: Namhyung Kim <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Zenghui Yu <[email protected]>
Link: http://lore.kernel.org/lkml/[email protected]
Signed-off-by: Arnaldo Carvalho de Melo <[email protected]>
---
tools/perf/pmu-events/jevents.c | 13 +++++++++++--
1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/tools/perf/pmu-events/jevents.c b/tools/perf/pmu-events/jevents.c
index 7d69727..079c77b 100644
--- a/tools/perf/pmu-events/jevents.c
+++ b/tools/perf/pmu-events/jevents.c
@@ -772,6 +772,7 @@ static int process_mapfile(FILE *outfp, char *fpath)
char *line, *p;
int line_num;
char *tblname;
+ int ret = 0;

pr_info("%s: Processing mapfile %s\n", prog, fpath);

@@ -783,6 +784,7 @@ static int process_mapfile(FILE *outfp, char *fpath)
if (!mapfp) {
pr_info("%s: Error %s opening %s\n", prog, strerror(errno),
fpath);
+ free(line);
return -1;
}

@@ -809,7 +811,8 @@ static int process_mapfile(FILE *outfp, char *fpath)
/* TODO Deal with lines longer than 16K */
pr_info("%s: Mapfile %s: line %d too long, aborting\n",
prog, fpath, line_num);
- return -1;
+ ret = -1;
+ goto out;
}
line[strlen(line)-1] = '\0';

@@ -839,7 +842,9 @@ static int process_mapfile(FILE *outfp, char *fpath)

out:
print_mapping_table_suffix(outfp);
- return 0;
+ fclose(mapfp);
+ free(line);
+ return ret;
}

/*
@@ -1136,6 +1141,7 @@ int main(int argc, char *argv[])
goto empty_map;
} else if (rc < 0) {
/* Make build fail */
+ fclose(eventsfp);
free_arch_std_events();
return 1;
} else if (rc) {
@@ -1148,6 +1154,7 @@ int main(int argc, char *argv[])
goto empty_map;
} else if (rc < 0) {
/* Make build fail */
+ fclose(eventsfp);
free_arch_std_events();
return 1;
} else if (rc) {
@@ -1165,6 +1172,8 @@ int main(int argc, char *argv[])
if (process_mapfile(eventsfp, mapfile)) {
pr_info("%s: Error processing mapfile %s\n", prog, mapfile);
/* Make build fail */
+ fclose(eventsfp);
+ free_arch_std_events();
return 1;
}