David, Jerry, thank you! On Wed, Feb 24, 2016 at 12:57 AM, David Holmes <david.hol...@oracle.com> wrote:
> On 24/02/2016 12:37 AM, Thomas Stüfe wrote: > >> Hi David, >> >> thanks for the review! >> >> new webrev: >> >> http://cr.openjdk.java.net/~stuefe/webrevs/8150379-fix-leaks-perfMemory_windows_cpp/webrev.01/webrev/ >> > > Looks good. I can sponsor this for you. > > comments inline. >> >> On Tue, Feb 23, 2016 at 2:24 AM, David Holmes <david.hol...@oracle.com >> <mailto:david.hol...@oracle.com>> wrote: >> >> Hi Thomas, >> >> On 23/02/2016 12:43 AM, Thomas Stüfe wrote: >> >> Hi all, >> >> please take a look at this small fix for two leaks >> in perfMemory_windows.cpp. >> >> bug report: https://bugs.openjdk.java.net/browse/JDK-8150379 >> >> webrev: >> >> http://cr.openjdk.java.net/~stuefe/webrevs/8150379-fix-leaks-perfMemory_windows_cpp/webrev.00/webrev/ >> >> >> Fixes look good, but there seems to be additional leakage here: >> >> 1437 // get the name of the user associated with this process >> 1438 char* user = get_user_name(); >> 1439 >> 1440 if (user == NULL) { >> 1441 return NULL; >> 1442 } >> 1443 >> 1444 // construct the name of the user specific temporary directory >> 1445 char* dirname = get_user_tmp_dir(user); >> 1446 >> 1447 // check that the file system is secure - i.e. it supports >> ACLs. >> 1448 if (!is_filesystem_secure(dirname)) { >> 1449 return NULL; >> 1450 } >> >> We need to free user and dirname before returning. These allocation >> methods are crying out for some RIIA assistance. :( >> >> >> I fixed this place too and took a look at other functions returning C >> heap array but did not find anything more. I also played around with an >> RAII object, but the change got big quickly, so I decided to keep the >> change small. I agree this is a mess. If we wanted to clean up this >> > > Ok. > > coding, I would do this as a separate issue. Also, instead of doing a >> RAII wrapper, could we not just move some allocations to resource area? >> > > I'm unclear on the allocation lifecycles in this code. > > Thanks, > David > > > Thomas >> >> Thanks, >> David >> >> Thank you, >> >> Thomas >> >> >>
