CFDataCreate/CFRelease - was : RE: java_props_macosx.c : CFLocaleCopyCurrent() needs CFRelease ?

[Baesken, Matthias]

Hello ,    I  checked  a number of other CF*Create*  /  CF*Copy*  methods (and corresp. CFRelease calls)   we have in java.base   and almost all  looked good to me .
However this one looked  bad to me :

https://hg.openjdk.java.net/jdk/jdk/file/aaa83519e723/src/java.base/macosx/native/libosxsecurity/KeystoreImpl.m#l522


    CFDataRef cfDataToImport = CFDataCreate(kCFAllocatorDefault, (UInt8 *)rawData, dataSize);
    ....

    err = SecKeychainItemImport(cfDataToImport, NULL, &dataFormat, NULL,
                                0, &paramBlock, defaultKeychain, &createdItems);


Don't  we need a CFRelease   here too    , the docu

https://developer.apple.com/documentation/corefoundation/1542359-cfdatacreate?language=objc

says the ownership of the returned object follows the Create  rule .

Do I miss something ?

Best regards, Matthias




> -----Original Message-----
> From: Baesken, Matthias
> Sent: Dienstag, 23. Juli 2019 08:43
> To: core-libs-dev at openjdk.java.net; 'naoto.sato at oracle.com'
> <naoto.sato at oracle.com>
> Subject: Re: java_props_macosx.c : CFLocaleCopyCurrent() needs CFRelease
> ?
> 
> Thanks for your input !
> 
> I opened
> 
> https://bugs.openjdk.java.net/browse/JDK-8228501
> 
> for this issue, will provide a patch .
> 
> Best regards, Matthias
> 
> 
> > Date: Mon, 22 Jul 2019 12:56:50 -0700
> > From: naoto.sato at oracle.com
> > To: core-libs-dev at openjdk.java.net
> > Subject: Re: java_props_macosx.c : CFLocaleCopyCurrent() needs
> > 	CFRelease ?
> > Message-ID: <72d41e80-82d2-44fc-dead-3fa6a653d6af at oracle.com>
> > Content-Type: text/plain; charset=utf-8; format=flowed
> >
> > Hi Matthias,
> >
> > Thanks for catching them. Yes, I believe they should be released
> > appropriately.
> >
> > Naoto
> >
> > On 7/22/19 4:01 AM, Baesken, Matthias wrote:
> > > Hello  , maybe  someone  with more  OSX dev knowledge could comment
> > on this .
> > > If I understand it correctly ,   the  OSX  Core Foundation Ownership Policy :
> > >
> > >
> >
> https://developer.apple.com/library/archive/documentation/CoreFoundatio
> >
> n/Conceptual/CFMemoryMgmt/Concepts/Ownership.html#//apple_ref/doc
> > /uid/20001148-103029
> > >
> > > says that   "Object-duplication functions that have "Copy" embedded in
> the
> > name."    (like  CFLocaleCopyCurrent )     need  to
> > > relinquish ownership (using
> >
> CFRelease<https://developer.apple.com/documentation/corefoundation/1
> > 521153-cfrelease>) when you have finished with it.
> > >
> > > Should we better add then  CFRelease  to the  2   CFLocaleCopyCurrent
> > usages  in  src/java.base/macosx/native/libjava/java_props_macosx.c
> > (coding below) ?
> > > Or do I miss something ?
> > >
> > > Thanks , Matthias
> > >
> > >
> > >
> > > --- a/src/java.base/macosx/native/libjava/java_props_macosx.c   Fri Jul
> 19
> > 10:18:48 2019 +0200
> > > +++ b/src/java.base/macosx/native/libjava/java_props_macosx.c   Mon
> Jul
> > 22 12:47:21 2019 +0200
> > > @@ -91,18 +91,22 @@
> > >               if (hyphenPos == NULL || // languageString contains ISO639 only,
> > e.g., "en"
> > >                   languageString + langStrLen - hyphenPos == 5) { // ISO639-
> > ScriptCode, e.g., "en-Latn"
> > > -
> > CFStringGetCString(CFLocaleGetIdentifier(CFLocaleCopyCurrent()),
> > > -                               localeString, LOCALEIDLENGTH,
> > CFStringGetSystemEncoding());
> > > -                char *underscorePos = strrchr(localeString, '_');
> > > -                char *region = NULL;
> > > +                CFLocaleRef cflocale = CFLocaleCopyCurrent();
> > > +                if (cflocale != NULL) {
> > > +                    CFStringGetCString(CFLocaleGetIdentifier(cflocale),
> > > +                                   localeString, LOCALEIDLENGTH,
> > CFStringGetSystemEncoding());
> > > +                    char *underscorePos = strrchr(localeString, '_');
> > > +                    char *region = NULL;
> > > -                if (underscorePos != NULL) {
> > > -                    region = underscorePos + 1;
> > > -                }
> > > +                    if (underscorePos != NULL) {
> > > +                        region = underscorePos + 1;
> > > +                    }
> > > -                if (region != NULL) {
> > > -                    strcat(languageString, "-");
> > > -                    strcat(languageString, region);
> > > +                    if (region != NULL) {
> > > +                        strcat(languageString, "-");
> > > +                        strcat(languageString, region);
> > > +                    }
> > > +                    CFRelease(cflocale);
> > >                   }
> > >               }
> > >
> > > @@ -112,12 +116,18 @@
> > >       default:
> > >           {
> > > -            if
> > (!CFStringGetCString(CFLocaleGetIdentifier(CFLocaleCopyCurrent()),
> > > -                                    localeString, LOCALEIDLENGTH,
> > CFStringGetSystemEncoding())) {
> > > +            CFLocaleRef cflocale = CFLocaleCopyCurrent();
> > > +            if (cflocale != NULL) {
> > > +                if (!CFStringGetCString(CFLocaleGetIdentifier(cflocale),
> > > +                                        localeString, LOCALEIDLENGTH,
> > CFStringGetSystemEncoding())) {
> > > +                    return NULL;
> > > +                }
> > > +
> > > +                retVal = localeString;
> > > +                CFRelease(cflocale);
> > > +            } else {
> > >                   return NULL;
> > >               }
> > > -
> > > -            retVal = localeString;
> > >           }
> > >           break;
> > >       }
> > >
> > >
> >
> >