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

[Baesken, Matthias]

Thanks for the review .
Do I need a second  review for this one ?


Best regards, Matthias


> -----Original Message-----
> From: Weijun Wang <weijun.w...@oracle.com>
> Sent: Mittwoch, 24. Juli 2019 15:41
> To: Baesken, Matthias <matthias.baes...@sap.com>
> Cc: security-dev@openjdk.java.net; naoto.s...@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.baes...@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@openjdk.java.net
> > Cc: 'naoto.s...@oracle.com' <naoto.s...@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);