Bug 223785 – ASSERTION FAILED: !m_needExceptionCheck in CloneSerializer::serialize with postMessage({g:42})

• Component: Bindings
• Status: RESOLVED FIXED

[Ryosuke Niwa]

Created attachment 424330 [details]
Test

SerializedScriptValue::create is missing can throw without an exception scope

% __XPC_JSC_validateExceptionChecks=1 ./Tools/Scripts/run-test-runner --debug --no-build repro_396.html

Starting WebKitTestRunner with DYLD_FRAMEWORK_PATH set to point to built WebKit in /Volumes/Data/safari-4/OpenSource/WebKitBuild/Debug.
ERROR: Unchecked JS exception:
    This scope can throw a JS exception: getOwnNonIndexPropertyNames @ ./runtime/JSObject.cpp:2476
        (ExceptionScope::m_recursionDepth was 6)
    But the exception was unchecked as of this scope: shouldTerminate @ ./bindings/js/SerializedScriptValue.cpp:504
        (ExceptionScope::m_recursionDepth was 6)

Unchecked exception detected at:
    1   0x78a9eaa1e JSC::VM::verifyExceptionCheckNeedIsSatisfied(unsigned int, JSC::ExceptionEventLocation&)
    2   0x78a9c4a2c JSC::ThrowScope::ThrowScope(JSC::VM&, JSC::ExceptionEventLocation)
    3   0x78a9c4a83 JSC::ThrowScope::ThrowScope(JSC::VM&, JSC::ExceptionEventLocation)
    4   0x76b05ae8b WebCore::CloneBase::shouldTerminate()
    5   0x76b058435 WebCore::CloneSerializer::serialize(JSC::JSValue)
    6   0x76b060739 WebCore::CloneSerializer::serialize(JSC::JSGlobalObject*, JSC::JSValue, WTF::Vector<WTF::RefPtr<WebCore::MessagePort, WTF::RawPtrTraits<WebCore::MessagePort>, WTF::DefaultRefDerefTraits<WebCore::MessagePort> >, 0ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc>&, WTF::Vector<WTF::RefPtr<JSC::ArrayBuffer, WTF::RawPtrTraits<JSC::ArrayBuffer>, WTF::DefaultRefDerefTraits<JSC::ArrayBuffer> >, 0ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc>&, WTF::Vector<WTF::RefPtr<WebCore::ImageBitmap, WTF::RawPtrTraits<WebCore::ImageBitmap>, WTF::DefaultRefDerefTraits<WebCore::ImageBitmap> >, 0ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc> const&, WTF::Vector<WTF::RefPtr<JSC::Wasm::Module, WTF::RawPtrTraits<JSC::Wasm::Module>, WTF::DefaultRefDerefTraits<JSC::Wasm::Module> >, 0ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc>&, WTF::Vector<WTF::RefPtr<JSC::Wasm::MemoryHandle, WTF::RawPtrTraits<JSC::Wasm::MemoryHandle>, WTF::DefaultRefDerefTraits<JSC::Wasm::MemoryHandle> >, 0ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc>&, WTF::Vector<WTF::String, 0ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc>&, WTF::Vector<unsigned char, 0ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc>&, WebCore::SerializationContext, WTF::Vector<JSC::ArrayBufferContents, 0ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc>&)
    7   0x76b06111a WebCore::SerializedScriptValue::create(JSC::JSGlobalObject&, JSC::JSValue, WTF::Vector<JSC::Strong<JSC::JSObject, (JSC::ShouldStrongDestructorGrabLock)0>, 0ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc>&&, WTF::Vector<WTF::RefPtr<WebCore::MessagePort, WTF::RawPtrTraits<WebCore::MessagePort>, WTF::DefaultRefDerefTraits<WebCore::MessagePort> >, 0ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc>&, WebCore::SerializationContext)
    8   0x76c3596a4 WebCore::DOMWindow::postMessage(JSC::JSGlobalObject&, WebCore::DOMWindow&, JSC::JSValue, WebCore::WindowPostMessageOptions&&)
    9   0x7691a7eca WebCore::jsDOMWindowInstanceFunction_postMessage2Body(JSC::JSGlobalObject*, JSC::CallFrame*, WebCore::JSDOMWindow*)
    10  0x7691a7a1b WebCore::jsDOMWindowInstanceFunction_postMessageOverloadDispatcher(JSC::JSGlobalObject*, JSC::CallFrame*, WebCore::JSDOMWindow*)
    11  0x768fd632c long long WebCore::IDLOperation<WebCore::JSDOMWindow>::call<&(WebCore::jsDOMWindowInstanceFunction_postMessageOverloadDispatcher(JSC::JSGlobalObject*, JSC::CallFrame*, WebCore::JSDOMWindow*)), (WebCore::CastedThisErrorBehavior)0>(JSC::JSGlobalObject&, JSC::CallFrame&, char const*)
    12  0x768fd6004 WebCore::jsDOMWindowInstanceFunction_postMessage(JSC::JSGlobalObject*, JSC::CallFrame*)
    13  0x48ccbce011d8
    14  0x7893d21ef llint_entry
    15  0x7893b0250 vmEntryToJavaScript
    16  0x78a26bb2b JSC::JITCode::execute(JSC::VM*, JSC::ProtoCallFrame*)
    17  0x78a26b088 JSC::Interpreter::executeProgram(JSC::SourceCode const&, JSC::JSGlobalObject*, JSC::JSObject*)
    18  0x78a648847 JSC::evaluate(JSC::JSGlobalObject*, JSC::SourceCode const&, JSC::JSValue, WTF::NakedPtr<JSC::Exception>&)
    19  0x78a64899a JSC::profiledEvaluate(JSC::JSGlobalObject*, JSC::ProfilingReason, JSC::SourceCode const&, JSC::JSValue, WTF::NakedPtr<JSC::Exception>&)
    20  0x76b04d84c WebCore::JSExecState::profiledEvaluate(JSC::JSGlobalObject*, JSC::ProfilingReason, JSC::SourceCode const&, JSC::JSValue, WTF::NakedPtr<JSC::Exception>&)
    21  0x76b04d42e WebCore::ScriptController::evaluateInWorld(WebCore::ScriptSourceCode const&, WebCore::DOMWrapperWorld&)
    22  0x76b04d259 WebCore::ScriptController::evaluateInWorldIgnoringException(WebCore::ScriptSourceCode const&, WebCore::DOMWrapperWorld&)
    23  0x76b04db55 WebCore::ScriptController::evaluateIgnoringException(WebCore::ScriptSourceCode const&)
    24  0x76b79e7e6 WebCore::ScriptElement::executeClassicScript(WebCore::ScriptSourceCode const&)
    25  0x76b79c7fb WebCore::ScriptElement::prepareScript(WTF::TextPosition const&, WebCore::ScriptElement::LegacyTypeSupport)
    26  0x76bd2edb6 WebCore::HTMLScriptRunner::runScript(WebCore::ScriptElement&, WTF::TextPosition const&)
    27  0x76bd2ebb7 WebCore::HTMLScriptRunner::execute(WTF::Ref<WebCore::ScriptElement, WTF::RawPtrTraits<WebCore::ScriptElement> >&&, WTF::TextPosition const&)
    28  0x76bd0d401 WebCore::HTMLDocumentParser::runScriptsForPausedTreeBuilder()
    29  0x76bd0d885 WebCore::HTMLDocumentParser::pumpTokenizerLoop(WebCore::HTMLDocumentParser::SynchronousMode, bool, WebCore::PumpSession&)
    30  0x76bd0cbff WebCore::HTMLDocumentParser::pumpTokenizer(WebCore::HTMLDocumentParser::SynchronousMode)
    31  0x76bd0c396 WebCore::HTMLDocumentParser::pumpTokenizerIfPossible(WebCore::HTMLDocumentParser::SynchronousMode)
    32  0x76bd0e634 WebCore::HTMLDocumentParser::append(WTF::RefPtr<WTF::StringImpl, WTF::RawPtrTraits<WTF::StringImpl>, WTF::DefaultRefDerefTraits<WTF::StringImpl> >&&)
    33  0x76b5a5d56 WebCore::DecodedDataDocumentParser::flush(WebCore::DocumentWriter&)
    34  0x76c19b5fe WebCore::DocumentWriter::end()
    35  0x76c14da24 WebCore::DocumentLoader::finishedLoading()
    36  0x76c14d3c1 WebCore::DocumentLoader::notifyFinished(WebCore::CachedResource&, WebCore::NetworkLoadMetrics const&)
    37  0x76c2d008a WebCore::CachedResource::checkNotify(WebCore::NetworkLoadMetrics const&)
    38  0x76c2cbb7c WebCore::CachedResource::finishLoading(WebCore::SharedBuffer*, WebCore::NetworkLoadMetrics const&)
    39  0x76c2cd0fc WebCore::CachedRawResource::finishLoading(WebCore::SharedBuffer*, WebCore::NetworkLoadMetrics const&)
    40  0x76c2534a4 WebCore::SubresourceLoader::didFinishLoading(WebCore::NetworkLoadMetrics const&)
    41  0x759ceed5a WebKit::WebResourceLoader::didFinishResourceLoad(WebCore::NetworkLoadMetrics const&)
    42  0x75a2d3cc0 void IPC::callMemberFunctionImpl<WebKit::WebResourceLoader, void (WebKit::WebResourceLoader::*)(WebCore::NetworkLoadMetrics const&), std::__1::tuple<WebCore::NetworkLoadMetrics>, 0ul>(WebKit::WebResourceLoader*, void (WebKit::WebResourceLoader::*)(WebCore::NetworkLoadMetrics const&), std::__1::tuple<WebCore::NetworkLoadMetrics>&&, std::__1::integer_sequence<unsigned long, 0ul>)
    43  0x75a2d3c10 void IPC::callMemberFunction<WebKit::WebResourceLoader, void (WebKit::WebResourceLoader::*)(WebCore::NetworkLoadMetrics const&), std::__1::tuple<WebCore::NetworkLoadMetrics>, std::__1::integer_sequence<unsigned long, 0ul> >(std::__1::tuple<WebCore::NetworkLoadMetrics>&&, WebKit::WebResourceLoader*, void (WebKit::WebResourceLoader::*)(WebCore::NetworkLoadMetrics const&))
    44  0x75a2d19be void IPC::handleMessage<Messages::WebResourceLoader::DidFinishResourceLoad, WebKit::WebResourceLoader, void (WebKit::WebResourceLoader::*)(WebCore::NetworkLoadMetrics const&)>(IPC::Decoder&, WebKit::WebResourceLoader*, void (WebKit::WebResourceLoader::*)(WebCore::NetworkLoadMetrics const&))
    45  0x75a2d132e WebKit::WebResourceLoader::didReceiveWebResourceLoaderMessage(IPC::Connection&, IPC::Decoder&)
    46  0x759caff50 WebKit::NetworkProcessConnection::didReceiveMessage(IPC::Connection&, IPC::Decoder&)
    47  0x758085904 IPC::Connection::dispatchMessage(IPC::Decoder&)

ASSERTION FAILED: !m_needExceptionCheck
./runtime/VM.cpp(1418) : void JSC::VM::verifyExceptionCheckNeedIsSatisfied(unsigned int, JSC::ExceptionEventLocation &)
1   0x788de4cc9 WTFCrash
2   0x78a57d2db WTFCrashWithInfo(int, char const*, char const*, int)
3   0x78a9eab4e JSC::VM::verifyExceptionCheckNeedIsSatisfied(unsigned int, JSC::ExceptionEventLocation&)
4   0x78a9c4a2c JSC::ThrowScope::ThrowScope(JSC::VM&, JSC::ExceptionEventLocation)
5   0x78a9c4a83 JSC::ThrowScope::ThrowScope(JSC::VM&, JSC::ExceptionEventLocation)
6   0x76b05ae8b WebCore::CloneBase::shouldTerminate()
7   0x76b058435 WebCore::CloneSerializer::serialize(JSC::JSValue)
8   0x76b060739 WebCore::CloneSerializer::serialize(JSC::JSGlobalObject*, JSC::JSValue, WTF::Vector<WTF::RefPtr<WebCore::MessagePort, WTF::RawPtrTraits<WebCore::MessagePort>, WTF::DefaultRefDerefTraits<WebCore::MessagePort> >, 0ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc>&, WTF::Vector<WTF::RefPtr<JSC::ArrayBuffer, WTF::RawPtrTraits<JSC::ArrayBuffer>, WTF::DefaultRefDerefTraits<JSC::ArrayBuffer> >, 0ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc>&, WTF::Vector<WTF::RefPtr<WebCore::ImageBitmap, WTF::RawPtrTraits<WebCore::ImageBitmap>, WTF::DefaultRefDerefTraits<WebCore::ImageBitmap> >, 0ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc> const&, WTF::Vector<WTF::RefPtr<JSC::Wasm::Module, WTF::RawPtrTraits<JSC::Wasm::Module>, WTF::DefaultRefDerefTraits<JSC::Wasm::Module> >, 0ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc>&, WTF::Vector<WTF::RefPtr<JSC::Wasm::MemoryHandle, WTF::RawPtrTraits<JSC::Wasm::MemoryHandle>, WTF::DefaultRefDerefTraits<JSC::Wasm::MemoryHandle> >, 0ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc>&, WTF::Vector<WTF::String, 0ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc>&, WTF::Vector<unsigned char, 0ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc>&, WebCore::SerializationContext, WTF::Vector<JSC::ArrayBufferContents, 0ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc>&)
9   0x76b06111a WebCore::SerializedScriptValue::create(JSC::JSGlobalObject&, JSC::JSValue, WTF::Vector<JSC::Strong<JSC::JSObject, (JSC::ShouldStrongDestructorGrabLock)0>, 0ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc>&&, WTF::Vector<WTF::RefPtr<WebCore::MessagePort, WTF::RawPtrTraits<WebCore::MessagePort>, WTF::DefaultRefDerefTraits<WebCore::MessagePort> >, 0ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc>&, WebCore::SerializationContext)
10  0x76c3596a4 WebCore::DOMWindow::postMessage(JSC::JSGlobalObject&, WebCore::DOMWindow&, JSC::JSValue, WebCore::WindowPostMessageOptions&&)
11  0x7691a7eca WebCore::jsDOMWindowInstanceFunction_postMessage2Body(JSC::JSGlobalObject*, JSC::CallFrame*, WebCore::JSDOMWindow*)
12  0x7691a7a1b WebCore::jsDOMWindowInstanceFunction_postMessageOverloadDispatcher(JSC::JSGlobalObject*, JSC::CallFrame*, WebCore::JSDOMWindow*)
13  0x768fd632c long long WebCore::IDLOperation<WebCore::JSDOMWindow>::call<&(WebCore::jsDOMWindowInstanceFunction_postMessageOverloadDispatcher(JSC::JSGlobalObject*, JSC::CallFrame*, WebCore::JSDOMWindow*)), (WebCore::CastedThisErrorBehavior)0>(JSC::JSGlobalObject&, JSC::CallFrame&, char const*)
14  0x768fd6004 WebCore::jsDOMWindowInstanceFunction_postMessage(JSC::JSGlobalObject*, JSC::CallFrame*)
15  0x48ccbce011d8
16  0x7893d21ef llint_entry
17  0x7893b0250 vmEntryToJavaScript
18  0x78a26bb2b JSC::JITCode::execute(JSC::VM*, JSC::ProtoCallFrame*)
19  0x78a26b088 JSC::Interpreter::executeProgram(JSC::SourceCode const&, JSC::JSGlobalObject*, JSC::JSObject*)
20  0x78a648847 JSC::evaluate(JSC::JSGlobalObject*, JSC::SourceCode const&, JSC::JSValue, WTF::NakedPtr<JSC::Exception>&)
21  0x78a64899a JSC::profiledEvaluate(JSC::JSGlobalObject*, JSC::ProfilingReason, JSC::SourceCode const&, JSC::JSValue, WTF::NakedPtr<JSC::Exception>&)
22  0x76b04d84c WebCore::JSExecState::profiledEvaluate(JSC::JSGlobalObject*, JSC::ProfilingReason, JSC::SourceCode const&, JSC::JSValue, WTF::NakedPtr<JSC::Exception>&)
23  0x76b04d42e WebCore::ScriptController::evaluateInWorld(WebCore::ScriptSourceCode const&, WebCore::DOMWrapperWorld&)
24  0x76b04d259 WebCore::ScriptController::evaluateInWorldIgnoringException(WebCore::ScriptSourceCode const&, WebCore::DOMWrapperWorld&)
25  0x76b04db55 WebCore::ScriptController::evaluateIgnoringException(WebCore::ScriptSourceCode const&)
26  0x76b79e7e6 WebCore::ScriptElement::executeClassicScript(WebCore::ScriptSourceCode const&)
27  0x76b79c7fb WebCore::ScriptElement::prepareScript(WTF::TextPosition const&, WebCore::ScriptElement::LegacyTypeSupport)
28  0x76bd2edb6 WebCore::HTMLScriptRunner::runScript(WebCore::ScriptElement&, WTF::TextPosition const&)
29  0x76bd2ebb7 WebCore::HTMLScriptRunner::execute(WTF::Ref<WebCore::ScriptElement, WTF::RawPtrTraits<WebCore::ScriptElement> >&&, WTF::TextPosition const&)
30  0x76bd0d401 WebCore::HTMLDocumentParser::runScriptsForPausedTreeBuilder()
31  0x76bd0d885 WebCore::HTMLDocumentParser::pumpTokenizerLoop(WebCore::HTMLDocumentParser::SynchronousMode, bool, WebCore::PumpSession&)

<rdar://68913460>

[Ryosuke Niwa]

I was able to reproduce this with debug build of WebKitTestRunner at r273504.

[Ryosuke Niwa]

Note that you need to specify __XPC_JSC_validateExceptionChecks=1 as an environment variable on macOS port. i.e. enable JSC's validateExceptionChecks option.

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

Created attachment 425618 [details]
Patch (proof-of-concept)

So I've been debugging this with the help of Caio, and he thinks CloneSerializer::serialize should check potential exceptions thrown by getOwnPropertyNames as well.

Mimicing the current approach with shouldTerminate() does not work here, because IIUC it will just rethrow the exception immediately when we create the second throw scope. Also in general, Caio thinks there could be an issue with that approach because of the Proxy object. He also wants to think and check a bit more what would be the correct approach here. I'll let him explain things better...

Anyway, here is a proof-of-concept patch that fixes the crash, so that people can comment.

[Ryosuke Niwa]

Comment on attachment 425618 [details]
Patch (proof-of-concept)

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

> Source/WebCore/bindings/js/SerializedScriptValue.cpp:1841
> +                if (scope.exception())

Can we also fix ArrayStartVisitMember and SetDataStartVisitEntry?

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

Created attachment 425736 [details]
Patch

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

Comment on attachment 425618 [details]
Patch (proof-of-concept)

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

>> Source/WebCore/bindings/js/SerializedScriptValue.cpp:1841
>> +                if (scope.exception())
> 
> Can we also fix ArrayStartVisitMember and SetDataStartVisitEntry?

Done.

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

Comment on attachment 425736 [details]
Patch

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

> Source/WebCore/ChangeLog:6
> +        Reviewed by NOBODY (OOPS!).

Same here, I don't know whether or not I should include the test.

[Yusuke Suzuki]

Comment on attachment 425736 [details]
Patch

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

> Source/WebCore/bindings/js/SerializedScriptValue.cpp:1791
> +                    if (scope.exception())

Let's put `UNLIKELY()` to these exception checks.

[Yusuke Suzuki]

Comment on attachment 425736 [details]
Patch

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

> Source/WebCore/bindings/js/SerializedScriptValue.cpp:1863
>                  inValue = getProperty(vm, object, properties[index]);
> -                if (shouldTerminate())
> +                if (scope.exception())
>                      return SerializationReturnCode::ExistingExceptionError;
>  
>                  if (!inValue) {

This is the only interesting place when considering about security related thing (whether inValue is valid or not if we ignore exception). But shouldTerminate checked exception anyway, so this is OK.

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

Created attachment 425841 [details]
Patch

Thanks for the explanation and review. here is a new version with a test.

[Yusuke Suzuki]

Comment on attachment 425841 [details]
Patch

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

> Source/WebCore/bindings/js/SerializedScriptValue.cpp:1795
>                          indexStack.append(0);

Let's insert error check after getDirectIndex (in L1804).

[Yusuke Suzuki]

Comment on attachment 425841 [details]
Patch

The other part looks good to me.

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

Created attachment 425852 [details]
Patch for landing

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

Comment on attachment 425841 [details]
Patch

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

>> Source/WebCore/bindings/js/SerializedScriptValue.cpp:1795
>>                          indexStack.append(0);
> 
> Let's insert error check after getDirectIndex (in L1804).

Done (I assumed you meant adding a new one, not moving that one... similar to the case of getProperty below)

[Yusuke Suzuki]

Comment on attachment 425841 [details]
Patch

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

>>> Source/WebCore/bindings/js/SerializedScriptValue.cpp:1795
>>>                          indexStack.append(0);
>> 
>> Let's insert error check after getDirectIndex (in L1804).
> 
> Done (I assumed you meant adding a new one, not moving that one... similar to the case of getProperty below)

Yes, adding a new one :)

[EWS]

Committed r275882 (236447@main): <https://commits.webkit.org/236447@main>

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