Bug 111219 - REGRESSION (r125809): CFStrings created via StringImpl::createCFString() might reference freed memory when Objective-C garbage collection is enabled
Summary: REGRESSION (r125809): CFStrings created via StringImpl::createCFString() mig...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Andy Estes
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2013-03-01 14:08 PST by Andy Estes
Modified: 2013-03-01 14:41 PST (History)
1 user (show)

See Also:


Attachments
Patch (3.39 KB, patch)
2013-03-01 14:18 PST, Andy Estes
benjamin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andy Estes 2013-03-01 14:08:24 PST
CFStrings created via StringImpl::createCFString()  might reference freed memory when Objective-C garbage collection is enabled
Comment 1 Andy Estes 2013-03-01 14:15:19 PST
<rdar://problem/12265868>
Comment 2 Andy Estes 2013-03-01 14:18:00 PST
Created attachment 191035 [details]
Patch
Comment 3 Benjamin Poulain 2013-03-01 14:24:39 PST
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
Comment 4 Benjamin Poulain 2013-03-01 14:24:59 PST
+ remove the static as asked on IRC :)
Comment 5 Benjamin Poulain 2013-03-01 14:26:51 PST
oh, + "inline" for static bool garbageCollectionEnabled() since the static can go away :)
Comment 6 Andy Estes 2013-03-01 14:41:40 PST
Committed r144507: <http://trac.webkit.org/changeset/144507>