Bug 111219 – REGRESSION (r125809): CFStrings created via StringImpl::createCFString() might reference freed memory when Objective-C garbage collection is enabled

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Andy Estes]

CFStrings created via StringImpl::createCFString()  might reference freed memory when Objective-C garbage collection is enabled

[Andy Estes]

<rdar://problem/12265868>

[Andy Estes]

Created attachment 191035 [details]
Patch

[Benjamin Poulain]

Comment on attachment 191035 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=191035&action=review

sorry about that!

> Source/WebCore/ChangeLog:17
> +        However, custom allocators aren't supported when Objective-C garbage
> +        collection is enabled, so in this case we use the default CF allocator.
> +        Since we can't guarantee the lifetime of the StringImpl in this case,
> +        we should just fall back to copying the string.

You can add rXXXX stupidly broke this by not checking if StringWrapperCFAllocator::allocator returns something.

> Source/WebCore/platform/text/cf/StringImplCF.cpp:38
> +#if PLATFORM(MAC)

&& !PLATFORM(IOS)

> Source/WebCore/platform/text/cf/StringImplCF.cpp:-123
> -#if PLATFORM(MAC)
>          // Since garbage collection isn't compatible with custom allocators, don't use this at all when garbage collection is active.
> -        if (objc_collectingEnabled())
> +        if (garbageCollectionEnabled())
>              return 0;
> -#endif

You should get rid of this and just ASSERT(!garbageCollectionEnabled)

The comment:
   // Since garbage collection isn't compatible with custom allocators, don't use this at all when garbage collection is active.
should be put before garbageCollectionEnabled() in StringImpl::createCFString() IMHO

[Benjamin Poulain]

+ remove the static as asked on IRC :)

[Benjamin Poulain]

oh, + "inline" for static bool garbageCollectionEnabled() since the static can go away :)

[Andy Estes]

Committed r144507: <http://trac.webkit.org/changeset/144507>