Bug 208303 – CanvasRenderingContext2D.putImageData() should not process neutered ImageData

• Component: Canvas
• Status: RESOLVED FIXED

[Ali Juma]

Created attachment 391857 [details]
Minimal test case

Filing this as a security bug since it was found using a fuzzer; there's no disclosure deadline for this bug.

Crash stack:

=================================================================
==70884==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x7fff2dcc6200 bp 0x7ffeea06a650 sp 0x7ffeea06a628 T0)
==70884==The signal is caused by a READ memory access.
==70884==Hint: address points to the zero page.
==70884==WARNING: invalid path to external symbolizer!
==70884==WARNING: Failed to use and restart external symbolizer!
    #0 0x7fff2dcc61ff in vPremultiplyData_RGBA8888_CV_avx (/System/Library/Frameworks/Accelerate.framework/Versions/A/Frameworks/vImage.framework/Versions/A/vImage:x86_64+0x6981ff)
    #1 0x47d2dd3af in WebCore::premultiplyBufferData(vImage_Buffer const&, vImage_Buffer const&) (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/WebCore.framework/Versions/A/WebCore:x86_64+0x43d73af)
    #2 0x47d2da253 in WebCore::ImageBufferData::putData(JSC::GenericTypedArrayView<JSC::Uint8ClampedAdaptor> const&, WebCore::AlphaPremultiplication, WebCore::IntSize const&, WebCore::IntRect const&, WebCore::IntPoint const&, WebCore::IntSize const&, bool) (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/WebCore.framework/Versions/A/WebCore:x86_64+0x43d4253)
    #3 0x47d2d99b7 in WebCore::ImageBuffer::putByteArray(JSC::GenericTypedArrayView<JSC::Uint8ClampedAdaptor> const&, WebCore::AlphaPremultiplication, WebCore::IntSize const&, WebCore::IntRect const&, WebCore::IntPoint const&, WebCore::ImageBuffer::CoordinateSystem) (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/WebCore.framework/Versions/A/WebCore:x86_64+0x43d39b7)
    #4 0x47c5dbd66 in WebCore::CanvasRenderingContext2DBase::putImageData(WebCore::ImageData&, WebCore::ImageBuffer::CoordinateSystem, float, float, float, float, float, float) (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/WebCore.framework/Versions/A/WebCore:x86_64+0x36d5d66)
    #5 0x47961205f in WebCore::jsCanvasRenderingContext2DPrototypeFunctionPutImageData1Body(JSC::JSGlobalObject*, JSC::CallFrame*, WebCore::JSCanvasRenderingContext2D*, JSC::ThrowScope&) (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/WebCore.framework/Versions/A/WebCore:x86_64+0x70c05f)
    #6 0x479611c75 in WebCore::jsCanvasRenderingContext2DPrototypeFunctionPutImageDataOverloadDispatcher(JSC::JSGlobalObject*, JSC::CallFrame*, WebCore::JSCanvasRenderingContext2D*, JSC::ThrowScope&) (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/WebCore.framework/Versions/A/WebCore:x86_64+0x70bc75)
    #7 0x479575b35 in long long WebCore::IDLOperation<WebCore::JSCanvasRenderingContext2D>::call<&(WebCore::jsCanvasRenderingContext2DPrototypeFunctionPutImageDataOverloadDispatcher(JSC::JSGlobalObject*, JSC::CallFrame*, WebCore::JSCanvasRenderingContext2D*, JSC::ThrowScope&)), (WebCore::CastedThisErrorBehavior)0>(JSC::JSGlobalObject&, JSC::CallFrame&, char const*) (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/WebCore.framework/Versions/A/WebCore:x86_64+0x66fb35)
    #8 0x277644001177  (<unknown module>)
    #9 0x493a6745b in llint_entry (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/JavaScriptCore.framework/Versions/A/JavaScriptCore:x86_64+0xa8c45b)
    #10 0x493a503d8 in vmEntryToJavaScript (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/JavaScriptCore.framework/Versions/A/JavaScriptCore:x86_64+0xa753d8)
    #11 0x49506a744 in JSC::Interpreter::execute(JSC::EvalExecutable*, JSC::JSGlobalObject*, JSC::JSValue, JSC::JSScope*) (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/JavaScriptCore.framework/Versions/A/JavaScriptCore:x86_64+0x208f744)
    #12 0x495068f39 in JSC::eval(JSC::JSGlobalObject*, JSC::CallFrame*) (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/JavaScriptCore.framework/Versions/A/JavaScriptCore:x86_64+0x208df39)
    #13 0x49532881d in JSC::LLInt::commonCallEval(JSC::CallFrame*, JSC::Instruction const*, JSC::MacroAssemblerCodePtr<(WTF::PtrTag)357>) (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/JavaScriptCore.framework/Versions/A/JavaScriptCore:x86_64+0x234d81d)
    #14 0x493a68ee2 in llint_entry (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/JavaScriptCore.framework/Versions/A/JavaScriptCore:x86_64+0xa8dee2)
    #15 0x493a6745b in llint_entry (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/JavaScriptCore.framework/Versions/A/JavaScriptCore:x86_64+0xa8c45b)
    #16 0x493a503d8 in vmEntryToJavaScript (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/JavaScriptCore.framework/Versions/A/JavaScriptCore:x86_64+0xa753d8)
    #17 0x49507440d in JSC::Interpreter::executeProgram(JSC::SourceCode const&, JSC::JSGlobalObject*, JSC::JSObject*) (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/JavaScriptCore.framework/Versions/A/JavaScriptCore:x86_64+0x209940d)
    #18 0x4957263fb in JSC::evaluate(JSC::JSGlobalObject*, JSC::SourceCode const&, JSC::JSValue, WTF::NakedPtr<JSC::Exception>&) (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/JavaScriptCore.framework/Versions/A/JavaScriptCore:x86_64+0x274b3fb)
    #19 0x4957266cc in JSC::profiledEvaluate(JSC::JSGlobalObject*, JSC::ProfilingReason, JSC::SourceCode const&, JSC::JSValue, WTF::NakedPtr<JSC::Exception>&) (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/JavaScriptCore.framework/Versions/A/JavaScriptCore:x86_64+0x274b6cc)
    #20 0x47b86dcd3 in WebCore::JSExecState::profiledEvaluate(JSC::JSGlobalObject*, JSC::ProfilingReason, JSC::SourceCode const&, JSC::JSValue, WTF::NakedPtr<JSC::Exception>&) (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/WebCore.framework/Versions/A/WebCore:x86_64+0x2967cd3)
    #21 0x47b86d4fb in WebCore::ScriptController::evaluateInWorld(WebCore::ScriptSourceCode const&, WebCore::DOMWrapperWorld&) (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/WebCore.framework/Versions/A/WebCore:x86_64+0x29674fb)
    #22 0x47b86d10c in WebCore::ScriptController::evaluateInWorldIgnoringException(WebCore::ScriptSourceCode const&, WebCore::DOMWrapperWorld&) (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/WebCore.framework/Versions/A/WebCore:x86_64+0x296710c)
    #23 0x47c049481 in WebCore::ScriptElement::executeClassicScript(WebCore::ScriptSourceCode const&) (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/WebCore.framework/Versions/A/WebCore:x86_64+0x3143481)
    #24 0x47c046490 in WebCore::ScriptElement::prepareScript(WTF::TextPosition const&, WebCore::ScriptElement::LegacyTypeSupport) (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/WebCore.framework/Versions/A/WebCore:x86_64+0x3140490)
    #25 0x47c6f528e in WebCore::HTMLScriptRunner::runScript(WebCore::ScriptElement&, WTF::TextPosition const&) (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/WebCore.framework/Versions/A/WebCore:x86_64+0x37ef28e)
    #26 0x47c6f4f64 in WebCore::HTMLScriptRunner::execute(WTF::Ref<WebCore::ScriptElement, WTF::DumbPtrTraits<WebCore::ScriptElement> >&&, WTF::TextPosition const&) (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/WebCore.framework/Versions/A/WebCore:x86_64+0x37eef64)
    #27 0x47c6d535c in WebCore::HTMLDocumentParser::runScriptsForPausedTreeBuilder() (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/WebCore.framework/Versions/A/WebCore:x86_64+0x37cf35c)
    #28 0x47c6d59f4 in WebCore::HTMLDocumentParser::pumpTokenizerLoop(WebCore::HTMLDocumentParser::SynchronousMode, bool, WebCore::PumpSession&) (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/WebCore.framework/Versions/A/WebCore:x86_64+0x37cf9f4)
    #29 0x47c6d49dd in WebCore::HTMLDocumentParser::pumpTokenizer(WebCore::HTMLDocumentParser::SynchronousMode) (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/WebCore.framework/Versions/A/WebCore:x86_64+0x37ce9dd)
    #30 0x47c6d7109 in WebCore::HTMLDocumentParser::resumeParsingAfterScriptExecution() (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/WebCore.framework/Versions/A/WebCore:x86_64+0x37d1109)
    #31 0x47c6d7388 in WebCore::HTMLDocumentParser::notifyFinished(WebCore::PendingScript&) (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/WebCore.framework/Versions/A/WebCore:x86_64+0x37d1388)
    #32 0x47c00b122 in WebCore::PendingScript::notifyClientFinished() (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/WebCore.framework/Versions/A/WebCore:x86_64+0x3105122)
    #33 0x47bf9a811 in WebCore::LoadableScript::notifyClientFinished() (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/WebCore.framework/Versions/A/WebCore:x86_64+0x3094811)
    #34 0x47bf9a229 in WebCore::LoadableClassicScript::notifyFinished(WebCore::CachedResource&) (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/WebCore.framework/Versions/A/WebCore:x86_64+0x3094229)
    #35 0x47cc50927 in WebCore::CachedResource::checkNotify() (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/WebCore.framework/Versions/A/WebCore:x86_64+0x3d4a927)
    #36 0x47cbd0cde in WebCore::SubresourceLoader::didFinishLoading(WebCore::NetworkLoadMetrics const&) (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/WebCore.framework/Versions/A/WebCore:x86_64+0x3ccacde)
    #37 0x1072e9ca6 in WebKit::WebResourceLoader::didFinishResourceLoad(WebCore::NetworkLoadMetrics const&) (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/WebKit.framework/Versions/A/WebKit:x86_64+0x1754ca6)
    #38 0x1079eb547 in void IPC::handleMessage<Messages::WebResourceLoader::DidFinishResourceLoad, WebKit::WebResourceLoader, void (WebKit::WebResourceLoader::*)(WebCore::NetworkLoadMetrics const&)>(IPC::Decoder&, WebKit::WebResourceLoader*, void (WebKit::WebResourceLoader::*)(WebCore::NetworkLoadMetrics const&)) (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/WebKit.framework/Versions/A/WebKit:x86_64+0x1e56547)
    #39 0x1079ea649 in WebKit::WebResourceLoader::didReceiveWebResourceLoaderMessage(IPC::Connection&, IPC::Decoder&) (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/WebKit.framework/Versions/A/WebKit:x86_64+0x1e55649)
    #40 0x1072a6334 in WebKit::NetworkProcessConnection::didReceiveMessage(IPC::Connection&, IPC::Decoder&) (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/WebKit.framework/Versions/A/WebKit:x86_64+0x1711334)
    #41 0x105c1a98a in IPC::Connection::dispatchMessage(IPC::Decoder&) (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/WebKit.framework/Versions/A/WebKit:x86_64+0x8598a)
    #42 0x105c1b67a in IPC::Connection::dispatchMessage(std::__1::unique_ptr<IPC::Decoder, std::__1::default_delete<IPC::Decoder> >) (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/WebKit.framework/Versions/A/WebKit:x86_64+0x8667a)
    #43 0x105c1c2b8 in IPC::Connection::dispatchOneIncomingMessage() (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/WebKit.framework/Versions/A/WebKit:x86_64+0x872b8)
    #44 0x493098746 in WTF::RunLoop::performWork() (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/JavaScriptCore.framework/Versions/A/JavaScriptCore:x86_64+0xbd746)
    #45 0x49309925a in WTF::RunLoop::performWork(void*) (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/JavaScriptCore.framework/Versions/A/JavaScriptCore:x86_64+0xbe25a)
    #46 0x7fff313eb31a in __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ (/System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation:x86_64+0x5731a)
    #47 0x7fff313eb2c0 in __CFRunLoopDoSource0 (/System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation:x86_64+0x572c0)
    #48 0x7fff313cf1ba in __CFRunLoopDoSources0 (/System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation:x86_64+0x3b1ba)
    #49 0x7fff313ce782 in __CFRunLoopRun (/System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation:x86_64+0x3a782)
    #50 0x7fff313ce084 in CFRunLoopRunSpecific (/System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation:x86_64+0x3a084)
    #51 0x7fff33642a9e in -[NSRunLoop(NSRunLoop) runMode:beforeDate:] (/System/Library/Frameworks/Foundation.framework/Versions/C/Foundation:x86_64+0x1ca9e)
    #52 0x7fff33642973 in -[NSRunLoop(NSRunLoop) run] (/System/Library/Frameworks/Foundation.framework/Versions/C/Foundation:x86_64+0x1c973)
    #53 0x7fff5daba1d6 in _xpc_objc_main (/usr/lib/system/libxpc.dylib:x86_64+0x111d6)
    #54 0x7fff5dab9cd8 in xpc_main (/usr/lib/system/libxpc.dylib:x86_64+0x10cd8)
    #55 0x106499465 in WebKit::XPCServiceMain(int, char const**) (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/WebKit.framework/Versions/A/WebKit:x86_64+0x904465)
    #56 0x7fff5d8873d4 in start (/usr/lib/system/libdyld.dylib:x86_64+0x163d4)
==70884==Register values:
rax = 0x0000000000000001  rbx = 0x00007ffeea06a990  rcx = 0x0000000000000001  rdx = 0x0000000000000000
rdi = 0x00000004a441b000  rsi = 0x00007ffeea06a990  rbp = 0x00007ffeea06a650  rsp = 0x00007ffeea06a628
 r8 = 0x00000004a441b000   r9 = 0x0000000000000000  r10 = 0x00000001113d14d8  r11 = 0x00000004a441b000
r12 = 0x0000000000000000  r13 = 0x0000000000000000  r14 = 0x0000000000000001  r15 = 0x0000000000000000

[Radar WebKit Bug Importer]

<rdar://problem/59845825>

[Ryosuke Niwa]

Could you also attach the content of worker-onmessage-noop.js?

[Ali Juma]

Created attachment 392022 [details]
Minimal test case

[Ali Juma]

(In reply to Ryosuke Niwa from comment #2)
> Could you also attach the content of worker-onmessage-noop.js?

There's actually no such file -- the crash reproduces without the file existing. I've changed that line in the test case to 'non-existent-file' to clarify that.

[Ryosuke Niwa]

(In reply to Ali Juma from comment #4)
> (In reply to Ryosuke Niwa from comment #2)
> > Could you also attach the content of worker-onmessage-noop.js?
> 
> There's actually no such file -- the crash reproduces without the file
> existing. I've changed that line in the test case to 'non-existent-file' to
> clarify that.

Ah, okay. Thanks for the clarification.

[Pinki Gyanchandani]

Created attachment 394281 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 394281 [details]
Patch

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

> Source/WebCore/html/canvas/CanvasRenderingContext2DBase.cpp:2162
> +    if (!data.data() || !data.data()->data())

Why is it null, though? The buffer is 1x1?

> LayoutTests/ChangeLog:8
> +        Added the test case attached to bugzilla ticket 208303 with little modification.

We don't call them "tickets". And this is bug 208303 so this comment is redundant.

> LayoutTests/canvas/philip/tests/canvas-bug.html:4
> +<div>Test passes if it does not crash.</div>
> +<canvas id=canvas >

These should be inside <body>. <canvas> should have a closing tag.

> LayoutTests/canvas/philip/tests/canvas-bug.html:11
> +    var worker = new Worker('./resources/worker-onmessage-noop.js');

Is the worker necessary?

[Pinki Gyanchandani]

(In reply to Simon Fraser (smfr) from comment #7)
> Comment on attachment 394281 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=394281&action=review
> 
> > Source/WebCore/html/canvas/CanvasRenderingContext2DBase.cpp:2162
> > +    if (!data.data() || !data.data()->data())
> 
> Why is it null, though? The buffer is 1x1?

Probably because the file which is used for worker is not valid. In the test case attached by google, ./resources/worker-onmessage-noop.js is modified to "non-existent-file".

So array buffer content in CanvasRenderingContext2DBase::putImageData is received as NULL


> 
> > LayoutTests/ChangeLog:8
> > +        Added the test case attached to bugzilla ticket 208303 with little modification.
> 
> We don't call them "tickets". And this is bug 208303 so this comment is
> redundant.

OK, will correct the comment

> 
> > LayoutTests/canvas/philip/tests/canvas-bug.html:4
> > +<div>Test passes if it does not crash.</div>
> > +<canvas id=canvas >
> 
> These should be inside <body>. 
<div> </div> If I put inside body, it doesn't dump the text in canvas-big-exptected.txt file.
So I moved it here. Any other way to get it right?? 

<canvas> should have a closing tag.

OK, Will add </canvas> to the test case


> 
> > LayoutTests/canvas/philip/tests/canvas-bug.html:11
> > +    var worker = new Worker('./resources/worker-onmessage-noop.js');
> 
> Is the worker necessary?

Yes, not having worker, the crash is not reproduced.
The attached file its changed to var worker = new Worker('non-existent-file');.
I have also changed the same in test case and crash is still happening.

(In reply to Simon Fraser (smfr) from comment #7)
> Comment on attachment 394281 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=394281&action=review
> 
> > Source/WebCore/html/canvas/CanvasRenderingContext2DBase.cpp:2162
> > +    if (!data.data() || !data.data()->data())
> 
> Why is it null, though? The buffer is 1x1?
> 
> > LayoutTests/ChangeLog:8
> > +        Added the test case attached to bugzilla ticket 208303 with little modification.
> 
> We don't call them "tickets". And this is bug 208303 so this comment is
> redundant.
> 
> > LayoutTests/canvas/philip/tests/canvas-bug.html:4
> > +<div>Test passes if it does not crash.</div>
> > +<canvas id=canvas >
> 
> These should be inside <body>. <canvas> should have a closing tag.
> 
> > LayoutTests/canvas/philip/tests/canvas-bug.html:11
> > +    var worker = new Worker('./resources/worker-onmessage-noop.js');
> 
> Is the worker necessary?

[Pinki Gyanchandani]

Created attachment 394457 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 394457 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        Test: canvas/philip/tests/canvas-bug.html

This line should appear after the long description but above the list of files.

> Source/WebCore/ChangeLog:13
> +

Right here, surrounded by single blank lines.

[Pinki Gyanchandani]

Created attachment 394458 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 394281 [details]
Patch

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

>>>> Source/WebCore/html/canvas/CanvasRenderingContext2DBase.cpp:2162
>>>> +    if (!data.data() || !data.data()->data())
>>> 
>>> Why is it null, though? The buffer is 1x1?
>> 
>> Probably because the file which is used for worker is not valid. In the test case attached by google, ./resources/worker-onmessage-noop.js is modified to "non-existent-file".
>> 
>> So array buffer content in CanvasRenderingContext2DBase::putImageData is received as NULL
> 
> 

This explanation is incorrect.  This JavaScript statement 

    worker.postMessage({data: image.data.buffer}, [image.data.buffer]);

transfers the ownership of the image.data.buffer to the worker. Therefore image.data becomes "neutered". See https://developer.mozilla.org/en-US/docs/Web/API/Worker/postMessage. So this change is okay but you may change it to 

    if (!data.data() || data.data()->isNeutered())

[Said Abou-Hallawa]

Comment on attachment 394457 [details]
Patch

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

> LayoutTests/ChangeLog:11
> +        * canvas/philip/tests/canvas-bug-expected.txt: Added.
> +        * canvas/philip/tests/canvas-bug.html: Added.

Are you sure this test crashes when you run-webkit-tests it without your fix? I could only got it to crash when I made it an http test. Please put it under LayoutTests/http/tests/misc/. And please do not make its name "canvas-bug.html". It is too generic. You may use canvas-putImageData-neutered-ImageData.html or something similar.

[Said Abou-Hallawa]

Comment on attachment 394457 [details]
Patch

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

>> LayoutTests/ChangeLog:11
>> +        * canvas/philip/tests/canvas-bug.html: Added.
> 
> Are you sure this test crashes when you run-webkit-tests it without your fix? I could only got it to crash when I made it an http test. Please put it under LayoutTests/http/tests/misc/. And please do not make its name "canvas-bug.html". It is too generic. You may use canvas-putImageData-neutered-ImageData.html or something similar.

I was wrong. I could get it to crash when it is local layout test. But please put it under LayoutTests/fast/canvas since canvas/Philip/tests are imported tests.

[Pinki Gyanchandani]

Created attachment 394470 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 394470 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        Crash in WebCore::premultiplyBufferData

Please change the bug title in Bugzilla and in the ChangeLogs to reflect what exactly the bug is. The bug title should say that CanvasRenderingContext2D.putImageData() should not process neutered ImageData.

> Source/WebCore/ChangeLog:13
> +        Crash is observed in PremultiplyData_RGBA8888_CV_avx2 () as src is NULL.
> +        arrayBuffercontent for data of imageData's object is NULL. 
> +        The postMessage() method of the worker, transfers the ownership of the image.data.buffer to the worker,
> +        causing image.data becomes "neutered". Added the check for the same.

Please consider revising the description of the bug and the fix. The bug has nothing to do with PremultiplyData_RGBA8888_CV_avx2(), so you do not have even to mention it. The description should focus on when and why the crash happens and how it is fixed.

> LayoutTests/ChangeLog:8
> +        Added slightly modified version of testcase from bugzilla.

This comment adds no value. Either describe what the test is doing or just remove it.

> LayoutTests/fast/canvas/canvas-putImageData-neutered-ImageData.html:12
> +    var worker = new Worker('non-existent-file');

Do we still need 'non-existent-file'?

> LayoutTests/fast/canvas/canvas-putImageData-neutered-ImageData.html:17
> +    worker.postMessage({data: image.data.buffer}, [image.data.buffer]);

Can't this just be:

    worker.postMessage([image.data.buffer]);

> LayoutTests/fast/canvas/canvas-putImageData-neutered-ImageData.html:23
> +<body onload=canvasTest()>
> +</body>

Usually the <body></body> tag encloses all the other elements. So this test can look like this:

<body onload=canvasTest()>
    <div>Test passes if it does not crash.</div>
    <canvas id=canvas></canvas>
</body>

[Pinki Gyanchandani]

(In reply to Said Abou-Hallawa from comment #16)
> Comment on attachment 394470 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=394470&action=review
> 
> > Source/WebCore/ChangeLog:3
> > +        Crash in WebCore::premultiplyBufferData
> 
> Please change the bug title in Bugzilla and in the ChangeLogs to reflect
> what exactly the bug is. The bug title should say that
> CanvasRenderingContext2D.putImageData() should not process neutered
> ImageData.

I don't have permission to change the title. Seems only submitter can change it.
Can anyone help with this??


> 
> > Source/WebCore/ChangeLog:13
> > +        Crash is observed in PremultiplyData_RGBA8888_CV_avx2 () as src is NULL.
> > +        arrayBuffercontent for data of imageData's object is NULL. 
> > +        The postMessage() method of the worker, transfers the ownership of the image.data.buffer to the worker,
> > +        causing image.data becomes "neutered". Added the check for the same.
> 
> Please consider revising the description of the bug and the fix. The bug has
> nothing to do with PremultiplyData_RGBA8888_CV_avx2(), so you do not have
> even to mention it. The description should focus on when and why the crash
> happens and how it is fixed


OK will modify it.

> > LayoutTests/ChangeLog:8
> > +        Added slightly modified version of testcase from bugzilla.
> 
> This comment adds no value. Either describe what the test is doing or just
> remove it.
> 
> > LayoutTests/fast/canvas/canvas-putImageData-neutered-ImageData.html:12
> > +    var worker = new Worker('non-existent-file');
> 
> Do we still need 'non-existent-file'?


Removing var worker = new Worker('non-existent-file'); doesn't work. crash is not reproduced for me if removed.

> 
> > LayoutTests/fast/canvas/canvas-putImageData-neutered-ImageData.html:17
> > +    worker.postMessage({data: image.data.buffer}, [image.data.buffer]);
> 
> Can't this just be:
> 
>     worker.postMessage([image.data.buffer]);

No, crash is not reproduced for me with this statement changed. It has to be as is.


> 
> > LayoutTests/fast/canvas/canvas-putImageData-neutered-ImageData.html:23
> > +<body onload=canvasTest()>
> > +</body>
> 
> Usually the <body></body> tag encloses all the other elements. So this test
> can look like this:
> 
> <body onload=canvasTest()>
>     <div>Test passes if it does not crash.</div>
>     <canvas id=canvas></canvas>
> </body>

will change it

[Pinki Gyanchandani]

Created attachment 394520 [details]
Patch

[Ryosuke Niwa]

There is no security implication here (checked with Simon & Said).

[Ryosuke Niwa]

Let's wait for EWS (e.g. iOS WK2).

[EWS]

Committed r259024: <https://trac.webkit.org/changeset/259024>

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