Bug 201597 - Harden JSC against the abuse of runtime options.
Summary: Harden JSC against the abuse of runtime options.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Lam
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-09-08 18:05 PDT by Mark Lam
Modified: 2020-03-18 14:01 PDT (History)
18 users (show)

See Also:


Attachments
work in progress for EWS testing. (184.57 KB, patch)
2019-09-08 18:13 PDT, Mark Lam
no flags Details | Formatted Diff | Diff
work in progress for EWS testing. (204.32 KB, patch)
2019-09-09 19:59 PDT, Mark Lam
no flags Details | Formatted Diff | Diff
work in progress for EWS testing. (204.16 KB, patch)
2019-09-09 20:13 PDT, Mark Lam
no flags Details | Formatted Diff | Diff
work in progress for EWS testing. (205.51 KB, patch)
2019-09-10 12:48 PDT, Mark Lam
no flags Details | Formatted Diff | Diff
work in progress for EWS testing. (206.40 KB, patch)
2019-09-10 15:20 PDT, Mark Lam
ews-watchlist: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews215 for win-future (13.25 MB, application/zip)
2019-09-10 17:47 PDT, EWS Watchlist
no flags Details
patch for review. (224.70 KB, patch)
2019-09-10 20:44 PDT, Mark Lam
ews-watchlist: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews213 for win-future (92.68 KB, application/zip)
2019-09-10 22:33 PDT, EWS Watchlist
no flags Details
proposed patch. (224.71 KB, patch)
2019-09-10 22:47 PDT, Mark Lam
no flags Details | Formatted Diff | Diff
proposed patch. (224.76 KB, patch)
2019-09-10 23:39 PDT, Mark Lam
ews-watchlist: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews210 for win-future (12.98 MB, application/zip)
2019-09-11 01:32 PDT, EWS Watchlist
no flags Details
Patch for GTK and WPE (6.08 KB, patch)
2019-09-11 05:28 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
proposed patch. (242.21 KB, patch)
2019-09-11 16:32 PDT, Mark Lam
no flags Details | Formatted Diff | Diff
proposed patch. (242.21 KB, patch)
2019-09-11 17:34 PDT, Mark Lam
ews-watchlist: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews210 for win-future (13.13 MB, application/zip)
2019-09-11 20:19 PDT, EWS Watchlist
no flags Details
Patch for Windows (551 bytes, patch)
2019-09-11 21:32 PDT, Fujii Hironori
no flags Details | Formatted Diff | Diff
proposed patch. (242.90 KB, patch)
2019-09-11 23:17 PDT, Mark Lam
fpizlo: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Lam 2019-09-08 18:05:47 PDT
Details to follow.
Comment 1 Radar WebKit Bug Importer 2019-09-08 18:06:24 PDT
<rdar://problem/55167068>
Comment 2 Mark Lam 2019-09-08 18:13:42 PDT
Created attachment 378342 [details]
work in progress for EWS testing.
Comment 3 Mark Lam 2019-09-09 19:59:32 PDT
Created attachment 378434 [details]
work in progress for EWS testing.
Comment 4 Mark Lam 2019-09-09 20:13:44 PDT
Created attachment 378435 [details]
work in progress for EWS testing.
Comment 5 Mark Lam 2019-09-10 12:48:03 PDT
Created attachment 378477 [details]
work in progress for EWS testing.
Comment 6 Mark Lam 2019-09-10 15:20:32 PDT
Created attachment 378499 [details]
work in progress for EWS testing.
Comment 7 EWS Watchlist 2019-09-10 17:47:26 PDT
Comment on attachment 378499 [details]
work in progress for EWS testing.

Attachment 378499 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/13021231

Number of test failures exceeded the failure limit.
Comment 8 EWS Watchlist 2019-09-10 17:47:39 PDT
Created attachment 378517 [details]
Archive of layout-test-results from ews215 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews215  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit
Comment 9 Mark Lam 2019-09-10 20:44:12 PDT
Created attachment 378534 [details]
patch for review.
Comment 10 Tim Horton 2019-09-10 21:35:09 PDT
Comment on attachment 378534 [details]
patch for review.

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

The parts above JSC look fine.

> Source/WebKit/ChangeLog:11
> +        2. Removed the call enable Options::useBigInt in WebInspectorUI.
> +           WebInspectorUI doesn't really need it for now.

I assume inspector people are OK with this?

> Tools/TestWebKitAPI/Tests/WebKitCocoa/WebsiteDataStoreCustomPaths.mm:621
> +    JSC::Config::configureForTesting();

What's the trick here? This is a WK2 test, and this is the UI process, so why do you need this/does this have any effect?
Comment 11 Mark Lam 2019-09-10 22:09:20 PDT
Comment on attachment 378534 [details]
patch for review.

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

>> Source/WebKit/ChangeLog:11
>> +           WebInspectorUI doesn't really need it for now.
> 
> I assume inspector people are OK with this?

Yes, this was suggested by the Inspector folks.

>> Tools/TestWebKitAPI/Tests/WebKitCocoa/WebsiteDataStoreCustomPaths.mm:621
>> +    JSC::Config::configureForTesting();
> 
> What's the trick here? This is a WK2 test, and this is the UI process, so why do you need this/does this have any effect?

Are you sure this one is WK2?  This test was crashing without this change.  I didn't dig too deeply.  Adding this here made the crash go away.
Comment 12 Mark Lam 2019-09-10 22:13:01 PDT
Comment on attachment 378534 [details]
patch for review.

I must have broken something after I rebased and added some finishing touches.  Taking this out of review while I investigate and fix.
Comment 13 EWS Watchlist 2019-09-10 22:33:20 PDT
Comment on attachment 378534 [details]
patch for review.

Attachment 378534 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/13022038

Number of test failures exceeded the failure limit.
Comment 14 EWS Watchlist 2019-09-10 22:33:22 PDT
Created attachment 378540 [details]
Archive of layout-test-results from ews213 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews213  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit
Comment 15 Mark Lam 2019-09-10 22:47:32 PDT
Created attachment 378541 [details]
proposed patch.
Comment 16 Mark Lam 2019-09-10 23:39:50 PDT
Created attachment 378542 [details]
proposed patch.
Comment 17 EWS Watchlist 2019-09-11 01:32:35 PDT
Comment on attachment 378542 [details]
proposed patch.

Attachment 378542 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/13022381

Number of test failures exceeded the failure limit.
Comment 18 EWS Watchlist 2019-09-11 01:32:38 PDT
Created attachment 378545 [details]
Archive of layout-test-results from ews210 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews210  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit
Comment 19 Carlos Garcia Campos 2019-09-11 05:28:18 PDT
Created attachment 378556 [details]
Patch for GTK and WPE

I think this is enough
Comment 20 Mark Lam 2019-09-11 07:33:25 PDT
(In reply to Carlos Garcia Campos from comment #19)
> Created attachment 378556 [details]
> Patch for GTK and WPE
> 
> I think this is enough

Thanks.  I'll apply the patch.
Comment 21 Mark Lam 2019-09-11 16:32:48 PDT
Created attachment 378595 [details]
proposed patch.
Comment 22 Tim Horton 2019-09-11 17:21:27 PDT
Comment on attachment 378595 [details]
proposed patch.

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

> Source/WebKit/UIProcess/API/Cocoa/_WKProcessPoolConfiguration.h:34
> +@property (nonatomic) BOOL configureJSCForTesting;

This needs availability annotations (also it's a bit odd to add it at the top)
Comment 23 Mark Lam 2019-09-11 17:24:24 PDT
(In reply to Tim Horton from comment #22)
> Comment on attachment 378595 [details]
> proposed patch.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=378595&action=review
> 
> > Source/WebKit/UIProcess/API/Cocoa/_WKProcessPoolConfiguration.h:34
> > +@property (nonatomic) BOOL configureJSCForTesting;
> 
> This needs availability annotations (also it's a bit odd to add it at the
> top)

Thanks.  Will add availability annotations.  I can also move it elsewhere.  I only put there because I modeled it after injectedBundleURL right next to it.
Comment 24 Mark Lam 2019-09-11 17:34:02 PDT
Created attachment 378604 [details]
proposed patch.
Comment 25 EWS Watchlist 2019-09-11 20:19:14 PDT
Comment on attachment 378604 [details]
proposed patch.

Attachment 378604 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/13024598

Number of test failures exceeded the failure limit.
Comment 26 EWS Watchlist 2019-09-11 20:19:17 PDT
Created attachment 378614 [details]
Archive of layout-test-results from ews210 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews210  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit
Comment 27 Fujii Hironori 2019-09-11 21:32:07 PDT
Created attachment 378619 [details]
Patch for Windows

In AppleWin, RELEASE_ASSERT is failing in JSC::JSGlobalObject::exposeDollarVM because
DumpRenderTree doesn't call JSC::Config::configureForTesting().

> JavaScriptCore.dll!abort() Line 77	C++
> JavaScriptCore.dll!JSC::JSGlobalObject::exposeDollarVM(JSC::VM & vm) Line 1829	C++
> DumpRenderTreeLib.dll!WebCoreTestSupport::injectInternalsObject(const OpaqueJSContext * context) Line 69	C++
> [Inline Frame] DumpRenderTreeLib.dll!FrameLoadDelegate::didClearWindowObjectForFrameInStandardWorld(IWebFrame *) Line 370	C++
> DumpRenderTreeLib.dll!FrameLoadDelegate::didClearWindowObjectForFrameInScriptWorld(IWebView * webView, IWebFrame * frame, IWebScriptWorld * world) Line 319	C++
> WebKit.dll!WebFrameLoaderClient::dispatchDidClearWindowObjectInWorld(WebCore::DOMWrapperWorld & world) Line 1250	C++
> WebKit.dll!WebCore::FrameLoader::dispatchDidClearWindowObjectInWorld(WebCore::DOMWrapperWorld & world) Line 3943	C++
> WebKit.dll!WebCore::ScriptController::initScriptForWindowProxy(WebCore::JSWindowProxy & windowProxy) Line 262	C++
> WebKit.dll!WebCore::WindowProxy::createJSWindowProxyWithInitializedScript(WebCore::DOMWrapperWorld & world) Line 125	C++
> [Inline Frame] WebKit.dll!WebCore::WindowProxy::jsWindowProxy(WebCore::DOMWrapperWorld &) Line 70	C++
> [Inline Frame] WebKit.dll!WebCore::ScriptController::jsWindowProxy(WebCore::DOMWrapperWorld &) Line 337	C++
> WebKit.dll!WebCore::ScriptController::evaluateInWorld(const WebCore::ScriptSourceCode & sourceCode, WebCore::DOMWrapperWorld & world, WebCore::ExceptionDetails * exceptionDetails) Line 123	C++
> WebKit.dll!WebCore::ScriptController::evaluate(const WebCore::ScriptSourceCode & sourceCode, WebCore::ExceptionDetails * exceptionDetails) Line 149	C++
> WebKit.dll!WebCore::ScriptElement::executeClassicScript(const WebCore::ScriptSourceCode & sourceCode) Line 390	C++
> WebKit.dll!WebCore::ScriptElement::prepareScript(const WTF::TextPosition & scriptStartPosition, WebCore::ScriptElement::LegacyTypeSupport supportLegacyTypes) Line 267	C++
> WebKit.dll!WebCore::HTMLScriptRunner::runScript(WebCore::ScriptElement & scriptElement, const WTF::TextPosition & scriptStartPosition) Line 252	C++
> WebKit.dll!WebCore::HTMLScriptRunner::execute(WTF::Ref<WebCore::ScriptElement,WTF::DumbPtrTraits<WebCore::ScriptElement> > && element, const WTF::TextPosition & scriptStartPosition) Line 142	C++
> WebKit.dll!WebCore::HTMLDocumentParser::runScriptsForPausedTreeBuilder() Line 234	C++
> WebKit.dll!WebCore::HTMLDocumentParser::pumpTokenizerLoop(WebCore::HTMLDocumentParser::SynchronousMode mode, bool parsingFragment, WebCore::PumpSession & session) Line 255	C++
> WebKit.dll!WebCore::HTMLDocumentParser::pumpTokenizer(WebCore::HTMLDocumentParser::SynchronousMode mode) Line 309	C++
> [Inline Frame] WebKit.dll!WebCore::HTMLDocumentParser::pumpTokenizerIfPossible(WebCore::HTMLDocumentParser::SynchronousMode) Line 186	C++
> WebKit.dll!WebCore::HTMLDocumentParser::append(WTF::RefPtr<WTF::StringImpl,WTF::DumbPtrTraits<WTF::StringImpl> > && inputSource) Line 419	C++
> WebKit.dll!WebCore::DecodedDataDocumentParser::appendBytes(WebCore::DocumentWriter & writer, const char * data, unsigned int length) Line 50	C++
> WebKit.dll!WebCore::DocumentWriter::addData(const char * bytes, unsigned int length) Line 246	C++
> WebKit.dll!WebCore::DocumentLoader::commitData(const char * bytes, unsigned int length) Line 1161	C++
> WebKit.dll!WebFrameLoaderClient::committedLoad(WebCore::DocumentLoader * loader, const char * data, int length) Line 695	C++
> WebKit.dll!WebCore::DocumentLoader::commitLoad(const char * data, int length) Line 1049	C++
> WebKit.dll!WebCore::DocumentLoader::dataReceived(const char * data, int length) Line 1194	C++
> WebKit.dll!WebCore::DocumentLoader::dataReceived(WebCore::CachedResource & resource, const char * data, int length) Line 1167	C++
> WebKit.dll!WebCore::CachedRawResource::notifyClientsDataWasReceived(const char * data, unsigned int length) Line 135	C++
> WebKit.dll!WebCore::CachedRawResource::updateBuffer(WebCore::SharedBuffer & data) Line 74	C++
> WebKit.dll!WebCore::SubresourceLoader::didReceiveDataOrBuffer(const char * data, int length, WTF::RefPtr<WebCore::SharedBuffer,WTF::DumbPtrTraits<WebCore::SharedBuffer> > && buffer, __int64 encodedDataLength, WebCore::DataPayloadType dataPayloadType) Line 482	C++
> WebKit.dll!WebCore::SubresourceLoader::didReceiveBuffer(WTF::Ref<WebCore::SharedBuffer,WTF::DumbPtrTraits<WebCore::SharedBuffer> > && buffer, __int64 encodedDataLength, WebCore::DataPayloadType dataPayloadType) Line 461	C++
> WebKit.dll!WebCore::ResourceLoader::didReceiveBuffer(WebCore::ResourceHandle * __formal, WTF::Ref<WebCore::SharedBuffer,WTF::DumbPtrTraits<WebCore::SharedBuffer> > && buffer, int encodedDataLength) Line 694	C++
> [Inline Frame] WebKit.dll!WebCore::ResourceHandleCFURLConnectionDelegateWithOperationQueue::didReceiveData::__l2::<lambda_5f349daa7f72b184051aee54a6289cd4>::operator()() Line 225	C++
> WebKit.dll!WTF::Detail::CallableWrapper<<lambda_5f349daa7f72b184051aee54a6289cd4>,void>::call() Line 52	C++
> [Inline Frame] WTF.dll!WTF::Function<void __cdecl(void)>::operator()() Line 79	C++
> WTF.dll!WTF::dispatchFunctionsFromMainThread() Line 117	C++
> WTF.dll!WTF::ThreadingWindowWndProc(HWND__ * hWnd, unsigned int message, unsigned int wParam, long lParam) Line 51	C++
> [External Code]	
> [Frames below may be incorrect and/or missing, no symbols loaded for user32.dll]	
> DumpRenderTreeLib.dll!runTest(const std::basic_string<char,std::char_traits<char>,std::allocator<char> > & inputLine) Line 1264	C++
Comment 28 Mark Lam 2019-09-11 23:17:57 PDT
Created attachment 378625 [details]
proposed patch.
Comment 29 Mark Lam 2019-09-12 08:05:26 PDT
Thanks for the review.  Landed in r249808: <http://trac.webkit.org/r249808>.