Bug 185819

Summary: Crash when loading a SVG image
Product: WebKit Reporter: Carlos Garcia Campos <cgarcia>
Component: WebKit2Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, agomez, bfulgham, mcatanzaro, product-security, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch bfulgham: review+

Description Carlos Garcia Campos 2018-05-21 06:25:10 PDT 
This is happening in WebLoaderStrategy::scheduleLoad() when getting the value of FrameLoaderClient::pageID(). SVGImage uses the empty clients for the loader, and EmptyFrameLoaderClient::pageID() returns std::nullopt. The same happens with the frameID. This changed in r225934, when pageID() and frameID() were changed to return std::optional, EmptyFrameLoaderClient was updated to return std::nullopt instead of 0. I don't know why this crash didn't happen before to me, maybe getting the value of an empty optional has changed in a newer version of GCC, I don't know. The crash can be fixed by using value_or(0) instead of value() for both the frameID and the pageID. I'm not sure about the security implications of this crash, so I've used the security product just in case.

Thread 1 "WebKitWebProces" received signal SIGABRT, Aborted.
__GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:51
51	../sysdeps/unix/sysv/linux/raise.c: No existe el fichero o el directorio.
(gdb) bt
#0  __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:51
#1  0x00007f8c41756231 in __GI_abort () at abort.c:79
#2  0x00007f8c4dd81de4 in WebKit::WebLoaderStrategy::scheduleLoad(WebCore::ResourceLoader&, WebCore::CachedResource*, bool) ()
   from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
#3  0x00007f8c4dd82460 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/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
#4  0x00007f8c4ebece4d 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/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
#5  0x00007f8c4ebebdb6 in WTF::Function<void (bool)>::CallableWrapper<WebCore::SubresourceLoader::init(WebCore::ResourceRequest&&, WTF::CompletionHandler<void (bool)>&&)::{lambda(bool)#1}>::call(bool) () from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
#6  0x00007f8c4ebe73f2 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/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
#7  0x00007f8c4ebebb80 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/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
#8  0x00007f8c4ebe7b20 in WebCore::ResourceLoader::willSendRequestInternal(WebCore::ResourceRequest&&, WebCore::ResourceResponse const&, WTF::CompletionHandler<void (WebCore::ResourceRequest&&)>&&) () from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
#9  0x00007f8c4ebf463c 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/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
#10 0x00007f8c4ebf53ae in WebCore::SubresourceLoader::willSendRequestInternal(WebCore::ResourceRequest&&, WebCore::ResourceResponse const&, WTF::CompletionHandler<void (WebCore::ResourceRequest&&)>&&) () from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
#11 0x00007f8c4ebe2670 in WebCore::ResourceLoader::init(WebCore::ResourceRequest&&, WTF::CompletionHandler<void (bool)>&&) ()
   from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
#12 0x00007f8c4ebf4421 in WebCore::SubresourceLoader::init(WebCore::ResourceRequest&&, WTF::CompletionHandler<void (bool)>&&) ()
   from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
#13 0x00007f8c4ebf44dd 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/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
#14 0x00007f8c4dd7e57d 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/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
#15 0x00007f8c4ec32a6a in WebCore::CachedResource::load(WebCore::CachedResourceLoader&) () from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
#16 0x00007f8c4ec377a9 in WebCore::CachedResourceLoader::requestResource(WebCore::CachedResource::Type, WebCore::CachedResourceRequest&&, WebCore::CachedResourceLoader::ForPreload, WebCore::CachedResourceLoader::DeferOption) () from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
#17 0x00007f8c4ec3916d in WebCore::CachedResourceLoader::requestImage(WebCore::CachedResourceRequest&&) ()
   from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
#18 0x00007f8c4ebcc794 in WebCore::ImageLoader::updateFromElement() () from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
#19 0x00007f8c4f20a5e4 in WebCore::SVGImageElement::svgAttributeChanged(WebCore::QualifiedName const&) ()
   from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
#20 0x00007f8c4e876086 in WebCore::Element::parserSetAttributes(WTF::Vector<WebCore::Attribute, 0ul, WTF::CrashOnOverflow, 16ul> const&) ()
   from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
#21 0x00007f8c4f2fc298 in WebCore::XMLDocumentParser::startElementNs(unsigned char const*, unsigned char const*, unsigned char const*, int, unsigned char const**, int, int, unsigned char const**) () from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
#22 0x00007f8c4c3e5a92 in ?? () from /usr/lib/x86_64-linux-gnu/libxml2.so.2
#23 0x00007f8c4c3f3b09 in ?? () from /usr/lib/x86_64-linux-gnu/libxml2.so.2
#24 0x00007f8c4c3f520e in xmlParseChunk () from /usr/lib/x86_64-linux-gnu/libxml2.so.2
#25 0x00007f8c4f2fa597 in WebCore::XMLDocumentParser::doWrite(WTF::String const&) () from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
#26 0x00007f8c4f2fa80b in WebCore::XMLDocumentParser::append(WTF::RefPtr<WTF::StringImpl, WTF::DumbPtrTraits<WTF::StringImpl> >&&) ()
   from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
#27 0x00007f8c4e828a4b in WebCore::DecodedDataDocumentParser::appendBytes(WebCore::DocumentWriter&, char const*, unsigned long) ()
   from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
#28 0x00007f8c4f2802ff in WebCore::SVGImage::dataChanged(bool) [clone .part.269] () from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
#29 0x00007f8c4ec2b47c in WebCore::CachedImage::updateImageData(bool) () from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
#30 0x00007f8c4ec33ab7 in WebCore::CachedImage::finishLoading(WebCore::SharedBuffer*) () from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
#31 0x00007f8c4ebf4e50 in WebCore::SubresourceLoader::didFinishLoading(WebCore::NetworkLoadMetrics const&) ()
   from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
#32 0x00007f8c4dd88bf8 in WebKit::WebResourceLoader::didReceiveResource(WebKit::ShareableResource::Handle const&) ()
   from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
#33 0x00007f8c4decf0d7 in WebKit::WebResourceLoader::didReceiveWebResourceLoaderMessage(IPC::Connection&, IPC::Decoder&) ()
   from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
#34 0x00007f8c4dbabe8b in IPC::Connection::dispatchMessage(std::unique_ptr<IPC::Decoder, std::default_delete<IPC::Decoder> >) ()
   from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
#35 0x00007f8c4dbac81c in IPC::Connection::dispatchOneMessage() () from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
#36 0x00007f8c4bb6e465 in WTF::RunLoop::performWork() () from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libjavascriptcoregtk-4.0.so.18
#37 0x00007f8c4bba2ea9 in WTF::RunLoop::RunLoop()::{lambda(void*)#1}::_FUN(void*) () from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libjavascriptcoregtk-4.0.so.18
#38 0x00007f8c4435d615 in g_main_dispatch (context=0x5625ccf99310) at gmain.c:3177
#39 g_main_context_dispatch (context=context@entry=0x5625ccf99310) at gmain.c:3830
#40 0x00007f8c4435d9b8 in g_main_context_iterate (context=0x5625ccf99310, block=block@entry=1, dispatch=dispatch@entry=1, self=<optimized out>) at gmain.c:3903
#41 0x00007f8c4435dcc2 in g_main_loop_run (loop=0x5625cd0211c0) at gmain.c:4099
#42 0x00007f8c4bba38e8 in WTF::RunLoop::run() () from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libjavascriptcoregtk-4.0.so.18
#43 0x00007f8c4e0829d0 in WebProcessMainUnix () from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
#44 0x00007f8c41741a87 in __libc_start_main (main=0x5625cceebc30 <main>, argc=3, argv=0x7fff0484a848, init=<optimized out>, fini=<optimized out>, rtld_fini=<optimized out>, 
    stack_end=0x7fff0484a838) at ../csu/libc-start.c:310
#45 0x00005625cceebcba in _start ()
Comment 1 Radar WebKit Bug Importer 2018-05-21 06:25:32 PDT 
<rdar://problem/40414695>
Comment 2 Carlos Garcia Campos 2018-05-21 06:28:07 PDT 
Created attachment 340848 [details]
Patch
Comment 3 Michael Catanzaro 2018-05-21 08:18:35 PDT 
CC WK2 owners
Comment 4 Michael Catanzaro 2018-05-21 08:25:52 PDT 
(In reply to Carlos Garcia Campos from comment #0)
> I'm not sure about the security implications of this crash, so I've used the
> security product just in case.

It dies with SIBABRT, so that's totally safe, and at worst this could be a denial of service.

std::optional::value throws a std::bad_optional_access exception, but that becomes abort() because WebKit is built with -fno-exceptions.
Comment 5 Michael Catanzaro 2018-05-21 09:26:16 PDT 
(In reply to Carlos Garcia Campos from comment #0)
> I don't know why this crash didn't happen before to me

A few weeks ago, Yusuke switched us from WTF's implementation of std::optional to the standard library's. That requires GCC 7.
Comment 6 Brent Fulgham 2018-05-21 09:47:26 PDT 
Comment on attachment 340848 [details]
Patch

Good catch! r=me
Comment 7 Alex Christensen 2018-05-21 09:50:17 PDT 
Comment on attachment 340848 [details]
Patch

Why is this not testable?  Why don't we make WebResourceLoader::TrackingParameters::pageID and frameID std::optional, too?
Comment 8 Carlos Garcia Campos 2018-05-22 00:44:47 PDT 
(In reply to Alex Christensen from comment #7)
> Comment on attachment 340848 [details]
> Patch
> 
> Why is this not testable?

I tried to make a test case, but I couldn't. I'm not familiar with SVG, it seems to happen when an image tag inside an svg contains another image, but I haven't managed te create a test. This always happens when visiting http://www.mutua.es. I also tried to extract the test case from that page but failed too. If someone knows how to create a test case for this we can add it in a follow up patch.

> Why don't we make
> WebResourceLoader::TrackingParameters::pageID and frameID std::optional, too?

I don't think that helps. The tracking parameters are used in a lot of log messages, we would need to use value_or(0) everywhere. Then we would also need to make std::optional webPageID and webFrameID in NetworkLoadParameters (or use vale_or(0) when assigning the network load parameters). That would also require a lot of changes in network process code. If we ever get rid of the empty client, those will no longer be optional, so I don't think it's worth all the changes involved.
Comment 9 Carlos Garcia Campos 2018-05-22 00:46:15 PDT 
Committed r232056: <https://trac.webkit.org/changeset/232056>
Comment 10 Carlos Garcia Campos 2018-05-29 04:14:52 PDT 
*** Bug 186048 has been marked as a duplicate of this bug. ***