Bug 221376 - Crash in RetainPtr<CGImage*>::RetainPtr via ImageBufferCGBackend::toCFData
Summary: Crash in RetainPtr<CGImage*>::RetainPtr via ImageBufferCGBackend::toCFData
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Images (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Frédéric Wang (:fredw)
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-02-03 20:09 PST by Ryosuke Niwa
Modified: 2021-02-15 02:38 PST (History)
13 users (show)

See Also:


Attachments
Test (1.28 MB, text/html)
2021-02-03 20:09 PST, Ryosuke Niwa
no flags Details
Reduced testcase for ASSERTION FAILED: m_accumulatedOffsetMightBeSaturated || roundedIntPoint(LayoutPoint(rendererMappedResult)) == result (54 bytes, text/html)
2021-02-04 07:18 PST, Frédéric Wang (:fredw)
no flags Details
Reduced testcase (225 bytes, text/html)
2021-02-05 11:39 PST, Frédéric Wang (:fredw)
no flags Details
Reduced testcase (104 bytes, text/html)
2021-02-10 07:28 PST, Frédéric Wang (:fredw)
no flags Details
Layout tests (1.48 KB, patch)
2021-02-11 00:43 PST, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Layout tests (2.69 KB, patch)
2021-02-11 01:03 PST, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Layout tests (1.47 KB, patch)
2021-02-11 01:04 PST, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (2.18 KB, patch)
2021-02-11 01:35 PST, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (2.73 KB, patch)
2021-02-11 03:46 PST, Frédéric Wang (:fredw)
sabouhallawa: review+
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (5.16 KB, patch)
2021-02-12 04:18 PST, Frédéric Wang (:fredw)
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (5.79 KB, patch)
2021-02-13 00:44 PST, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch for landing (5.80 KB, patch)
2021-02-15 01:08 PST, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2021-02-03 20:09:34 PST
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>
Comment 1 Ryosuke Niwa 2021-02-03 20:09:57 PST
Created attachment 419221 [details]
Test
Comment 2 Frédéric Wang (:fredw) 2021-02-04 07:18:21 PST
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.
Comment 3 Ryosuke Niwa 2021-02-04 16:54:08 PST
You need a release build to hit this crash.
Comment 4 Frédéric Wang (:fredw) 2021-02-05 11:39:47 PST
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).
Comment 5 Simon Fraser (smfr) 2021-02-06 10:07:18 PST
function resize() { canvas.width = 1827092040186686; }
Comment 6 Frédéric Wang (:fredw) 2021-02-06 10:26:05 PST
(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?
Comment 7 Ryosuke Niwa 2021-02-06 15:55:06 PST
Isn't the issue here that the canvas is too big to allocate CGImage for?
Comment 8 Simon Fraser (smfr) 2021-02-08 11:20:02 PST
No, I'm just pointing out the obvious cause.
Comment 9 Frédéric Wang (:fredw) 2021-02-10 07:28:01 PST
Created attachment 419840 [details]
Reduced testcase

Further reduction.

Will take a look at this later.
Comment 10 Frédéric Wang (:fredw) 2021-02-11 00:43:28 PST
Created attachment 419959 [details]
Layout tests
Comment 11 Frédéric Wang (:fredw) 2021-02-11 01:03:00 PST
Created attachment 419960 [details]
Layout tests
Comment 12 Frédéric Wang (:fredw) 2021-02-11 01:04:20 PST
Created attachment 419961 [details]
Layout tests
Comment 13 Frédéric Wang (:fredw) 2021-02-11 01:35:41 PST
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).
Comment 14 Frédéric Wang (:fredw) 2021-02-11 03:46:17 PST
Created attachment 419973 [details]
Patch

oops, I uploaded a bad version this morning.
Comment 15 Said Abou-Hallawa 2021-02-11 23:11:28 PST
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.
Comment 16 Ryosuke Niwa 2021-02-12 01:31:27 PST
Is this an actual security bug or just a nullptr dereference? If latter, please include the test in the patch.
Comment 17 Frédéric Wang (:fredw) 2021-02-12 04:18:54 PST
Created attachment 420112 [details]
Patch
Comment 18 Frédéric Wang (:fredw) 2021-02-12 04:20:31 PST
(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.
Comment 19 Frédéric Wang (:fredw) 2021-02-13 00:44:58 PST
Created attachment 420216 [details]
Patch
Comment 20 Said Abou-Hallawa 2021-02-14 15:53:07 PST
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".
Comment 21 Frédéric Wang (:fredw) 2021-02-15 01:08:07 PST
Created attachment 420277 [details]
Patch for landing
Comment 22 EWS 2021-02-15 02:38:35 PST
Committed r272845: <https://commits.webkit.org/r272845>

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