Bug 220355

Summary: Missing exception check with new MediaStream(0)
Product: WebKit Reporter: Ryosuke Niwa <rniwa>
Component: MediaAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, cgarcia, ews-feeder, fred.wang, gpoo, mark.lam, product-security, rbuis, svillar, webkit-bug-importer, ysuzuki
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=220790
Attachments:
Description Flags
Test
none
Patch
mark.lam: review-
Patch none

Ryosuke Niwa
Reported 2021-01-05 23:25:23 PST
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>
Attachments
Test (41 bytes, text/html)
2021-01-05 23:25 PST, Ryosuke Niwa 		no flags
Details
Patch (5.17 KB, patch)
2021-01-18 08:27 PST, Carlos Garcia Campos 		mark.lam: review-
DetailsFormatted DiffDiff
Patch (7.03 KB, patch)
2021-02-01 02:17 PST, Carlos Garcia Campos 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Carlos Garcia Campos
Comment 1 2021-01-18 08:27:03 PST
Created attachment 417835 [details] Patch
Mark Lam
Comment 2 2021-01-18 12:22:51 PST
Comment on attachment 417835 [details] Patch Can you add the test in the patch as well?
Ryosuke Niwa
Comment 3 2021-01-18 14:00:25 PST
(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
Comment 4 2021-01-18 18:28:46 PST
(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
Comment 5 2021-01-19 00:28:35 PST
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
Comment 6 2021-01-21 02:42:05 PST
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
Comment 7 2021-01-21 02:44:12 PST
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
Comment 8 2021-01-21 10:23:31 PST
(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
Comment 9 2021-01-22 00:49:47 PST
(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
Comment 10 2021-01-27 00:57:32 PST
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 11 2021-01-27 20:17:43 PST
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 12 2021-01-27 20:20:16 PST
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
Comment 13 2021-02-01 02:17:22 PST
Created attachment 418849 [details] Patch
Mark Lam
Comment 14 2021-02-01 11:32:21 PST
Comment on attachment 418849 [details] Patch r=me
EWS
Comment 15 2021-02-02 01:33:41 PST
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
Comment 16 2021-02-04 20:52:48 PST
Can we add a test since we don't believe this is a security issue?
Ryosuke Niwa
Comment 17 2021-02-04 20:55:02 PST
(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.
Note You need to log in before you can comment on or make changes to this bug.