Bug 186433 – Crash under com.apple.WebKit.Networking at WebCore: WebCore::NetworkStorageSession::hasStorageAccess const

• Component: WebKit2
• Status: RESOLVED FIXED

[Chris Dumez]

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

[Chris Dumez]

<rdar://problem/40750907>

[Chris Dumez]

<rdar://problem/40008877>

[Chris Dumez]

Created attachment 342295 [details]
Patch

[Chris Dumez]

More details on the radar.

[John Wilander]

Looks good to me. Thanks for fixing this one!

[John Wilander]

BTW, didn't we have a stable repro case? Could we make a layout test out of it?

[Chris Dumez]

(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 :)

[Brent Fulgham]

Comment on attachment 342295 [details]
Patch

Yes. This seems like a wise hardening step.

Do we need the same protection in "NetworkStorageSession::grantStorageAccess", too?

[Chris Dumez]

Created attachment 342302 [details]
Patch

[Chris Dumez]

(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.

[EWS Watchlist]

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

[EWS Watchlist]

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

[Chris Dumez]

Failure is unrelated.

[Chris Dumez]

ping review?

[Geoffrey Garen]

Comment on attachment 342302 [details]
Patch

r=me

[youenn fablet]

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?

[WebKit Commit Bot]

Comment on attachment 342302 [details]
Patch

Clearing flags on attachment: 342302

Committed r232720: <https://trac.webkit.org/changeset/232720>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.