Bug 220355 – Missing exception check with new MediaStream(0)

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Ryosuke Niwa]

Created attachment 417073 [details]
Test

__XPC_JSC_validateExceptionChecks=1 ./Tools/Scripts/run-test-runner --debug repro_406.html

Unchecked exception detected at:
    1   0x63d74bd68 JSC::VM::verifyExceptionCheckNeedIsSatisfied(unsigned int, JSC::ExceptionEventLocation&)
    2   0x63d6ef9d7 JSC::ThrowScope::throwException(JSC::JSGlobalObject*, JSC::JSValue)
    3   0x63d2fa231 JSC::throwException(JSC::JSGlobalObject*, JSC::ThrowScope&, JSC::JSValue)
    4   0x63cec5f8b JSC::throwTypeError(JSC::JSGlobalObject*, JSC::ThrowScope&)
    5   0x5ff0ae8f3 JSC::throwVMTypeError(JSC::JSGlobalObject*, JSC::ThrowScope&)
    6   0x600f0c77e WebCore::JSDOMConstructor<WebCore::JSMediaStream>::construct(JSC::JSGlobalObject*, JSC::CallFrame*)
    7   0x63c6b13d5 JSC::NativeFunction::operator()(JSC::JSGlobalObject*, JSC::CallFrame*)
    8   0x63c6b0ed0 JSC::TaggedNativeFunction::operator()(JSC::JSGlobalObject*, JSC::CallFrame*)
    9   0x63c7ff8b2 JSC::LLInt::handleHostCall(JSC::CallFrame*, JSC::JSValue, JSC::CodeSpecializationKind)
    10  0x63c7fce1d JSC::LLInt::setUpCall(JSC::CallFrame*, JSC::CodeSpecializationKind, JSC::JSValue, JSC::LLIntCallLinkInfo*)
    11  0x63c7cd6af JSC::SlowPathReturnType JSC::LLInt::genericCall<JSC::OpConstruct>(JSC::CodeBlock*, JSC::CallFrame*, JSC::OpConstruct&&, JSC::CodeSpecializationKind, unsigned int)
    12  0x63c7cd012 llint_slow_path_construct
    13  0x639bf1c2a llint_entry
    14  0x639bcf5e2 vmEntryToJavaScript
    15  0x63c3d04a7 JSC::JITCode::execute(JSC::VM*, JSC::ProtoCallFrame*)
    16  0x63c3ce040 JSC::Interpreter::executeProgram(JSC::SourceCode const&, JSC::JSGlobalObject*, JSC::JSObject*)
    17  0x63ce5a23a JSC::evaluate(JSC::JSGlobalObject*, JSC::SourceCode const&, JSC::JSValue, WTF::NakedPtr<JSC::Exception>&)
    18  0x63ce5a721 JSC::profiledEvaluate(JSC::JSGlobalObject*, JSC::ProfilingReason, JSC::SourceCode const&, JSC::JSValue, WTF::NakedPtr<JSC::Exception>&)
    19  0x60515f639 WebCore::JSExecState::profiledEvaluate(JSC::JSGlobalObject*, JSC::ProfilingReason, JSC::SourceCode const&, JSC::JSValue, WTF::NakedPtr<JSC::Exception>&)
    20  0x60515e977 WebCore::ScriptController::evaluateInWorld(WebCore::ScriptSourceCode const&, WebCore::DOMWrapperWorld&)
    21  0x60515e393 WebCore::ScriptController::evaluateInWorldIgnoringException(WebCore::ScriptSourceCode const&, WebCore::DOMWrapperWorld&)
    22  0x60515fb0e WebCore::ScriptController::evaluateIgnoringException(WebCore::ScriptSourceCode const&)
    23  0x606208567 WebCore::ScriptElement::executeClassicScript(WebCore::ScriptSourceCode const&)
    24  0x6062035aa WebCore::ScriptElement::prepareScript(WTF::TextPosition const&, WebCore::ScriptElement::LegacyTypeSupport)
    25  0x606f40eaf WebCore::HTMLScriptRunner::runScript(WebCore::ScriptElement&, WTF::TextPosition const&)
    26  0x606f40871 WebCore::HTMLScriptRunner::execute(WTF::Ref<WebCore::ScriptElement, WTF::RawPtrTraits<WebCore::ScriptElement> >&&, WTF::TextPosition const&)
    27  0x606efac94 WebCore::HTMLDocumentParser::runScriptsForPausedTreeBuilder()
    28  0x606efb504 WebCore::HTMLDocumentParser::pumpTokenizerLoop(WebCore::HTMLDocumentParser::SynchronousMode, bool, WebCore::PumpSession&)
    29  0x606ef9dbd WebCore::HTMLDocumentParser::pumpTokenizer(WebCore::HTMLDocumentParser::SynchronousMode)
    30  0x606ef92f6 WebCore::HTMLDocumentParser::pumpTokenizerIfPossible(WebCore::HTMLDocumentParser::SynchronousMode)
    31  0x606efd226 WebCore::HTMLDocumentParser::append(WTF::RefPtr<WTF::StringImpl, WTF::RawPtrTraits<WTF::StringImpl>, WTF::DefaultRefDerefTraits<WTF::StringImpl> >&&)
    32  0x605d90baa WebCore::DecodedDataDocumentParser::flush(WebCore::DocumentWriter&)
    33  0x6079ab4a5 WebCore::DocumentWriter::end()
    34  0x60790c6cd WebCore::DocumentLoader::finishedLoading()
    35  0x60790bae0 WebCore::DocumentLoader::notifyFinished(WebCore::CachedResource&, WebCore::NetworkLoadMetrics const&)
    36  0x607c653d7 WebCore::CachedResource::checkNotify(WebCore::NetworkLoadMetrics const&)
    37  0x607c5ba58 WebCore::CachedResource::finishLoading(WebCore::SharedBuffer*, WebCore::NetworkLoadMetrics const&)
    38  0x607c5e704 WebCore::CachedRawResource::finishLoading(WebCore::SharedBuffer*, WebCore::NetworkLoadMetrics const&)
    39  0x607b5906f WebCore::SubresourceLoader::didFinishLoading(WebCore::NetworkLoadMetrics const&)
    40  0x5ebdace68 WebKit::WebResourceLoader::didFinishResourceLoad(WebCore::NetworkLoadMetrics const&)
    41  0x5ecae0a87 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>)
    42  0x5ecae076e 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&))
    43  0x5ecadae62 void IPC::handleMessage<Messages::WebResourceLoader::DidFinishResourceLoad, WebKit::WebResourceLoader, void (WebKit::WebResourceLoader::*)(WebCore::NetworkLoadMetrics const&)>(IPC::Decoder&, WebKit::WebResourceLoader*, void (WebKit::WebResourceLoader::*)(WebCore::NetworkLoadMetrics const&))
    44  0x5ecad929b WebKit::WebResourceLoader::didReceiveWebResourceLoaderMessage(IPC::Connection&, IPC::Decoder&)
    45  0x5ebd8bf3f WebKit::NetworkProcessConnection::didReceiveMessage(IPC::Connection&, IPC::Decoder&)
    46  0x5e8120acd IPC::Connection::dispatchMessage(IPC::Decoder&)
    47  0x5e812353b IPC::Connection::dispatchMessage(std::__1::unique_ptr<IPC::Decoder, std::__1::default_delete<IPC::Decoder> >)
    48  0x5e8125235 IPC::Connection::dispatchOneIncomingMessage()
    49  0x5e816c798 IPC::Connection::enqueueIncomingMessage(std::__1::unique_ptr<IPC::Decoder, std::__1::default_delete<IPC::Decoder> >)::$_8::operator()()
    50  0x5e816c67e WTF::Detail::CallableWrapper<IPC::Connection::enqueueIncomingMessage(std::__1::unique_ptr<IPC::Decoder, std::__1::default_delete<IPC::Decoder> >)::$_8, void>::call()
    51  0x6389f1fe5 WTF::Function<void ()>::operator()() const
    52  0x638b387de WTF::RunLoop::performWork()
    53  0x638b3ffd6 WTF::RunLoop::performWork(void*)
    54  0x7fff20462a0c __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__
    55  0x7fff20462974 __CFRunLoopDoSource0
    56  0x7fff204626ef __CFRunLoopDoSources0
    57  0x7fff20461121 __CFRunLoopRun
    58  0x7fff204606ce CFRunLoopRunSpecific
    59  0x7fff211edfa1 -[NSRunLoop(NSRunLoop) runMode:beforeDate:]
    60  0x7fff2127c384 -[NSRunLoop(NSRunLoop) run]
    61  0x7fff200b63dd _xpc_objc_main
    62  0x7fff200b5e65 _xpc_copy_xpcservice_dictionary
    63  0x5e9bfe178 WebKit::XPCServiceMain(int, char const**)
    64  0x5ecc1173b WKXPCServiceMain
    65  0x100298e12 main
    66  0x7fff20385621 start
    67  0x1

ASSERTION FAILED: !m_needExceptionCheck
./runtime/VM.cpp(1416) : void JSC::VM::verifyExceptionCheckNeedIsSatisfied(unsigned int, JSC::ExceptionEventLocation &)
1   0x63898df49 WTFCrash
2   0x63a293180 JSC::B3::PatchpointSpecial::admitsExtendedOffsetAddr(JSC::B3::Air::Inst&, unsigned int)
3   0x63d74bf49 JSC::VM::verifyExceptionCheckNeedIsSatisfied(unsigned int, JSC::ExceptionEventLocation&)
4   0x63d6ef9d7 JSC::ThrowScope::throwException(JSC::JSGlobalObject*, JSC::JSValue)
5   0x63d2fa231 JSC::throwException(JSC::JSGlobalObject*, JSC::ThrowScope&, JSC::JSValue)
6   0x63cec5f8b JSC::throwTypeError(JSC::JSGlobalObject*, JSC::ThrowScope&)
7   0x5ff0ae8f3 JSC::throwVMTypeError(JSC::JSGlobalObject*, JSC::ThrowScope&)
8   0x600f0c77e WebCore::JSDOMConstructor<WebCore::JSMediaStream>::construct(JSC::JSGlobalObject*, JSC::CallFrame*)
9   0x63c6b13d5 JSC::NativeFunction::operator()(JSC::JSGlobalObject*, JSC::CallFrame*)
10  0x63c6b0ed0 JSC::TaggedNativeFunction::operator()(JSC::JSGlobalObject*, JSC::CallFrame*)
11  0x63c7ff8b2 JSC::LLInt::handleHostCall(JSC::CallFrame*, JSC::JSValue, JSC::CodeSpecializationKind)
12  0x63c7fce1d JSC::LLInt::setUpCall(JSC::CallFrame*, JSC::CodeSpecializationKind, JSC::JSValue, JSC::LLIntCallLinkInfo*)
13  0x63c7cd6af JSC::SlowPathReturnType JSC::LLInt::genericCall<JSC::OpConstruct>(JSC::CodeBlock*, JSC::CallFrame*, JSC::OpConstruct&&, JSC::CodeSpecializationKind, unsigned int)
14  0x63c7cd012 llint_slow_path_construct
15  0x639bf1c2a llint_entry
16  0x639bcf5e2 vmEntryToJavaScript
17  0x63c3d04a7 JSC::JITCode::execute(JSC::VM*, JSC::ProtoCallFrame*)
18  0x63c3ce040 JSC::Interpreter::executeProgram(JSC::SourceCode const&, JSC::JSGlobalObject*, JSC::JSObject*)
19  0x63ce5a23a JSC::evaluate(JSC::JSGlobalObject*, JSC::SourceCode const&, JSC::JSValue, WTF::NakedPtr<JSC::Exception>&)
20  0x63ce5a721 JSC::profiledEvaluate(JSC::JSGlobalObject*, JSC::ProfilingReason, JSC::SourceCode const&, JSC::JSValue, WTF::NakedPtr<JSC::Exception>&)
21  0x60515f639 WebCore::JSExecState::profiledEvaluate(JSC::JSGlobalObject*, JSC::ProfilingReason, JSC::SourceCode const&, JSC::JSValue, WTF::NakedPtr<JSC::Exception>&)
22  0x60515e977 WebCore::ScriptController::evaluateInWorld(WebCore::ScriptSourceCode const&, WebCore::DOMWrapperWorld&)
23  0x60515e393 WebCore::ScriptController::evaluateInWorldIgnoringException(WebCore::ScriptSourceCode const&, WebCore::DOMWrapperWorld&)
24  0x60515fb0e WebCore::ScriptController::evaluateIgnoringException(WebCore::ScriptSourceCode const&)
25  0x606208567 WebCore::ScriptElement::executeClassicScript(WebCore::ScriptSourceCode const&)
26  0x6062035aa WebCore::ScriptElement::prepareScript(WTF::TextPosition const&, WebCore::ScriptElement::LegacyTypeSupport)
27  0x606f40eaf WebCore::HTMLScriptRunner::runScript(WebCore::ScriptElement&, WTF::TextPosition const&)
28  0x606f40871 WebCore::HTMLScriptRunner::execute(WTF::Ref<WebCore::ScriptElement, WTF::RawPtrTraits<WebCore::ScriptElement> >&&, WTF::TextPosition const&)
29  0x606efac94 WebCore::HTMLDocumentParser::runScriptsForPausedTreeBuilder()
30  0x606efb504 WebCore::HTMLDocumentParser::pumpTokenizerLoop(WebCore::HTMLDocumentParser::SynchronousMode, bool, WebCore::PumpSession&)
31  0x606ef9dbd WebCore::HTMLDocumentParser::pumpTokenizer(WebCore::HTMLDocumentParser::SynchronousMode)
#CRASHED - com.apple.WebKit.WebContent.Development (pid 78792)

<rdar://problem/69490078>

[Carlos Garcia Campos]

Created attachment 417835 [details]
Patch

[Mark Lam]

Comment on attachment 417835 [details]
Patch

Can you add the test in the patch as well?

[Ryosuke Niwa]

(In reply to Mark Lam from comment #2)
> Comment on attachment 417835 [details]
> Patch
> 
> Can you add the test in the patch as well?

Does this missing exception check have a security implication? If so, we can't check in the test.

[Yusuke Suzuki]

(In reply to Ryosuke Niwa from comment #3)
> (In reply to Mark Lam from comment #2)
> > Comment on attachment 417835 [details]
> > Patch
> > 
> > Can you add the test in the patch as well?
> 
> Does this missing exception check have a security implication? If so, we
> can't check in the test.

Let's attach the test to this bug. We will just land it into Internal repository.

[Carlos Garcia Campos]

I thought about the test, but I was not sure how to do it. It requires ENABLE_EXCEPTION_SCOPE_VERIFICATION to be enabled, which is the default for debug builds, but it also needs JSC_validateExceptionChecks=1. I guess we can always set the env var from WTR, or at least when --debug is passed.

[Carlos Garcia Campos]

This is already covered by fast/mediastream/MediaStreamConstructor.html but we need to run it with JSC_validateExceptionChecks=1 to make it crash in debug.

[Carlos Garcia Campos]

I suspect there might be more existing tests crashing with JSC_validateExceptionChecks=1, so I'll file a new bug report to set the env var for debug config and see what EWS says.

[Mark Lam]

(In reply to Carlos Garcia Campos from comment #6)
> This is already covered by fast/mediastream/MediaStreamConstructor.html but
> we need to run it with JSC_validateExceptionChecks=1 to make it crash in
> debug.

To enable the option in just 1 test, you can use `[ jscOptions=--validateExceptionChecks=true ]`.  See LayoutTests/js/dom/promise-should-have-exception-check-on-operation.html for an example.

The reason we haven't turned it on for all debug build runs is because there are a lot more failures that need to be fixed first before we can do that (to avoid red test bots).  You're welcome to try to fix them all though and turn on the flag for all debug builds.

[Carlos Garcia Campos]

(In reply to Mark Lam from comment #8)
> (In reply to Carlos Garcia Campos from comment #6)
> > This is already covered by fast/mediastream/MediaStreamConstructor.html but
> > we need to run it with JSC_validateExceptionChecks=1 to make it crash in
> > debug.
> 
> To enable the option in just 1 test, you can use `[
> jscOptions=--validateExceptionChecks=true ]`.  See
> LayoutTests/js/dom/promise-should-have-exception-check-on-operation.html for
> an example.

Great, that should work for this particular case then. I guess that should be done in a separate commit in any case, right?

> The reason we haven't turned it on for all debug build runs is because there
> are a lot more failures that need to be fixed first before we can do that
> (to avoid red test bots).  You're welcome to try to fix them all though and
> turn on the flag for all debug builds.

Indeed, see bug #220790.

[Carlos Garcia Campos]

So, can we land this fix then? There's no new test for this, we just need to enable JSC_validateExceptionChecks for fast/mediastream/MediaStreamConstructor.html in a follow up.

[Mark Lam]

Comment on attachment 417835 [details]
Patch

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

Please rebase the bindings test after applying the fix below.

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3669
>          push(@implContent, "#if ${conditionalString}\n") if $conditionalString;
>          push(@implContent, "        if ($condition)\n    ") if $condition;
>          push(@implContent, "        RELEASE_AND_RETURN(throwScope, (" . $overloadFunctionPrefix . $overload->{overloadIndex} . $overloadFunctionSuffix . "(${parametersToForward})));\n");
> +        push(@implContent, "        RETURN_IF_EXCEPTION(throwScope, { });\n") if $canThrow;

This fix is not correct.  The exception is potentially thrown by the evaluation of $condition.  And then the if clause can call another function that may also throw.  Hence, the exception check needs to appear before the second function call.  Also, I recommend renaming $canThrow to $conditionCanThrow to make it clear that this bool only covers a throw from the condition.

The correct fix is to do this instead:

        if ($condition && $conditionCanThrow) {
            push(@implContent, "        {\n ");
            push(@implContent, "            bool success = $condition;\n ");
            push(@implContent, "            RETURN_IF_EXCEPTION(throwScope, { });\n");
            push(@implContent, "            if (success)\n ");
            push(@implContent, "                RELEASE_AND_RETURN(throwScope, (" . $overloadFunctionPrefix . $overload->{overloadIndex} . $overloadFunctionSuffix . "
            push(@implContent, "        }\n ");
        } elsif ($condition) {
            push(@implContent, "        if ($condition)\n ")
            push(@implContent, "            RELEASE_AND_RETURN(throwScope, (" . $overloadFunctionPrefix . $overload->{overloadIndex} . $overloadFunctionSuffix . "
        } else {
            push(@implContent, "        RELEASE_AND_RETURN(throwScope, (" . $overloadFunctionPrefix . $overload->{overloadIndex} . $overloadFunctionSuffix . "
        }

[Mark Lam]

Comment on attachment 417835 [details]
Patch

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

>> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3669
>> +        push(@implContent, "        RETURN_IF_EXCEPTION(throwScope, { });\n") if $canThrow;
> 
> This fix is not correct.  The exception is potentially thrown by the evaluation of $condition.  And then the if clause can call another function that may also throw.  Hence, the exception check needs to appear before the second function call.  Also, I recommend renaming $canThrow to $conditionCanThrow to make it clear that this bool only covers a throw from the condition.
> 
> The correct fix is to do this instead:
> 
>         if ($condition && $conditionCanThrow) {
>             push(@implContent, "        {\n ");
>             push(@implContent, "            bool success = $condition;\n ");
>             push(@implContent, "            RETURN_IF_EXCEPTION(throwScope, { });\n");
>             push(@implContent, "            if (success)\n ");
>             push(@implContent, "                RELEASE_AND_RETURN(throwScope, (" . $overloadFunctionPrefix . $overload->{overloadIndex} . $overloadFunctionSuffix . "
>             push(@implContent, "        }\n ");
>         } elsif ($condition) {
>             push(@implContent, "        if ($condition)\n ")
>             push(@implContent, "            RELEASE_AND_RETURN(throwScope, (" . $overloadFunctionPrefix . $overload->{overloadIndex} . $overloadFunctionSuffix . "
>         } else {
>             push(@implContent, "        RELEASE_AND_RETURN(throwScope, (" . $overloadFunctionPrefix . $overload->{overloadIndex} . $overloadFunctionSuffix . "
>         }

Oops, I'm missing the `(${parametersToForward})));\n");` at the end of the RELEASE_AND_RETURN in each case of the above 3 cases.  Please add.

[Carlos Garcia Campos]

Created attachment 418849 [details]
Patch

[Mark Lam]

Comment on attachment 418849 [details]
Patch

r=me

[EWS]

Committed r272199: <https://trac.webkit.org/changeset/272199>

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

[Ryosuke Niwa]

Can we add a test since we don't believe this is a security issue?

[Ryosuke Niwa]

(In reply to Ryosuke Niwa from comment #16)
> Can we add a test since we don't believe this is a security issue?

Oh never mind. Forgot that we already had an existing test for this.