RFR : [XS] 8228578: fix CFData object leak in macosx KeystoreImpl.m

[Weijun Wang]

I don't think so.

--Max

> On Jul 25, 2019, at 5:09 PM, Baesken, Matthias <matthias.baesken at sap.com> wrote:
> 
> Thanks for the review .
> Do I need a second  review for this one ?
> 
> 
> Best regards, Matthias
> 
> 
>> -----Original Message-----
>> From: Weijun Wang <weijun.wang at oracle.com>
>> Sent: Mittwoch, 24. Juli 2019 15:41
>> To: Baesken, Matthias <matthias.baesken at sap.com>
>> Cc: security-dev at openjdk.java.net; naoto.sato at oracle.com
>> Subject: Re: RFR : [XS] 8228578: fix CFData object leak in macosx
>> KeystoreImpl.m
>> 
>> This looks fine to me.
>> 
>> You might want to add a noreg-* label to the bug. Maybe noreg-cleanup?
>> 
>> Thanks,
>> Max
>> 
>>> On Jul 24, 2019, at 7:35 PM, Baesken, Matthias
>> <matthias.baesken at sap.com> wrote:
>>> 
>>> Hello,  here is the webrev for easier review .
>>> 
>>> Bug/webrev  (after my webrev creation works again) :
>>> 
>>> 
>>> http://cr.openjdk.java.net/~mbaesken/webrevs/8228578.0/
>>> 
>>> https://bugs.openjdk.java.net/browse/JDK-8228578
>>> 
>>> Best regards, Matthias
>>> 
>>> From: Baesken, Matthias
>>> Sent: Mittwoch, 24. Juli 2019 13:17
>>> To: security-dev at openjdk.java.net
>>> Cc: 'naoto.sato at oracle.com' <naoto.sato at oracle.com>
>>> Subject: RFR : [XS] 8228578: fix CFData object leak in macosx
>> KeystoreImpl.m
>>> 
>>> Hello,   please  review the  following small  patch .
>>> 
>>> In KeystoreImpl.m   we call CFDataCreate at one place.  According to
>>> 
>>> https://developer.apple.com/documentation/corefoundation/1542359-
>> cfdatacreate?language=objc
>>> 
>>> the return value of CFDataCreate is :    "A new CFData object, or NULL if
>> there was a problem creating the object. Ownership follows the The Create
>> Rule."
>>> 
>>> Following the "Create Rule" ,  we have to release the return value to avoid
>> leaks.
>>> Or do I miss something ?
>>> 
>>> 
>>> Bug /  (no  webrev currently  because  I have some technical issues at the
>> moment with webrev creation, getting connection reset by peer for some
>> reason )
>>> 
>>> https://bugs.openjdk.java.net/browse/JDK-8228578
>>> 
>>> patch is attached, and change also  below .
>>> 
>>> Thanks, Matthias
>>> 
>>> 
>>> 
>>> Change :
>>> 
>>> # HG changeset patch
>>> # Parent 042dfb697624926507649a4a8ad17a5e6730ba04
>>> 8228578: fix CFData object leak in macosx KeystoreImpl.m
>>> 
>>> diff -r 042dfb697624 -r 9f43fea81900
>> src/java.base/macosx/native/libosxsecurity/KeystoreImpl.m
>>> --- a/src/java.base/macosx/native/libosxsecurity/KeystoreImpl.m           Tue
>> Jul 23 20:03:03 2019 -0700
>>> +++ b/src/java.base/macosx/native/libosxsecurity/KeystoreImpl.m
>> Wed Jul 24 12:36:12 2019 +0200
>>> @@ -1,5 +1,5 @@
>>> /*
>>> - * Copyright (c) 2011, 2018, Oracle and/or its affiliates. All rights reserved.
>>> + * Copyright (c) 2011, 2019, Oracle and/or its affiliates. All rights reserved.
>>>  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
>>>  *
>>>  * This code is free software; you can redistribute it and/or modify it
>>> @@ -562,6 +562,9 @@
>>> 
>>>     err = SecKeychainItemImport(cfDataToImport, NULL, &dataFormat,
>> NULL,
>>>                                 0, &paramBlock, defaultKeychain, &createdItems);
>>> +    if (cfDataToImport != NULL) {
>>> +        CFRelease(cfDataToImport);
>>> +    }
>>> 
>>>     if (err == noErr) {
>>>         SecKeychainItemRef anItem =
>> (SecKeychainItemRef)CFArrayGetValueAtIndex(createdItems, 0);
>