Bug 186433

Summary: Crash under com.apple.WebKit.Networking at WebCore: WebCore::NetworkStorageSession::hasStorageAccess const
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: WebKit2Assignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, bfulgham, commit-queue, ews-watchlist, ggaren, webkit-bug-importer, wilander, youennf
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Archive of layout-test-results from ews200 for win-future none

Description Chris Dumez 2018-06-08 08:42:34 PDT 
Crash under com.apple.WebKit.Networking at WebCore: WebCore::NetworkStorageSession::hasStorageAccess const:
Exception Type:        EXC_BAD_ACCESS (SIGSEGV)
Exception Codes:       KERN_INVALID_ADDRESS at 0x0000000000000010
Exception Note:        EXC_CORPSE_NOTIFY
Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.WebCore             	0x00007fff412432a4 WebCore::NetworkStorageSession::hasStorageAccess(WTF::String const&, WTF::String const&, std::optional<unsigned long long>, unsigned long long) const + 484
1   com.apple.WebCore             	0x00007fff41243002 WebCore::NetworkStorageSession::cookieStoragePartition(WebCore::URL const&, WebCore::URL const&, std::optional<unsigned long long>, std::optional<unsigned long long>) const + 706
2   com.apple.WebCore             	0x00007fff41242d22 WebCore::NetworkStorageSession::cookieStoragePartition(WebCore::ResourceRequest const&, std::optional<unsigned long long>, std::optional<unsigned long long>) const + 130
3   com.apple.WebKit              	0x00007fff41adc2b7 WebKit::NetworkDataTaskCocoa::NetworkDataTaskCocoa(WebKit::NetworkSession&, WebKit::NetworkDataTaskClient&, WebCore::ResourceRequest const&, unsigned long long, unsigned long long, WebCore::StoredCredentialsPolicy, WebCore::ContentSniffingPolicy, WebCore::ContentEncodingSniffingPolicy, bool, WebKit::PreconnectOnly, bool, std::optional<WebKit::NetworkActivityTracker>) + 1779
4   com.apple.WebKit              	0x00007fff41ad8df1 WebKit::NetworkDataTaskCocoa::create(WebKit::NetworkSession&, WebKit::NetworkDataTaskClient&, WebCore::ResourceRequest const&, unsigned long long, unsigned long long, WebCore::StoredCredentialsPolicy, WebCore::ContentSniffingPolicy, WebCore::ContentEncodingSniffingPolicy, bool, WebKit::PreconnectOnly, bool, std::optional<WebKit::NetworkActivityTracker>) + 189
5   com.apple.WebKit              	0x00007fff41ad8d0e WebKit::NetworkDataTask::create(WebKit::NetworkSession&, WebKit::NetworkDataTaskClient&, WebKit::NetworkLoadParameters const&) + 334
6   com.apple.WebKit              	0x00007fff41adfff1 WebKit::NetworkLoad::initialize(WebKit::NetworkSession&) + 33
7   com.apple.WebKit              	0x00007fff41b54e22 WebKit::PreconnectTask::PreconnectTask(WebKit::NetworkLoadParameters&&, WTF::CompletionHandler<void (WebCore::ResourceError const&)>&&) + 258
8   com.apple.WebKit              	0x00007fff41acbe6f WebKit::NetworkConnectionToWebProcess::preconnectTo(unsigned long long, WebKit::NetworkResourceLoadParameters&&) + 93
9   com.apple.WebKit              	0x00007fff41ad0020 void IPC::handleMessage<Messages::NetworkConnectionToWebProcess::PreconnectTo, WebKit::NetworkConnectionToWebProcess, void (WebKit::NetworkConnectionToWebProcess::*)(unsigned long long, WebKit::NetworkResourceLoadParameters&&)>(IPC::Decoder&, WebKit::NetworkConnectionToWebProcess*, void (WebKit::NetworkConnectionToWebProcess::*)(unsigned long long, WebKit::NetworkResourceLoadParameters&&)) + 408
10  com.apple.WebKit              	0x00007fff41a5fd53 IPC::Connection::dispatchMessage(std::__1::unique_ptr<IPC::Decoder, std::__1::default_delete<IPC::Decoder> >) + 119
11  com.apple.WebKit              	0x00007fff41a626c1 IPC::Connection::dispatchOneMessage() + 177
12  com.apple.JavaScriptCore      	0x00007fff360b26a7 WTF::RunLoop::performWork() + 231
13  com.apple.JavaScriptCore      	0x00007fff360b2932 WTF::RunLoop::performWork(void*) + 34
14  com.apple.CoreFoundation      	0x00007fff328406b0 __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ + 17
15  com.apple.CoreFoundation      	0x00007fff328ddeb2 __CFRunLoopDoSource0 + 108
16  com.apple.CoreFoundation      	0x00007fff32827718 __CFRunLoopDoSources0 + 195
17  com.apple.CoreFoundation      	0x00007fff32826cad __CFRunLoopRun + 1225
18  com.apple.CoreFoundation      	0x00007fff328265c7 CFRunLoopRunSpecific + 463
19  com.apple.Foundation          	0x00007fff34c50d71 -[NSRunLoop(NSRunLoop) runMode:beforeDate:] + 280
20  com.apple.Foundation          	0x00007fff34c50c46 -[NSRunLoop(NSRunLoop) run] + 76
21  libxpc.dylib                  	0x00007fff5fde50ee _xpc_objc_main + 567
22  libxpc.dylib                  	0x00007fff5fde3d45 xpc_main + 443
Comment 1 Chris Dumez 2018-06-08 08:42:49 PDT 
<rdar://problem/40750907>
Comment 2 Chris Dumez 2018-06-08 08:47:56 PDT 
<rdar://problem/40008877>
Comment 3 Chris Dumez 2018-06-08 11:14:41 PDT 
Created attachment 342295 [details]
Patch
Comment 4 Chris Dumez 2018-06-08 11:14:59 PDT 
More details on the radar.
Comment 5 John Wilander 2018-06-08 11:17:54 PDT 
Looks good to me. Thanks for fixing this one!
Comment 6 John Wilander 2018-06-08 11:18:30 PDT 
BTW, didn't we have a stable repro case? Could we make a layout test out of it?
Comment 7 Chris Dumez 2018-06-08 11:22:00 PDT 
(In reply to John Wilander from comment #6)
> BTW, didn't we have a stable repro case? Could we make a layout test out of
> it?

As noted on the radar, I believe the crash would no longer occurs after http://trac.webkit.org/r232198 but I still think it is worth hardening rather than relying on the call sites to not pass null.

I will send you a layout test attempt I wrote. I got close to exercising this code path but I was not able to populate m_pagesGrantedStorageAccess from the layout test despite my efforts. Maybe you can help tweak the test :)
Comment 8 Brent Fulgham 2018-06-08 11:52:50 PDT 
Comment on attachment 342295 [details]
Patch

Yes. This seems like a wise hardening step.

Do we need the same protection in "NetworkStorageSession::grantStorageAccess", too?
Comment 9 Chris Dumez 2018-06-08 11:56:23 PDT 
Created attachment 342302 [details]
Patch
Comment 10 Chris Dumez 2018-06-08 11:56:38 PDT 
(In reply to Brent Fulgham from comment #8)
> Comment on attachment 342295 [details]
> Patch
> 
> Yes. This seems like a wise hardening step.
> 
> Do we need the same protection in
> "NetworkStorageSession::grantStorageAccess", too?

Good idea.
Comment 11 EWS Watchlist 2018-06-08 17:14:53 PDT 
Comment on attachment 342302 [details]
Patch

Attachment 342302 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/8091587

New failing tests:
http/tests/security/contentSecurityPolicy/userAgentShadowDOM/allow-video.html
Comment 12 EWS Watchlist 2018-06-08 17:15:05 PDT 
Created attachment 342348 [details]
Archive of layout-test-results from ews200 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews200  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment 13 Chris Dumez 2018-06-08 17:17:44 PDT 
Failure is unrelated.
Comment 14 Chris Dumez 2018-06-11 08:25:50 PDT 
ping review?
Comment 15 Geoffrey Garen 2018-06-11 09:31:29 PDT 
Comment on attachment 342302 [details]
Patch

r=me
Comment 16 youenn fablet 2018-06-11 09:54:32 PDT 
Comment on attachment 342302 [details]
Patch

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

> Source/WebCore/platform/network/cf/NetworkStorageSessionCFNet.cpp:342
>  void NetworkStorageSession::grantStorageAccess(const String& resourceDomain, const String& firstPartyDomain, std::optional<uint64_t> frameID, uint64_t pageID)

Is there a reason to request storage access if firstPartyDomain is empty?
Comment 17 WebKit Commit Bot 2018-06-11 09:58:23 PDT 
Comment on attachment 342302 [details]
Patch

Clearing flags on attachment: 342302

Committed r232720: <https://trac.webkit.org/changeset/232720>
Comment 18 WebKit Commit Bot 2018-06-11 09:58:24 PDT 
All reviewed patches have been landed.  Closing bug.