Adding retain to CFCalendar.c - SR2879


(Alex Blewitt) #1

I'm looking at 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

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#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

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

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 as documented in 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> :stuck_out_tongue: (int)swift_retainCount(&__kCFEmptyString)
(int) $11 = 1
  2> _ = Calendar(identifier:.gregorian)
  3> :stuck_out_tongue: (int)swift_retainCount(&__kCFEmptyString)
(int) $12 = 3
  3> _ = Calendar(identifier:.chinese)
  4> :stuck_out_tongue: (int)swift_retainCount(&__kCFEmptyString)
(int) $13 = 3
  4> _ = Calendar(identifier:.hebrew)
  5> :stuck_out_tongue: (int)swift_retainCount(&__kCFEmptyString)
(int) $14 = 3
  5> ^D

Alex


(Tony Parker) #2

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@swift.org> wrote:

I'm looking at 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

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#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

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

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 as documented in 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> :stuck_out_tongue: (int)swift_retainCount(&__kCFEmptyString)
(int) $11 = 1
  2> _ = Calendar(identifier:.gregorian)
  3> :stuck_out_tongue: (int)swift_retainCount(&__kCFEmptyString)
(int) $12 = 3
  3> _ = Calendar(identifier:.chinese)
  4> :stuck_out_tongue: (int)swift_retainCount(&__kCFEmptyString)
(int) $13 = 3
  4> _ = Calendar(identifier:.hebrew)
  5> :stuck_out_tongue: (int)swift_retainCount(&__kCFEmptyString)
(int) $14 = 3
  5> ^D

Alex
_______________________________________________
swift-corelibs-dev mailing list
swift-corelibs-dev@swift.org
https://lists.swift.org/mailman/listinfo/swift-corelibs-dev


(Alex Blewitt) #3

That's what I was expecting, too, but then I found this:

https://github.com/apple/swift-corelibs-foundation/blob/1a76e814212e781a9d50782ee24117760cfe9b48/CoreFoundation/Base.subproj/CFInternal.h#L387-L389

#if DEPLOYMENT_RUNTIME_SWIFT
// TODO: Pinned retain count for constants?
#define __CFSTR_RC_INIT _CF_CONSTANT_OBJECT_STRONG_RC, 0,

Alex

···

On 7 Oct 2016, at 17:51, Tony Parker <anthony.parker@apple.com> wrote:

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@swift.org <mailto:swift-corelibs-dev@swift.org>> wrote:

I'm looking at 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

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#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

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

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 as documented in 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> :stuck_out_tongue: (int)swift_retainCount(&__kCFEmptyString)
(int) $11 = 1
  2> _ = Calendar(identifier:.gregorian)
  3> :stuck_out_tongue: (int)swift_retainCount(&__kCFEmptyString)
(int) $12 = 3
  3> _ = Calendar(identifier:.chinese)
  4> :stuck_out_tongue: (int)swift_retainCount(&__kCFEmptyString)
(int) $13 = 3
  4> _ = Calendar(identifier:.hebrew)
  5> :stuck_out_tongue: (int)swift_retainCount(&__kCFEmptyString)
(int) $14 = 3
  5> ^D

Alex
_______________________________________________
swift-corelibs-dev mailing list
swift-corelibs-dev@swift.org <mailto:swift-corelibs-dev@swift.org>
https://lists.swift.org/mailman/listinfo/swift-corelibs-dev


(Tony Parker) #4

Yes, that was the “skirting of the issue”. =)

It’s likely not a huge deal in the end; since CF is an implementation detail of swift-corelibs-foundation, we just need to remember to retain/release constant strings as we would others and we would be ok. However, I suspect there are other places in CF where we get away with it on Darwin but can’t on Swift. So it would be nice to have them be consistent, for our own sanity.

- Tony

···

On Oct 7, 2016, at 10:31 AM, Alex Blewitt <alblue@apple.com> wrote:

That's what I was expecting, too, but then I found this:

https://github.com/apple/swift-corelibs-foundation/blob/1a76e814212e781a9d50782ee24117760cfe9b48/CoreFoundation/Base.subproj/CFInternal.h#L387-L389

#if DEPLOYMENT_RUNTIME_SWIFT
// TODO: Pinned retain count for constants?
#define __CFSTR_RC_INIT _CF_CONSTANT_OBJECT_STRONG_RC, 0,

Alex

On 7 Oct 2016, at 17:51, Tony Parker <anthony.parker@apple.com <mailto:anthony.parker@apple.com>> wrote:

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@swift.org <mailto:swift-corelibs-dev@swift.org>> wrote:

I'm looking at 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

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#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

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

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 as documented in 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> :stuck_out_tongue: (int)swift_retainCount(&__kCFEmptyString)
(int) $11 = 1
  2> _ = Calendar(identifier:.gregorian)
  3> :stuck_out_tongue: (int)swift_retainCount(&__kCFEmptyString)
(int) $12 = 3
  3> _ = Calendar(identifier:.chinese)
  4> :stuck_out_tongue: (int)swift_retainCount(&__kCFEmptyString)
(int) $13 = 3
  4> _ = Calendar(identifier:.hebrew)
  5> :stuck_out_tongue: (int)swift_retainCount(&__kCFEmptyString)
(int) $14 = 3
  5> ^D

Alex
_______________________________________________
swift-corelibs-dev mailing list
swift-corelibs-dev@swift.org <mailto:swift-corelibs-dev@swift.org>
https://lists.swift.org/mailman/listinfo/swift-corelibs-dev