Bug 182920 – Null-dereference of the second argument `resource` of DocumentLoader::scheduleSubstituteResourceLoad

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Fujii Hironori]

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

[Fujii Hironori]

Created attachment 334139 [details]
debug patch

[Fujii Hironori]

If this debug patch is applied, the assertion fails even in Debug builds.

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Fujii Hironori]

These EWS layout test failures mean Mac port also has the null dereference issue. This is not GTK specific bug.

[Fujii Hironori]

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.

[Fujii Hironori]

Created attachment 334257 [details]
Patch

[Michael Catanzaro]

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.

[Fujii Hironori]

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.

[Fujii Hironori]

Created attachment 334329 [details]
Patch

[Michael Catanzaro]

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.

[WebKit Commit Bot]

Comment on attachment 334329 [details]
Patch

Clearing flags on attachment: 334329

Committed r228975: <https://trac.webkit.org/changeset/228975>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/37860702>