* [PATCH] perf jevents: Fix resource leak in process_mapfile()
@ 2019-10-16 7:47 Yunfeng Ye
2019-10-16 10:11 ` Jiri Olsa
2019-10-16 10:40 ` John Garry
0 siblings, 2 replies; 6+ messages in thread
From: Yunfeng Ye @ 2019-10-16 7:47 UTC (permalink / raw)
To: peterz, mingo, acme, mark.rutland, alexander.shishkin, jolsa,
namhyung, john.garry, ak, lukemujica, kan.liang, yuzenghui
Cc: linux-kernel, hushiyuan, linfeilong
There are memory leaks and file descriptor resource leaks in
process_mapfile().
Fix this by adding free() and fclose() on the error paths.
Fixes: 80eeb67fe577 ("perf jevents: Program to convert JSON file")
Signed-off-by: Yunfeng Ye <yeyunfeng@huawei.com>
---
tools/perf/pmu-events/jevents.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/tools/perf/pmu-events/jevents.c b/tools/perf/pmu-events/jevents.c
index e2837260ca4d..6e60d4cff592 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;
}
/*
--
2.7.4.3
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] perf jevents: Fix resource leak in process_mapfile()
2019-10-16 7:47 [PATCH] perf jevents: Fix resource leak in process_mapfile() Yunfeng Ye
@ 2019-10-16 10:11 ` Jiri Olsa
2019-10-16 10:40 ` John Garry
1 sibling, 0 replies; 6+ messages in thread
From: Jiri Olsa @ 2019-10-16 10:11 UTC (permalink / raw)
To: Yunfeng Ye
Cc: peterz, mingo, acme, mark.rutland, alexander.shishkin, namhyung,
john.garry, ak, lukemujica, kan.liang, yuzenghui, linux-kernel,
hushiyuan, linfeilong
On Wed, Oct 16, 2019 at 03:47:23PM +0800, Yunfeng Ye wrote:
> There are memory leaks and file descriptor resource leaks in
> process_mapfile().
>
> Fix this by adding free() and fclose() on the error paths.
>
> Fixes: 80eeb67fe577 ("perf jevents: Program to convert JSON file")
> Signed-off-by: Yunfeng Ye <yeyunfeng@huawei.com>
Acked-by: Jiri Olsa <jolsa@kernel.org>
thanks,
jirka
> ---
> tools/perf/pmu-events/jevents.c | 9 +++++++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/tools/perf/pmu-events/jevents.c b/tools/perf/pmu-events/jevents.c
> index e2837260ca4d..6e60d4cff592 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;
> }
>
> /*
> --
> 2.7.4.3
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] perf jevents: Fix resource leak in process_mapfile()
2019-10-16 7:47 [PATCH] perf jevents: Fix resource leak in process_mapfile() Yunfeng Ye
2019-10-16 10:11 ` Jiri Olsa
@ 2019-10-16 10:40 ` John Garry
2019-10-16 10:58 ` Yunfeng Ye
1 sibling, 1 reply; 6+ messages in thread
From: John Garry @ 2019-10-16 10:40 UTC (permalink / raw)
To: Yunfeng Ye, peterz, mingo, acme, mark.rutland,
alexander.shishkin, jolsa, namhyung, ak, lukemujica, kan.liang,
yuzenghui
Cc: linux-kernel, hushiyuan, linfeilong
On 16/10/2019 08:47, Yunfeng Ye wrote:
> There are memory leaks and file descriptor resource leaks in
> process_mapfile().
>
> Fix this by adding free() and fclose() on the error paths.
>
> Fixes: 80eeb67fe577 ("perf jevents: Program to convert JSON file")
> Signed-off-by: Yunfeng Ye <yeyunfeng@huawei.com>
> ---
> tools/perf/pmu-events/jevents.c | 9 +++++++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/tools/perf/pmu-events/jevents.c b/tools/perf/pmu-events/jevents.c
> index e2837260ca4d..6e60d4cff592 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;
There's a subtle change of behaviour here, i.e. now calling
print_mapping_table_suffix(), but I don't think that it makes any
difference.
However, does outfp remain open also in this case:
main(int argc, char *argv[])
{
...
if (process_mapfile(eventsfp, mapfile)) {
pr_info("%s: Error processing mapfile %s\n", prog, mapfile);
/* Make build fail */
return 1;
}
return 0;
empty_map:
fclose(eventsfp);
...
}
I think that this code works on the basis that the program exits on any
sort of error and releases resources automatically. Having said that, it
is a good practice to tidy up.
John
> }
> 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;
> }
>
> /*
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] perf jevents: Fix resource leak in process_mapfile()
2019-10-16 10:40 ` John Garry
@ 2019-10-16 10:58 ` Yunfeng Ye
2019-10-16 12:08 ` John Garry
0 siblings, 1 reply; 6+ messages in thread
From: Yunfeng Ye @ 2019-10-16 10:58 UTC (permalink / raw)
To: John Garry, peterz, mingo, acme, mark.rutland,
alexander.shishkin, jolsa, namhyung, ak, lukemujica, kan.liang,
yuzenghui
Cc: linux-kernel, hushiyuan, linfeilong
On 2019/10/16 18:40, John Garry wrote:
> On 16/10/2019 08:47, Yunfeng Ye wrote:
>> There are memory leaks and file descriptor resource leaks in
>> process_mapfile().
>>
>> Fix this by adding free() and fclose() on the error paths.
>>
>> Fixes: 80eeb67fe577 ("perf jevents: Program to convert JSON file")
>> Signed-off-by: Yunfeng Ye <yeyunfeng@huawei.com>
>> ---
>> tools/perf/pmu-events/jevents.c | 9 +++++++--
>> 1 file changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/tools/perf/pmu-events/jevents.c b/tools/perf/pmu-events/jevents.c
>> index e2837260ca4d..6e60d4cff592 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;
>
> There's a subtle change of behaviour here, i.e. now calling print_mapping_table_suffix(), but I don't think that it makes any difference.
>
yes, I know that "goto out" will run print_mapping_table_suffix(outfp), because the error path before is done like this.
so I think it should be use "goto out" to run run print_mapping_table_suffix(outfp).
> However, does outfp remain open also in this case:
>
Because it has a comment that "Make build fail", so I am not handle the outfp, only modify the process_mapfile() function.
> main(int argc, char *argv[])
> {
> ...
>
> if (process_mapfile(eventsfp, mapfile)) {
> pr_info("%s: Error processing mapfile %s\n", prog, mapfile);
> /* Make build fail */
> return 1;
> }
>
> return 0;
>
> empty_map:
> fclose(eventsfp);
> ...
> }
>
> I think that this code works on the basis that the program exits on any sort of error and releases resources automatically. Having said that, it is a good practice to tidy up.
>
I agree with you, when program exits, it will releases resources automatically. It's just to make the program clearer and more correct.
> John
>
>> }
>> 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;
>> }
>>
>> /*
>>
>
>
>
> .
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] perf jevents: Fix resource leak in process_mapfile()
2019-10-16 10:58 ` Yunfeng Ye
@ 2019-10-16 12:08 ` John Garry
2019-10-16 12:49 ` Yunfeng Ye
0 siblings, 1 reply; 6+ messages in thread
From: John Garry @ 2019-10-16 12:08 UTC (permalink / raw)
To: Yunfeng Ye, peterz, mingo, acme, mark.rutland,
alexander.shishkin, jolsa, namhyung, ak, lukemujica, kan.liang,
yuzenghui
Cc: linux-kernel, hushiyuan, linfeilong
>>> + ret = -1;
>>> + goto out;
>>
>> There's a subtle change of behaviour here, i.e. now calling print_mapping_table_suffix(), but I don't think that it makes any difference.
>>
> yes, I know that "goto out" will run print_mapping_table_suffix(outfp), because the error path before is done like this.
> so I think it should be use "goto out" to run run print_mapping_table_suffix(outfp).
>
>> However, does outfp remain open also in this case:
>>
> Because it has a comment that "Make build fail", so I am not handle the outfp, only modify the process_mapfile() function.
>
>> main(int argc, char *argv[])
>> {
>> ...
>>
>> if (process_mapfile(eventsfp, mapfile)) {
>> pr_info("%s: Error processing mapfile %s\n", prog, mapfile);
>> /* Make build fail */
>> return 1;
>> }
>>
>> return 0;
>>
>> empty_map:
>> fclose(eventsfp);
>> ...
>> }
>>
>> I think that this code works on the basis that the program exits on any sort of error and releases resources automatically. Having said that, it is a good practice to tidy up.
>>
> I agree with you, when program exits, it will releases resources automatically. It's just to make the program clearer and more correct.
So can you make that change also (to close outfp)?
Thanks,
John
>
>> John
>>
>>> }
>>> 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;
>>> }
>>>
>>> /*
>>>
>>
>>
>>
>> .
>>
>
>
> .
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] perf jevents: Fix resource leak in process_mapfile()
2019-10-16 12:08 ` John Garry
@ 2019-10-16 12:49 ` Yunfeng Ye
0 siblings, 0 replies; 6+ messages in thread
From: Yunfeng Ye @ 2019-10-16 12:49 UTC (permalink / raw)
To: John Garry, peterz, mingo, acme, mark.rutland,
alexander.shishkin, jolsa, namhyung, ak, lukemujica, kan.liang,
yuzenghui
Cc: linux-kernel, hushiyuan, linfeilong
On 2019/10/16 20:08, John Garry wrote:
>>>> + ret = -1;
>>>> + goto out;
>>>
>>> There's a subtle change of behaviour here, i.e. now calling print_mapping_table_suffix(), but I don't think that it makes any difference.
>>>
>> yes, I know that "goto out" will run print_mapping_table_suffix(outfp), because the error path before is done like this.
>> so I think it should be use "goto out" to run run print_mapping_table_suffix(outfp).
>>
>>> However, does outfp remain open also in this case:
>>>
>> Because it has a comment that "Make build fail", so I am not handle the outfp, only modify the process_mapfile() function.
>>
>>> main(int argc, char *argv[])
>>> {
>>> ...
>>>
>>> if (process_mapfile(eventsfp, mapfile)) {
>>> pr_info("%s: Error processing mapfile %s\n", prog, mapfile);
>>> /* Make build fail */
>>> return 1;
>>> }
>>>
>>> return 0;
>>>
>>> empty_map:
>>> fclose(eventsfp);
>>> ...
>>> }
>>>
>>> I think that this code works on the basis that the program exits on any sort of error and releases resources automatically. Having said that, it is a good practice to tidy up.
>>>
>> I agree with you, when program exits, it will releases resources automatically. It's just to make the program clearer and more correct.
>
> So can you make that change also (to close outfp)?
>
ok, I will modify by adding fclose(eventsfp) on the error path, In addition, free_arch_std_events() is need too.
> Thanks,
> John
>
>>
>>> John
>>>
>>>> }
>>>> 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;
>>>> }
>>>>
>>>> /*
>>>>
>>>
>>>
>>>
>>> .
>>>
>>
>>
>> .
>>
>
>
>
> .
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-10-16 12:49 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-16 7:47 [PATCH] perf jevents: Fix resource leak in process_mapfile() Yunfeng Ye
2019-10-16 10:11 ` Jiri Olsa
2019-10-16 10:40 ` John Garry
2019-10-16 10:58 ` Yunfeng Ye
2019-10-16 12:08 ` John Garry
2019-10-16 12:49 ` Yunfeng Ye
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.