[swift-corelibs-dev] Adding retain to CFCalendar.c - SR2879

[Tony Parker]

Hi Alex,

Thanks for digging into this. This seems like a correct solution for now.

I think there is a larger question though, which is why it’s possible to overrelease kCFEmptyString. I think we skirted the issue early in bringup of SCL-Foundation, but constant strings are supposed to be “pinned” and ref count operations on them a no-op.

- Tony

> On Oct 7, 2016, at 6:47 AM, Alex Blewitt via swift-corelibs-dev <swift-corelibs-dev at swift.org> wrote:
> 
> I'm looking at https://bugs.swift.org/browse/SR-2879 <https://bugs.swift.org/browse/SR-2879> which is exposing itself through an over-release of a constant CF string (in this case, kCFEmptyString). I don't believe it to be a Swift related problem, because Swift doesn't get into the internals of CFCalendar where the problem occurs.
> 
> The problem is that CFCalendar releases the localeID when it's deallocated:
> 
> if (calendar->_localeID) CFRelease(calendar->_localeID);
> 
> https://github.com/apple/swift-corelibs-foundation/blob/1a76e814212e781a9d50782ee24117760cfe9b48/CoreFoundation/Locale.subproj/CFCalendar.c#L54 <https://github.com/apple/swift-corelibs-foundation/blob/1a76e814212e781a9d50782ee24117760cfe9b48/CoreFoundation/Locale.subproj/CFCalendar.c#L54> 
> 
> The problem is that when the localeID is assigned, it doesn't appear to be copied or renamed:
> 
> calendar->_localeID = CFLocaleGetIdentifier(CFLocaleGetSystem());
> 
> https://github.com/apple/swift-corelibs-foundation/blob/1a76e814212e781a9d50782ee24117760cfe9b48/CoreFoundation/Locale.subproj/CFCalendar.c#L252 <https://github.com/apple/swift-corelibs-foundation/blob/1a76e814212e781a9d50782ee24117760cfe9b48/CoreFoundation/Locale.subproj/CFCalendar.c#L252> 
> https://github.com/apple/swift-corelibs-foundation/blob/1a76e814212e781a9d50782ee24117760cfe9b48/CoreFoundation/Locale.subproj/CFCalendar.c#L281 <https://github.com/apple/swift-corelibs-foundation/blob/1a76e814212e781a9d50782ee24117760cfe9b48/CoreFoundation/Locale.subproj/CFCalendar.c#L281>  
> 
> but elsewhere in the code, we do retain it:
> 
>    CFStringRef localeID = CFLocaleGetIdentifier(locale);
>     if (localeID != calendar->_localeID) {
> 	CFRelease(calendar->_localeID);
> 	CFRetain(localeID);
> 
> https://github.com/apple/swift-corelibs-foundation/blob/1a76e814212e781a9d50782ee24117760cfe9b48/CoreFoundation/Locale.subproj/CFCalendar.c#L303-L306 <https://github.com/apple/swift-corelibs-foundation/blob/1a76e814212e781a9d50782ee24117760cfe9b48/CoreFoundation/Locale.subproj/CFCalendar.c#L303-L306> 
> 
> When a locale isn't supplied, it uses the default global one, which is defined to be an empty string:
> 
> CFLocaleRef CFLocaleGetSystem(void) {
>     CFLocaleRef locale;
>     CFLocaleRef uselessLocale = NULL; //if we lose the race creating the global locale, we need to release the one we created, but we want to do it outside the lock.
>     __CFLocaleLockGlobal();
>     if (NULL == __CFLocaleSystem) {
> 	__CFLocaleUnlockGlobal();
> 	locale = CFLocaleCreate(kCFAllocatorSystemDefault, CFSTR(""));
> 	if (!locale) return NULL;
> 
> https://github.com/apple/swift-corelibs-foundation/blob/1a76e814212e781a9d50782ee24117760cfe9b48/CoreFoundation/Locale.subproj/CFLocale.c#L255-L261 <https://github.com/apple/swift-corelibs-foundation/blob/1a76e814212e781a9d50782ee24117760cfe9b48/CoreFoundation/Locale.subproj/CFLocale.c#L255-L261> 
> 
> The CFSTR("") results in a reference to kCFEmptyString, which reduces by one each time a CFCalendar is created and destroyed, leading to the (unrelated) test failures of https://github.com/apple/swift-corelibs-foundation/pull/667 <https://github.com/apple/swift-corelibs-foundation/pull/667> as documented in https://bugs.swift.org/browse/SR-2879 <https://bugs.swift.org/browse/SR-2879>
> 
> My suggestion is to insert a CFRetain when the calendar->locale is set, to balance out the CFRelease that's being performed in the deallocator. Building with this simple change and checking the retain count of kCFEmptyString verifies that it does fix the problem, although I'm open to suggestions as to improvements of where the retain takes place, if not on lines 252 and 282.
> 
>   1> import Foundation 
>   2> :p (int)swift_retainCount(&__kCFEmptyString) 
> (int) $11 = 1
>   2> _ = Calendar(identifier:.gregorian)
>   3> :p (int)swift_retainCount(&__kCFEmptyString) 
> (int) $12 = 3
>   3> _ = Calendar(identifier:.chinese)
>   4> :p (int)swift_retainCount(&__kCFEmptyString) 
> (int) $13 = 3
>   4> _ = Calendar(identifier:.hebrew)
>   5> :p (int)swift_retainCount(&__kCFEmptyString) 
> (int) $14 = 3
>   5> ^D
> 
> Alex
> _______________________________________________
> swift-corelibs-dev mailing list
> swift-corelibs-dev at swift.org
> https://lists.swift.org/mailman/listinfo/swift-corelibs-dev

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.swift.org/pipermail/swift-corelibs-dev/attachments/20161007/e2df7b8f/attachment.html>