Bug 162823 - REGRESSION (r203424): WebCore::ImageBuffer::createCompatibleBuffer() in ImageBufferCG.cpp over-releases CGColorSpaceRef objects
Summary: REGRESSION (r203424): WebCore::ImageBuffer::createCompatibleBuffer() in Image...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Images (show other bugs)
Version: Safari 10
Hardware: Unspecified Unspecified
: P1 Normal
Assignee: David Kilzer (:ddkilzer)
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2016-09-30 18:37 PDT by David Kilzer (:ddkilzer)
Modified: 2016-10-01 03:28 PDT (History)
10 users (show)

See Also:

Attachments
Patch v1 (1.97 KB, patch)
2016-09-30 18:46 PDT, David Kilzer (:ddkilzer) 		no flags Details | Formatted Diff | Diff
View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description David Kilzer (:ddkilzer) 2016-09-30 18:37:35 PDT 
WebCore::ImageBuffer::createCompatibleBuffer() in Source/WebCore/platform/graphics/cg/ImageBufferCG.cpp over-releases CGColorSpaceRef objects in two different code paths:

    RetainPtr<CGColorSpaceRef> colorSpace;
#if PLATFORM(COCOA)
    CGContextRef cgContext = context.platformContext();
    switch (CGContextGetType(cgContext)) {
    case kCGContextTypeBitmap:
        colorSpace = adoptCF(CGBitmapContextGetColorSpace(cgContext)); // BUG!
        break;
#if USE(IOSURFACE)
    case kCGContextTypeIOSurface:
        colorSpace = adoptCF(CGIOSurfaceContextGetColorSpace(cgContext)); // BUG!
        break;
#endif
    default:
        colorSpace = adoptCF(CGContextCopyDeviceColorSpace(cgContext));
    }

Neither CGBitmapContextGetColorSpace() nor CGIOSurfaceContextGetColorSpace() returns a +1 retained CGColorSpaceRef, so using adoptCF() will over-release the CGColorSpaceRef object later when RetainPtr<CGColorSpaceRef> colorSpace goes out of scope.

<rdar://problem/27723268>
Comment 1 David Kilzer (:ddkilzer) 2016-09-30 18:46:40 PDT 
Created attachment 290418 [details]
Patch v1
Comment 2 David Kilzer (:ddkilzer) 2016-09-30 18:47:42 PDT 
<rdar://problem/27723268>
Comment 3 Joseph Pecoraro 2016-09-30 20:40:39 PDT 
Comment on attachment 290418 [details]
Patch v1

r=me
Comment 4 WebKit Commit Bot 2016-09-30 21:01:47 PDT 
Comment on attachment 290418 [details]
Patch v1

Clearing flags on attachment: 290418

Committed r206706: <http://trac.webkit.org/changeset/206706>
Comment 5 WebKit Commit Bot 2016-09-30 21:01:52 PDT 
All reviewed patches have been landed.  Closing bug.
Comment 6 Alexey Proskuryakov 2016-09-30 21:41:54 PDT 
Is this a code path that cannot be executed in layout tests?
Comment 7 Tim Horton 2016-09-30 22:11:36 PDT 
(In reply to comment #6)
> Is this a code path that cannot be executed in layout tests?

No, the first case should happen in tons and tons of tests.
Comment 8 Alexey Proskuryakov 2016-10-01 00:00:55 PDT 
Why was this not detected, not even with GuardMalloc?
Comment 9 David Kilzer (:ddkilzer) 2016-10-01 03:28:24 PDT 
(In reply to comment #8)
> Why was this not detected, not even with GuardMalloc?

Two reasons that I'm aware of (the radar has a bit more information about CGContextGetType):

1. CGContextGetType() apparently doesn't return kCGContextTypeBitmap as often as we think it does.

2. The retain count of the CGColorSpaceRef object returned in the case of compositing/color-matching/pdf-image-match.html is somewhere in the neighborhood of 4294967295, so that one test would probably have to be run nearly 4294967295+1 times in the same process to over-release the object enough times to cause a crash.