Bug 221376 – Crash in RetainPtr<CGImage*>::RetainPtr via ImageBufferCGBackend::toCFData

• Component: Images
• Status: RESOLVED FIXED

[Ryosuke Niwa]

e.g.

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.WebCore             	0x000000039181b00f WTF::RetainPtr<CGImage*>::RetainPtr(WTF::RetainPtr<CGImage*> const&) + 0 (RetainPtr.h:82) [inlined]
1   com.apple.WebCore             	0x000000039181b00f WTF::RetainPtr<CGImage*>::RetainPtr(WTF::RetainPtr<CGImage*> const&) + 0 (RetainPtr.h:82) [inlined]
2   com.apple.WebCore             	0x000000039181b00f WTF::RetainPtr<CGImage*>::operator=(WTF::RetainPtr<CGImage*> const&) + 0 (RetainPtr.h:234) [inlined]
3   com.apple.WebCore             	0x000000039181b00f WebCore::ImageBufferCGBackend::toCFData(WTF::String const&, WTF::Optional<double>, WebCore::PreserveResolution) const + 367 (ImageBufferCGBackend.cpp:194)
4   com.apple.WebCore             	0x000000039181b4f9 WebCore::ImageBufferCGBackend::toData(WTF::String const&, WTF::Optional<double>) const + 57 (ImageBufferCGBackend.cpp:214)
5   com.apple.WebCore             	0x00000003917b5da8 WebCore::ConcreteImageBuffer<WebCore::ImageBufferCGBitmapBackend>::toData(WTF::String const&, WTF::Optional<double>) const + 104 (ConcreteImageBuffer.h:219)
6   com.apple.WebCore             	0x0000000391273cb2 WebCore::HTMLCanvasElement::toBlob(WebCore::ScriptExecutionContext&, WTF::Ref<WebCore::BlobCallback, WTF::RawPtrTraits<WebCore::BlobCallback> >&&, WTF::String const&, JSC::JSValue) + 754 (HTMLCanvasElement.cpp:763)
7   com.apple.WebCore             	0x0000000390488935 WebCore::jsHTMLCanvasElementPrototypeFunction_toBlobBody(JSC::JSGlobalObject*, JSC::CallFrame*, WebCore::JSHTMLCanvasElement*) + 493 (JSHTMLCanvasElement.cpp:361) [inlined]
8   com.apple.WebCore             	0x0000000390488935 long long WebCore::IDLOperation<WebCore::JSHTMLCanvasElement>::call<&(WebCore::jsHTMLCanvasElementPrototypeFunction_toBlobBody(JSC::JSGlobalObject*, JSC::CallFrame*, WebCore::JSHTMLCanvasElement*)), (WebCore::CastedThisErrorBehavior)0>(JSC::JSGlobalObject&, JSC::CallFrame&, char const*) + 644 (JSDOMOperation.h:53) [inlined]
9   com.apple.WebCore             	0x0000000390488935 WebCore::jsHTMLCanvasElementPrototypeFunction_toBlob(JSC::JSGlobalObject*, JSC::CallFrame*) + 677 (JSHTMLCanvasElement.cpp:367)
10  ???                           	0x00004a94b0e011d8 0 + 82002483089880

<rdar://problem/72988880>

[Ryosuke Niwa]

Created attachment 419221 [details]
Test

[Frédéric Wang (:fredw)]

Created attachment 419280 [details]
Reduced testcase for ASSERTION FAILED: m_accumulatedOffsetMightBeSaturated || roundedIntPoint(LayoutPoint(rendererMappedResult)) == result

I'm not able to reproduce the crash. I get a different assertion failure:

ASSERTION FAILED: m_accumulatedOffsetMightBeSaturated || roundedIntPoint(LayoutPoint(rendererMappedResult)) == result

(I'm attaching a reduced testcase for that one)

After removing the "translate:" rules from the original test case, it runs normally.

[Ryosuke Niwa]

You need a release build to hit this crash.

[Frédéric Wang (:fredw)]

Created attachment 419440 [details]
Reduced testcase

(In reply to Ryosuke Niwa from comment #3)
> You need a release build to hit this crash.

OK, apparently this does not crash on Linux. Here is a reduced testcase that crashes on mac (debug and release).

[Simon Fraser (smfr)]

function resize() { canvas.width = 1827092040186686; }

[Frédéric Wang (:fredw)]

(In reply to Simon Fraser (smfr) from comment #5)
> function resize() { canvas.width = 1827092040186686; }

Sorry Simon, what do you mean here? are you able to reduce it even more?

[Ryosuke Niwa]

Isn't the issue here that the canvas is too big to allocate CGImage for?

[Simon Fraser (smfr)]

No, I'm just pointing out the obvious cause.

[Frédéric Wang (:fredw)]

Created attachment 419840 [details]
Reduced testcase

Further reduction.

Will take a look at this later.

[Frédéric Wang (:fredw)]

Created attachment 419959 [details]
Layout tests

[Frédéric Wang (:fredw)]

Created attachment 419960 [details]
Layout tests

[Frédéric Wang (:fredw)]

Created attachment 419961 [details]
Layout tests

[Frédéric Wang (:fredw)]

Created attachment 419963 [details]
Patch

Here is a patch. The (reduced) layout test is attached separately. Note that only the part for copyNativeImage(CopyBackingStore) is necessary to fix the crash, but it seems good to do it for copyNativeImage(DontCopyBackingStore) too (not sure how to hit that branch though). Going over the rest of the copyNativeImage() calls, I think we now always null-check the result before using it (or at least return a nullptr to the callers).

[Frédéric Wang (:fredw)]

Created attachment 419973 [details]
Patch

oops, I uploaded a bad version this morning.

[Said Abou-Hallawa]

Comment on attachment 419973 [details]
Patch

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

> Source/WebCore/ChangeLog:7
> +

Please add a description for the fix.

[Ryosuke Niwa]

Is this an actual security bug or just a nullptr dereference? If latter, please include the test in the patch.

[Frédéric Wang (:fredw)]

Created attachment 420112 [details]
Patch

[Frédéric Wang (:fredw)]

(In reply to Ryosuke Niwa from comment #16)
> Is this an actual security bug or just a nullptr dereference? If latter,
> please include the test in the patch.

This is definitely a nullptr dereference that causes the application to crash. Whether this is an exploitable vulnerability, I don't know. I uploaded another version of the patch with a test.

[Frédéric Wang (:fredw)]

Created attachment 420216 [details]
Patch

[Said Abou-Hallawa]

This is a nullptr dereference and should not be a security bug. I think a RefPtr<NativeImage> with a null pointer can't be exploitable. Therefore the component of this bug should be changed to "Canvas".

[Frédéric Wang (:fredw)]

Created attachment 420277 [details]
Patch for landing

[EWS]

Committed r272845: <https://commits.webkit.org/r272845>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 420277 [details].