Re: [PATCH] perf jevents: Fix resource leak in process_mapfile()

[John Garry]

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 <yeyunf...@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;
 }

 /*