Bug 182920 - Null-dereference of the second argument `resource` of DocumentLoader::scheduleSubstituteResourceLoad
Summary: Null-dereference of the second argument `resource` of DocumentLoader::schedul...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Fujii Hironori
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-02-18 23:42 PST by Fujii Hironori
Modified: 2018-02-24 14:07 PST (History)
12 users (show)

See Also:


Attachments
debug patch (611 bytes, patch)
2018-02-18 23:43 PST, Fujii Hironori
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-sierra (2.26 MB, application/zip)
2018-02-19 00:44 PST, Build Bot
no flags Details
Archive of layout-test-results from ews105 for mac-sierra-wk2 (2.65 MB, application/zip)
2018-02-19 00:49 PST, Build Bot
no flags Details
Archive of layout-test-results from ews116 for mac-sierra (2.99 MB, application/zip)
2018-02-19 01:13 PST, Build Bot
no flags Details
Archive of layout-test-results from ews124 for ios-simulator-wk2 (2.30 MB, application/zip)
2018-02-19 01:14 PST, Build Bot
no flags Details
Patch (4.32 KB, patch)
2018-02-20 03:15 PST, Fujii Hironori
no flags Details | Formatted Diff | Diff
Patch (3.88 KB, patch)
2018-02-20 17:56 PST, Fujii Hironori
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Fujii Hironori 2018-02-18 23:42:39 PST
[GTK] imported/w3c/web-platform-tests/html/browsers/offline/appcache/workers/appcache-worker.html crash

On my Ubuntu 17.10, imported/w3c/web-platform-tests/html/browsers/offline/appcache/workers/appcache-worker.html always crashes.
GTK port, Release build, trunk@228417

Debug build doesn't crash.
BuildBots doesn't crash.

./Tools/Scripts/run-webkit-tests --gtk --release imported/w3c/web-platform-tests/html/browsers/offline/appcache/workers/appcache-worker.html


> Thread 1 (Thread 0x7fa846698ac0 (LWP 4687)):
> #0  0x00007fa844eabc68 in WebCore::DocumentLoader::scheduleSubstituteResourceLoad(WebCore::ResourceLoader&, WebCore::SubstituteResource&) () from /home/fujii/work/webkit/gb/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
> #1  0x00007fa844f0c0e1 in WebCore::ApplicationCacheHost::maybeLoadResource(WebCore::ResourceLoader&, WebCore::ResourceRequest const&, WebCore::URL const&) () from /home/fujii/work/webkit/gb/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
> #2  0x00007fa84402c804 in WebKit::WebLoaderStrategy::scheduleLoad(WebCore::ResourceLoader&, WebCore::CachedResource*, bool) () from /home/fujii/work/webkit/gb/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
> #3  0x00007fa84402d000 in WTF::Function<void (WTF::RefPtr<WebCore::SubresourceLoader, WTF::DumbPtrTraits<WebCore::SubresourceLoader> >&&)>::CallableWrapper<WebKit::WebLoaderStrategy::loadResource(WebCore::Frame&, WebCore::CachedResource&, WebCore::ResourceRequest&&, WebCore::ResourceLoaderOptions const&, WTF::CompletionHandler<void (WTF::RefPtr<WebCore::SubresourceLoader, WTF::DumbPtrTraits<WebCore::SubresourceLoader> >&&)>&&)::{lambda(WTF::RefPtr<WebCore::SubresourceLoader, WTF::DumbPtrTraits<WebCore::SubresourceLoader> >&&)#1}>::call(WTF::RefPtr<WebCore::SubresourceLoader, WTF::DumbPtrTraits<WebCore::SubresourceLoader> >&&) () from /home/fujii/work/webkit/gb/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
> #4  0x00007fa844efe0ea in WTF::Function<void (bool)>::CallableWrapper<WebCore::SubresourceLoader::create(WebCore::Frame&, WebCore::CachedResource&, WebCore::ResourceRequest&&, WebCore::ResourceLoaderOptions const&, WTF::CompletionHandler<void (WTF::RefPtr<WebCore::SubresourceLoader, WTF::DumbPtrTraits<WebCore::SubresourceLoader> >&&)>&&)::{lambda(bool)#1}>::call(bool) () from /home/fujii/work/webkit/gb/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
> #5  0x00007fa844efe9d6 in WTF::Function<void (bool)>::CallableWrapper<WebCore::SubresourceLoader::init(WebCore::ResourceRequest&&, WTF::CompletionHandler<void (bool)>&&)::{lambda(bool)#1}>::call(bool) () from /home/fujii/work/webkit/gb/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
> #6  0x00007fa844ef2f72 in WTF::Function<void (WebCore::ResourceRequest&&)>::CallableWrapper<WebCore::ResourceLoader::init(WebCore::ResourceRequest&&, WTF::CompletionHandler<void (bool)>&&)::{lambda(WebCore::ResourceRequest&&)#1}>::call(WebCore::ResourceRequest&&) () from /home/fujii/work/webkit/gb/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
> #7  0x00007fa844efa810 in WTF::Function<void (WebCore::ResourceRequest&&)>::CallableWrapper<WebCore::SubresourceLoader::willSendRequestInternal(WebCore::ResourceRequest&&, WebCore::ResourceResponse const&, WTF::CompletionHandler<void (WebCore::ResourceRequest&&)>&&)::{lambda(WTF::CompletionHandler<void (WebCore::ResourceRequest&&)>&&, WebCore::ResourceRequest&&)#1}::operator()(WTF::CompletionHandler<void (WebCore::ResourceRequest&&)>&&, WebCore::ResourceRequest&&)::{lambda(WebCore::ResourceRequest&&)#1}>::call(WebCore::ResourceRequest&&) () from /home/fujii/work/webkit/gb/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
> #8  0x00007fa844ef72c0 in WebCore::ResourceLoader::willSendRequestInternal(WebCore::ResourceRequest&&, WebCore::ResourceResponse const&, WTF::CompletionHandler<void (WebCore::ResourceRequest&&)>&&) () from /home/fujii/work/webkit/gb/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
> #9  0x00007fa844f03b1c in WebCore::SubresourceLoader::willSendRequestInternal(WebCore::ResourceRequest&&, WebCore::ResourceResponse const&, WTF::CompletionHandler<void (WebCore::ResourceRequest&&)>&&)::{lambda(WTF::CompletionHandler<void (WebCore::ResourceRequest&&)>&&, WebCore::ResourceRequest&&)#1}::operator()(WTF::CompletionHandler<void (WebCore::ResourceRequest&&)>&&, WebCore::ResourceRequest&&) () from /home/fujii/work/webkit/gb/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
> #10 0x00007fa844f0473e in WebCore::SubresourceLoader::willSendRequestInternal(WebCore::ResourceRequest&&, WebCore::ResourceResponse const&, WTF::CompletionHandler<void (WebCore::ResourceRequest&&)>&&) () from /home/fujii/work/webkit/gb/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
> #11 0x00007fa844ef29c0 in WebCore::ResourceLoader::init(WebCore::ResourceRequest&&, WTF::CompletionHandler<void (bool)>&&) () from /home/fujii/work/webkit/gb/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
> #12 0x00007fa844f037f9 in WebCore::SubresourceLoader::init(WebCore::ResourceRequest&&, WTF::CompletionHandler<void (bool)>&&) () from /home/fujii/work/webkit/gb/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
> #13 0x00007fa844f038ed in WebCore::SubresourceLoader::create(WebCore::Frame&, WebCore::CachedResource&, WebCore::ResourceRequest&&, WebCore::ResourceLoaderOptions const&, WTF::CompletionHandler<void (WTF::RefPtr<WebCore::SubresourceLoader, WTF::DumbPtrTraits<WebCore::SubresourceLoader> >&&)>&&) () from /home/fujii/work/webkit/gb/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
> #14 0x00007fa84402848d in WebKit::WebLoaderStrategy::loadResource(WebCore::Frame&, WebCore::CachedResource&, WebCore::ResourceRequest&&, WebCore::ResourceLoaderOptions const&, WTF::CompletionHandler<void (WTF::RefPtr<WebCore::SubresourceLoader, WTF::DumbPtrTraits<WebCore::SubresourceLoader> >&&)>&&) () from /home/fujii/work/webkit/gb/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
> #15 0x00007fa844f38adf in WebCore::CachedResource::load(WebCore::CachedResourceLoader&) () from /home/fujii/work/webkit/gb/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
> #16 0x00007fa844f3d4c1 in WebCore::CachedResourceLoader::requestResource(WebCore::CachedResource::Type, WebCore::CachedResourceRequest&&, WebCore::CachedResourceLoader::ForPreload, WebCore::CachedResourceLoader::DeferOption) () from /home/fujii/work/webkit/gb/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
> #17 0x00007fa844f3e9df in WebCore::CachedResourceLoader::requestRawResource(WebCore::CachedResourceRequest&&) () from /home/fujii/work/webkit/gb/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
> #18 0x00007fa844ea7adb in WebCore::DocumentThreadableLoader::loadRequest(WebCore::ResourceRequest&&, WebCore::SecurityCheckPolicy) () from /home/fujii/work/webkit/gb/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
> #19 0x00007fa844eae25c in WebCore::DocumentThreadableLoader::DocumentThreadableLoader(WebCore::Document&, WebCore::ThreadableLoaderClient&, WebCore::DocumentThreadableLoader::BlockingBehavior, WebCore::ResourceRequest&&, WebCore::ThreadableLoaderOptions const&, WTF::RefPtr<WebCore::SecurityOrigin, WTF::DumbPtrTraits<WebCore::SecurityOrigin> >&&, std::unique_ptr<WebCore::ContentSecurityPolicy, std::default_delete<WebCore::ContentSecurityPolicy> >&&, WTF::String&&, WebCore::DocumentThreadableLoader::ShouldLogError) () from /home/fujii/work/webkit/gb/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
> #20 0x00007fa844eae6cc in WebCore::DocumentThreadableLoader::create(WebCore::Document&, WebCore::ThreadableLoaderClient&, WebCore::ResourceRequest&&, WebCore::ThreadableLoaderOptions const&, WTF::RefPtr<WebCore::SecurityOrigin, WTF::DumbPtrTraits<WebCore::SecurityOrigin> >&&, std::unique_ptr<WebCore::ContentSecurityPolicy, std::default_delete<WebCore::ContentSecurityPolicy> >&&, WTF::String&&, WebCore::DocumentThreadableLoader::ShouldLogError) () from /home/fujii/work/webkit/gb/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
> #21 0x00007fa844efed80 in WTF::Function<void (WebCore::ScriptExecutionContext&)>::CallableWrapper<WebCore::WorkerThreadableLoader::MainThreadBridge::MainThreadBridge(WebCore::ThreadableLoaderClientWrapper&, WebCore::WorkerLoaderProxy&, WTF::String const&, WebCore::ResourceRequest&&, WebCore::ThreadableLoaderOptions const&, WTF::String const&, WebCore::WorkerGlobalScope&)::{lambda(WebCore::ScriptExecutionContext&)#1}>::call(WebCore::ScriptExecutionContext&) () from /home/fujii/work/webkit/gb/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
> #22 0x00007fa83f0a56b5 in WTF::dispatchFunctionsFromMainThread() () from /home/fujii/work/webkit/gb/WebKitBuild/Release/lib/libjavascriptcoregtk-4.0.so.18
> #23 0x00007fa83f0eb173 in WTF::RunLoop::TimerBase::TimerBase(WTF::RunLoop&)::{lambda(void*)#1}::_FUN(void*) () from /home/fujii/work/webkit/gb/WebKitBuild/Release/lib/libjavascriptcoregtk-4.0.so.18
> #24 0x00007fa83fe75c35 in g_main_dispatch () at /home/fujii/work/webkit/gb/WebKitBuild/DependenciesGTK/Source/glib-2.54.2/glib/gmain.c:3148
> #25 g_main_context_dispatch () at /home/fujii/work/webkit/gb/WebKitBuild/DependenciesGTK/Source/glib-2.54.2/glib/gmain.c:3813
> #26 0x00007fa83fe76000 in g_main_context_iterate () at /home/fujii/work/webkit/gb/WebKitBuild/DependenciesGTK/Source/glib-2.54.2/glib/gmain.c:3886
> #27 0x00007fa83fe76312 in g_main_loop_run () at /home/fujii/work/webkit/gb/WebKitBuild/DependenciesGTK/Source/glib-2.54.2/glib/gmain.c:4082
> #28 0x00007fa83f0eb600 in WTF::RunLoop::run() () from /home/fujii/work/webkit/gb/WebKitBuild/Release/lib/libjavascriptcoregtk-4.0.so.18
> #29 0x00007fa84430ed08 in WebProcessMainUnix () from /home/fujii/work/webkit/gb/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
> #30 0x00007fa8423ab1c1 in __libc_start_main (main=0x5590113a88c0 <main>, argc=3, argv=0x7ffe774b0378, init=<optimized out>, fini=<optimized out>, rtld_fini=<optimized out>, stack_end=0x7ffe774b0368) at ../csu/libc-start.c:308
> #31 0x00005590113a894a in _start ()
Comment 1 Fujii Hironori 2018-02-18 23:43:27 PST
Created attachment 334139 [details]
debug patch
Comment 2 Fujii Hironori 2018-02-18 23:53:05 PST
If this debug patch is applied, the assertion fails even in Debug builds.
Comment 3 Build Bot 2018-02-19 00:44:24 PST
Comment on attachment 334139 [details]
debug patch

Attachment 334139 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/6569219

New failing tests:
imported/w3c/web-platform-tests/html/browsers/offline/appcache/workers/appcache-worker.html
Comment 4 Build Bot 2018-02-19 00:44:26 PST
Created attachment 334140 [details]
Archive of layout-test-results from ews103 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 5 Build Bot 2018-02-19 00:49:12 PST
Comment on attachment 334139 [details]
debug patch

Attachment 334139 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/6569229

New failing tests:
imported/w3c/web-platform-tests/html/browsers/offline/appcache/workers/appcache-worker.html
Comment 6 Build Bot 2018-02-19 00:49:14 PST
Created attachment 334141 [details]
Archive of layout-test-results from ews105 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6
Comment 7 Build Bot 2018-02-19 01:13:04 PST
Comment on attachment 334139 [details]
debug patch

Attachment 334139 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/6569253

New failing tests:
imported/w3c/web-platform-tests/html/browsers/offline/appcache/workers/appcache-worker.html
Comment 8 Build Bot 2018-02-19 01:13:05 PST
Created attachment 334144 [details]
Archive of layout-test-results from ews116 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 9 Build Bot 2018-02-19 01:14:35 PST
Comment on attachment 334139 [details]
debug patch

Attachment 334139 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/6569266

New failing tests:
imported/w3c/web-platform-tests/html/browsers/offline/appcache/workers/appcache-worker.html
Comment 10 Build Bot 2018-02-19 01:14:36 PST
Created attachment 334145 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6
Comment 11 Fujii Hironori 2018-02-19 07:55:49 PST
These EWS layout test failures mean Mac port also has the null dereference issue. This is not GTK specific bug.
Comment 12 Fujii Hironori 2018-02-19 22:07:10 PST
request.url() was "http://localhost:8800/resources/testharness.js".

>     // Resources that match fallback namespaces or online whitelist entries are fetched from the network,
>     // unless they are also cached.
>     if (!resource && (cache->allowsAllNetworkRequests() || cache->urlMatchesFallbackNamespace(request.url()) || cache->isURLInOnlineWhitelist(request.url())))
>         return false;

resource was null, but following all conditions are false.
Comment 13 Fujii Hironori 2018-02-20 03:15:03 PST
Created attachment 334257 [details]
Patch
Comment 14 Michael Catanzaro 2018-02-20 09:41:14 PST
Comment on attachment 334257 [details]
Patch

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

> Source/WebCore/loader/DocumentLoader.cpp:1432
> -    m_pendingSubstituteResources.set(&loader, &resource);
> +    m_pendingSubstituteResources.set(&loader, resource);
>      deliverSubstituteResourcesAfterDelay();

I don't understand; why would you want to add nullptr to m_pendingSubstituteResources? Isn't it better to avoid calling scheduleSubstituteResourceLoad entirely in this case? Then you can make scheduleArchiveLoad return false, instead of true, to indicate failure.

Does deliverSubstituteResourcesAfterDelay need to be called when the second argument is nullptr? Probably not, I guess, so it should probably be OK to skip the call to scheduleSubstituteResourceLoad entirely, right?

> Source/WebCore/loader/appcache/ApplicationCacheHost.cpp:182
>      if (!shouldLoadResourceFromApplicationCache(request, resource))
>          return false;
>  
> -    m_documentLoader.scheduleSubstituteResourceLoad(loader, *resource);
> +    m_documentLoader.scheduleSubstituteResourceLoad(loader, resource);

I wonder about this, though. Looks like resource could be null here, as well. I suspect we should skip the call to scheduleSubstituteResourceLoad and return false in this case, too. Not sure.

> Source/WebCore/loader/appcache/ApplicationCacheHost.cpp:449
> -    m_documentLoader.scheduleSubstituteResourceLoad(*loader, *resource);
> +    m_documentLoader.scheduleSubstituteResourceLoad(*loader, resource);

resource will always be non-null here, so this should be fine.
Comment 15 Fujii Hironori 2018-02-20 17:30:37 PST
Comment on attachment 334257 [details]
Patch

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

Thank you for the review, Michael.

>> Source/WebCore/loader/DocumentLoader.cpp:1432
>>      deliverSubstituteResourcesAfterDelay();
> 
> I don't understand; why would you want to add nullptr to m_pendingSubstituteResources? Isn't it better to avoid calling scheduleSubstituteResourceLoad entirely in this case? Then you can make scheduleArchiveLoad return false, instead of true, to indicate failure.
> 
> Does deliverSubstituteResourcesAfterDelay need to be called when the second argument is nullptr? Probably not, I guess, so it should probably be OK to skip the call to scheduleSubstituteResourceLoad entirely, right?

Yes, DocumentLoader::scheduleSubstituteResourceLoad is tricky, there are a FIXME comment in DocumentLoader::substituteResourceDeliveryTimerFired.

https://trac.webkit.org/browser/webkit/trunk/Source/WebCore/loader/DocumentLoader.cpp?rev=228852#L1384

>  // A null resource means that we should fail the load.
>  // FIXME: Maybe we should use another error here - something like "not in cache".
>  loader->didFail(loader->cannotShowURLError());

I will submit another less tricky patch.
Comment 16 Fujii Hironori 2018-02-20 17:56:10 PST
Created attachment 334329 [details]
Patch
Comment 17 Michael Catanzaro 2018-02-20 19:05:50 PST
Hm, I think this looks better. It would be better if someone more experienced with WebCore would review it, though. I see Ryosuke is already CCed; maybe he would want to look at it.
Comment 18 WebKit Commit Bot 2018-02-24 14:06:43 PST
Comment on attachment 334329 [details]
Patch

Clearing flags on attachment: 334329

Committed r228975: <https://trac.webkit.org/changeset/228975>
Comment 19 WebKit Commit Bot 2018-02-24 14:06:45 PST
All reviewed patches have been landed.  Closing bug.
Comment 20 Radar WebKit Bug Importer 2018-02-24 14:07:28 PST
<rdar://problem/37860702>