linux-kernel - Re: [PATCH] perf jevents: Fix resource leak in process

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <5dcbcd6f-3789-69e1-b0c1-33416aa0790d@huawei.com>
Date:   Wed, 16 Oct 2019 13:08:48 +0100
From:   John Garry <john.garry@...wei.com>
To:     Yunfeng Ye <yeyunfeng@...wei.com>, <peterz@...radead.org>,
        <mingo@...hat.com>, <acme@...nel.org>, <mark.rutland@....com>,
        <alexander.shishkin@...ux.intel.com>, <jolsa@...hat.com>,
        <namhyung@...nel.org>, <ak@...ux.intel.com>,
        <lukemujica@...gle.com>, <kan.liang@...ux.intel.com>,
        <yuzenghui@...wei.com>
CC:     <linux-kernel@...r.kernel.org>, <hushiyuan@...wei.com>,
        <linfeilong@...wei.com>
Subject: Re: [PATCH] perf jevents: Fix resource leak in process_mapfile()

>>> +            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;
>>>  }
>>>
>>>  /*
>>>
>>
>>
>>
>> .
>>
>
>
> .
>