| Summary: | Bad optional access in WebCore::ContentSecurityPolicySource::portMatches | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Michael Catanzaro <mcatanzaro> | ||||||
| Component: | WebCore Misc. | Assignee: | Michael Catanzaro <mcatanzaro> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | bugs-noreply, calvaris, commit-queue, dbates, ews-watchlist, mcatanzaro, mkwst, webkit-bug-importer | ||||||
| Priority: | P2 | Keywords: | InRadar | ||||||
| Version: | WebKit Nightly Build | ||||||||
| Hardware: | PC | ||||||||
| OS: | Linux | ||||||||
| See Also: | https://bugs.webkit.org/show_bug.cgi?id=186536 | ||||||||
| Attachments: |
|
||||||||
|
Description
Michael Catanzaro
2018-06-11 12:02:42 PDT
Here's a better backtrace. It's built with -g1 so I don't have local variables, but the problem is clear enough: we now use libstdc++'s std::optional instead of WTF's implementation. The WTF implementation returns harmlessly when the std::optional is set to nullopt. But the standard std::optional throws. Then we abort immediately due to -fno-exceptions.
(gdb) bt
#0 __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50
#1 0x00007fe0c6dd64ac in __GI_abort () at abort.c:79
#2 0x00007fe0d3784925 in std::__throw_bad_optional_access () at /usr/include/c++/8.1.0/optional:98
#3 std::optional<unsigned short>::value() const & () at /usr/include/c++/8.1.0/optional:1251
#4 WebCore::ContentSecurityPolicySource::portMatches ()
at /run/build-runtime/WebKitGTK+/Source/WebCore/page/csp/ContentSecurityPolicySource.cpp:95
#5 0x00007fe0d37874e8 in WebCore::ContentSecurityPolicySource::matches ()
at /run/build-runtime/WebKitGTK+/Source/WebCore/page/csp/ContentSecurityPolicySource.cpp:53
#6 0x00007fe0d37900fb in WebCore::ContentSecurityPolicySourceList::matches ()
at /run/build-runtime/WebKitGTK+/Source/WebCore/page/csp/ContentSecurityPolicySourceList.cpp:138
#7 0x00007fe0d3783b7b in checkSource ()
at /run/build-runtime/WebKitGTK+/Source/WebCore/page/csp/ContentSecurityPolicyDirectiveList.cpp:60
#8 WebCore::ContentSecurityPolicyDirectiveList::violatedDirectiveForConnectSource ()
at /run/build-runtime/WebKitGTK+/Source/WebCore/page/csp/ContentSecurityPolicyDirectiveList.cpp:222
#9 0x00007fe0d378dc9b in WebCore::ContentSecurityPolicy::allPoliciesAllow<WebCore::ContentSecurityPolicyDirective const* (WebCore::ContentSecurityPolicyDirectiveList::*)(WebCore::URL const&, bool) const, WebCore::URL const&, bool>(std::function<void (WebCore::ContentSecurityPolicyDirective const&)>&&, WebCore::ContentSecurityPolicyDirective const* (WebCore::ContentSecurityPolicyDirectiveList::*&&)(WebCore::URL const&, bool) const, WebCore::URL const&, bool&&) const ()
at /run/build-runtime/WebKitGTK+/Source/WebCore/page/csp/ContentSecurityPolicy.cpp:321
#10 0x00007fe0d37858e8 in WebCore::ContentSecurityPolicy::allowConnectToSource ()
at /run/build-runtime/WebKitGTK+/Source/WebCore/page/csp/ContentSecurityPolicy.cpp:612
#11 0x00007fe0d36e81b7 in WebCore::EventSource::create ()
at /run/build-runtime/WebKitGTK+/Source/WebCore/page/EventSource.cpp:71
#12 0x00007fe0d2b1f58f in WebCore::JSDOMConstructor<WebCore::JSEventSource>::construct ()
at /run/build-runtime/WebKitGTK+/DerivedSources/WebCore/JSEventSource.cpp:145
#13 0x00007fe0d0f9f53b in JSC::NativeFunction::operator() ()
at /run/build-runtime/WebKitGTK+/Source/JavaScriptCore/runtime/NativeFunction.h:50
#14 JSC::TaggedNativeFunction::operator() ()
at /run/build-runtime/WebKitGTK+/Source/JavaScriptCore/runtime/NativeFunction.h:84
#15 handleHostCall ()
at /run/build-runtime/WebKitGTK+/Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:1402
#16 0x00007fe0d0f9f802 in JSC::LLInt::setUpCall ()
at /run/build-runtime/WebKitGTK+/Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:1449
#17 0x00007fe0d0f9e2c5 in llint_entry () from /usr/lib/x86_64-linux-gnu/libjavascriptcoregtk-4.0.so.18
#18 0x00007fe0d0f97613 in vmEntryToJavaScript ()
from /usr/lib/x86_64-linux-gnu/libjavascriptcoregtk-4.0.so.18
#19 0x00007fe0d0eef6b7 in JSC::JITCode::execute ()
at /run/build-runtime/WebKitGTK+/Source/JavaScriptCore/jit/JITCodeInlines.h:38
#20 JSC::Interpreter::executeProgram ()
at /run/build-runtime/WebKitGTK+/Source/JavaScriptCore/interpreter/Interpreter.cpp:964
#21 0x00007fe0d1103cf1 in JSC::evaluate ()
at /run/build-runtime/WebKitGTK+/Source/JavaScriptCore/runtime/Completion.cpp:103
#22 0x00007fe0d1103e84 in JSC::profiledEvaluate ()
at /run/build-runtime/WebKitGTK+/Source/JavaScriptCore/runtime/Completion.cpp:118
#23 0x00007fe0d30c8a4d in WebCore::JSMainThreadExecState::profiledEvaluate ()
at /run/build-runtime/WebKitGTK+/Source/WebCore/bindings/js/JSMainThreadExecState.h:78
#24 WebCore::ScriptController::evaluateInWorld ()
at /run/build-runtime/WebKitGTK+/Source/WebCore/bindings/js/ScriptController.cpp:130
#25 0x00007fe0d33175c9 in WebCore::ScriptElement::executeClassicScript ()
at /run/build-runtime/WebKitGTK+/Source/WebCore/dom/ScriptElement.cpp:387
#26 0x00007fe0d33228d7 in WebCore::ScriptElement::prepareScript ()
at /run/build-runtime/WebKitGTK+/Source/WebCore/dom/ScriptElement.cpp:267
#27 0x00007fe0d3533e81 in WebCore::HTMLScriptRunner::runScript ()
at /run/build-runtime/WebKitGTK+/Source/WebCore/html/parser/HTMLScriptRunner.cpp:250
#28 0x00007fe0d3534b3e in WebCore::HTMLScriptRunner::execute ()
at /run/build-runtime/WebKitGTK+/Source/WebCore/html/parser/HTMLScriptRunner.cpp:140
#29 0x00007fe0d351f032 in WebCore::HTMLDocumentParser::runScriptsForPausedTreeBuilder ()
at /run/build-runtime/WebKitGTK+/Source/WebCore/html/parser/HTMLDocumentParser.cpp:212
#30 0x00007fe0d351f1a9 in WebCore::HTMLDocumentParser::pumpTokenizerLoop ()
at /run/build-runtime/WebKitGTK+/Source/WebCore/html/parser/HTMLDocumentParser.cpp:231
#31 0x00007fe0d351f3ee in WebCore::HTMLDocumentParser::pumpTokenizer ()
at /run/build-runtime/WebKitGTK+/Source/WebCore/html/parser/HTMLDocumentParser.cpp:281
#32 0x00007fe0d3521bef in WebCore::HTMLDocumentParser::resumeParsingAfterYield ()
at /run/build-runtime/WebKitGTK+/Source/WebCore/html/parser/HTMLDocumentParser.cpp:189
#33 0x00007fe0d37e05ee in WebCore::ThreadTimers::sharedTimerFiredInternal ()
at /run/build-runtime/WebKitGTK+/Source/WebCore/platform/ThreadTimers.cpp:117
#34 0x00007fe0d1432d93 in operator() ()
at /run/build-runtime/WebKitGTK+/Source/WTF/wtf/glib/RunLoopGLib.cpp:170
#35 _FUN () at /run/build-runtime/WebKitGTK+/Source/WTF/wtf/glib/RunLoopGLib.cpp:176
#36 0x00007fe0c97e7448 in g_main_dispatch (context=0x561130776320) at gmain.c:3176
#37 g_main_context_dispatch (context=context@entry=0x561130776320) at gmain.c:3829
#38 0x00007fe0c97e7838 in g_main_context_iterate (context=0x561130776320, block=block@entry=1,
dispatch=dispatch@entry=1, self=<optimized out>) at gmain.c:3902
#39 0x00007fe0c97e7b32 in g_main_loop_run (loop=0x561130802340) at gmain.c:4098
#40 0x00007fe0d14331f0 in WTF::RunLoop::run ()
at /run/build-runtime/WebKitGTK+/Source/WTF/wtf/glib/RunLoopGLib.cpp:96
#41 0x00007fe0d2a429b8 in WebKit::ChildProcessMain<WebKit::WebProcess, WebKit::WebProcessMain> ()
at /run/build-runtime/WebKitGTK+/Source/WebKit/Shared/unix/ChildProcessMain.h:61
#42 0x00007fe0c6dd80cb in __libc_start_main (main=0x56112e85ac80 <main()>, argc=3, argv=0x7ffd1f282ee8,
init=<optimized out>, fini=<optimized out>, rtld_fini=<optimized out>, stack_end=0x7ffd1f282ed8)
at ../csu/libc-start.c:308
#43 0x000056112e85ad0a in _start () at ../sysdeps/x86_64/start.S:120
Reported bug #186536 to hopefully help surface these. Problem is here: if (isDefaultPortForProtocol(m_port.value(), "http") && ((!port && url.protocolIs("https")) || isDefaultPortForProtocol(port.value(), "https"))) return true; which is wrong because m_port.value() is used unsafely without a call to m_port.has_value(), and port.value() is used unsafely without a call to port.has_value(). Crash occurs when url=https://pagure.io:8088/fedora-workstation/issue/42. The CSP is on this page is: Content-Security-Policy: default-src 'self' https:; script-src 'self' 'unsafe-eval' 'unsafe-inline' https://apps.fedoraproject.org; style-src 'self' 'unsafe-inline' https://apps.fedoraproject.org But that's almost irrelevant, except to note that it doesn't include a URL with port 8088. In the usual case, the function returns earlier because port == m_port and there is no crash. Writing a layout test seems difficult because the test server listens on 8080, so the same CSP works fine under WebKitTestRunner. Created attachment 342471 [details]
Patch
Comment on attachment 342471 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=342471&action=review > Source/WebCore/ChangeLog:11 > + This is hard to test. If the layout test script-src-parsing-implicit-and-explicit-port-number > + continues to pass for WebKitLegacy, then I have at least probably not broken anything. To The test is passing! Comment on attachment 342471 [details] Patch Attachment 342471 [details] did not pass win-ews (win): Output: http://webkit-queues.webkit.org/results/8163556 New failing tests: http/tests/security/video-poster-cross-origin-crash2.html Created attachment 342661 [details]
Archive of layout-test-results from ews204 for win-future
The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews204 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Daniel, do you want to review this one? Ping any WebKit reviewers, this one is critical (In reply to Michael Catanzaro from comment #8) > Ping any WebKit reviewers, this one is critical Ping again. This can be handled by any reviewer. You can note: * That if either of the checks I added to this conditional are not satisfied, then the code will immediately crash * That the test added along with the code that added this conditional is still passing The crash can be reproduced by visiting https://pagure.io/fedora-workstation/issue/60 on a non-Cocoa port, when built with GCC 7 or GCC 8. Third ping, for any reviewer. Please, I need to use pagure. :P Comment on attachment 342471 [details]
Patch
r=me
Comment on attachment 342471 [details]
Patch
Thanks!
Comment on attachment 342471 [details] Patch Clearing flags on attachment: 342471 Committed r233036: <https://trac.webkit.org/changeset/233036> All reviewed patches have been landed. Closing bug. |