[webkit-reviews] review granted: [Bug 111219] CFStrings created via StringImpl::createCFString() might reference freed memory when Objective-C garbage collection is enabled : [Attachment 191035] Patch

[bugzilla-daemon at webkit.org]

Benjamin Poulain <benjamin at webkit.org> has granted Andy Estes
<aestes at apple.com>'s request for review:
Bug 111219: CFStrings created via StringImpl::createCFString()	might reference
freed memory when Objective-C garbage collection is enabled
https://bugs.webkit.org/show_bug.cgi?id=111219

Attachment 191035: Patch
https://bugs.webkit.org/attachment.cgi?id=191035&action=review

------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
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