Bug 381428

Summary: Crash in svg parser when loading svg symbols
Product: [Applications] krita Reporter: wolthera <griffinvalley>
Component: Layers/VectorAssignee: Krita Bugs <krita-bugs-null>
Status: RESOLVED FIXED    
Severity: crash CC: dimula73, halla, mikael.rosbacke
Priority: NOR    
Version First Reported In: git master (please specify the git hash!)   
Target Milestone: ---   
Platform: Compiled Sources   
OS: Linux   
Latest Commit: Version Fixed/Implemented In:
Sentry Crash Report:
Attachments: This file crashes Krita when put into the symbols/
Another one that crashes.
Diff to avoid having krita crash but will lose data in loaded files.
Attempt to partially fix the use element.

Description wolthera 2017-06-20 09:29:31 UTC 
Created attachment 106181 [details]
This file crashes Krita when put into the symbols/

This is different from bug 381224 as that one is when using the docker, while this one is on start up.

Put the attached file in the symbols resource directory, and Krita will crash on start up.

I suspect it is because I used Inkscape's "linked clone" feature, which translates to SVG <use> tags referencing a group within the symbol.

I seem to have lost the location of my saved backtrace, I will attach it when I find it :D
Comment 1 Halla Rempt 2017-06-20 09:33:23 UTC 
#0  0x00007ffff52cbcd0 in KoShape::parent() const (this=this@entry=0x0)
    at /home/boud/dev/krita/libs/flake/KoShape.cpp:1176
#1  0x00007ffff535c9a7 in KoShapeGroupCommandPrivate::init(KUndo2Command*) (this=0x1acb820, q=q@entry=
    0x7fffffffb9c0) at /home/boud/dev/krita/libs/flake/commands/KoShapeGroupCommand.cpp:100
#2  0x00007ffff535d1cf in KoShapeGroupCommand::KoShapeGroupCommand(KoShapeContainer*, QList<KoShape*> const&, bool, bool, bool, KUndo2Command*) (this=0x7fffffffb9c0, container=
    0x1a9de70, shapes=..., clipped=false, inheritTransform=true, shouldNormalize=<optimized out>, parent=0x0)
    at /home/boud/dev/krita/libs/flake/commands/KoShapeGroupCommand.cpp:83
#3  0x00007ffff53acbad in SvgParser::addToGroup(QList<KoShape*>, KoShapeGroup*) (this=this@entry=0x7fffffffc850, shapes=..., group=group@entry=0x1a9de70) at /home/boud/dev/krita/libs/flake/svg/SvgParser.cpp:1165
#4  0x00007ffff53ace16 in SvgParser::parseGroup(QDomElement const&, QDomElement const&) (this=this@entry=0x7fffffffc850, b=..., overrideChildrenFrom=...) at /home/boud/dev/krita/libs/flake/svg/SvgParser.cpp:1305
#5  0x00007ffff53ad807 in SvgParser::parseSymbol(QDomElement const&) (this=this@entry=0x7fffffffc850, e=...)
    at /home/boud/dev/krita/libs/flake/svg/SvgParser.cpp:606
#6  0x00007ffff53af2b5 in SvgParser::parseSingleElement(QDomElement const&) (this=this@entry=0x7fffffffc850, b=...) at /home/boud/dev/krita/libs/flake/svg/SvgParser.cpp:1391
#7  0x00007ffff53afb7c in SvgParser::parseContainer(QDomElement const&) (this=this@entry=0x7fffffffc850, e=...)
    at /home/boud/dev/krita/libs/flake/svg/SvgParser.cpp:1342
#8  0x00007ffff53ace75 in SvgParser::parseGroup(QDomElement const&, QDomElement const&) (this=this@entry=0x7fffffffc850, b=..., overrideChildrenFrom=...) at /home/boud/dev/krita/libs/flake/svg/SvgParser.cpp:1299
#9  0x00007ffff53af080 in SvgParser::parseSingleElement(QDomElement const&) (this=this@entry=0x7fffffffc850, b=...) at /home/boud/dev/krita/libs/flake/svg/SvgParser.cpp:1376
#10 0x00007ffff53afb7c in SvgParser::parseContainer(QDomElement const&) (this=this@entry=0x7fffffffc850, e=...)
    at /home/boud/dev/krita/libs/flake/svg/SvgParser.cpp:1342
#11 0x00007ffff53b2038 in SvgParser::parseSvg(QDomElement const&, QSizeF*) (this=this@entry=0x7fffffffc850, e=..., fragmentSize=fragmentSize@entry=0x7fffffffc7a0) at /home/boud/dev/krita/libs/flake/svg/SvgParser.cpp:1226
#12 0x00007ffff53d2129 in KoSvgSymbolCollectionResource::loadFromDevice(QIODevice*) (this=
    0xf48bf0, dev=<optimized out>)
    at /home/boud/dev/krita/libs/flake/resources/KoSvgSymbolCollectionResource.cpp:135
#13 0x00007ffff53d1785 in KoSvgSymbolCollectionResource::load() (this=0xf48bf0)
    at /home/boud/dev/krita/libs/flake/resources/KoSvgSymbolCollectionResource.cpp:84
#14 0x00007ffff5920eef in KoResourceServer<KoSvgSymbolCollectionResource, PointerStoragePolicy<KoSvgSymbolCollectionResource> >::loadResources(QStringList) (this=0xf306c0, filenames=...)
    at /home/boud/dev/krita/libs/widgets/KoResourceServer.h:199
#15 0x00007ffff5919a3f in KoResourceLoaderThread::loadSynchronously() (this=this@entry=0xf46bd0)
    at /home/boud/dev/krita/libs/widgets/KoResourceServerProvider.cpp:140
#16 0x00007ffff591abad in KoResourceServerProvider::KoResourceServerProvider() (this=0x7ffff5b97e90 <(anonymous namespace)::Q_QGS_s_instance::innerFunction()::holder>)
    at /home/boud/dev/krita/libs/widgets/KoResourceServerProvider.cpp:212
#17 0x00007ffff591b02c in KoResourceServerProvider::instance() (this=0x7ffff5b97e90 <(anonymous namespace)::Q_QGS_s_instance::innerFunction()::holder>) at /home/boud/dev/krita/libs/widgets/KoResourceServerProvider.cpp:230
#18 0x00007ffff591b02c in KoResourceServerProvider::instance() ()
    at /home/boud/dev/krita/libs/widgets/KoResourceServerProvider.cpp:230
#19 0x00007ffff591b02c in KoResourceServerProvider::instance() (this=<optimized out>)
    at /home/boud/dev/deps/include/QtCore/qglobalstatic.h:128
#20 0x00007ffff591b02c in KoResourceServerProvider::instance() ()
    at /home/boud/dev/krita/libs/widgets/KoResourceServerProvider.cpp:234
#21 0x00007ffff78afc80 in KisApplication::loadResources() (this=this@entry=0x7fffffffd560)
    at /home/boud/dev/krita/libs/ui/KisApplication.cpp:281
#22 0x00007ffff78b33b0 in KisApplication::start(KisApplicationArguments const&) (this=this@entry=0x7fffffffd560, args=...) at /home/boud/dev/krita/libs/ui/KisApplication.cpp:413
#23 0x00000000004053d9 in main(int, char**) (argc=1, argv=<optimized out>)
    at /home/boud/dev/krita/krita/main.cc:254
Comment 2 wolthera 2017-08-23 14:13:10 UTC 
Created attachment 107479 [details]
Another one that crashes.

And this one gives this crash:

Application: krita (krita), signal: Segmentation fault
Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
[Current thread is 1 (Thread 0x7f87235798c0 (LWP 4992))]

Thread 3 (Thread 0x7f87159f6700 (LWP 4994)):
#0  0x00007f872bfeaa79 in g_mutex_lock () from /lib/x86_64-linux-gnu/libglib-2.0.so.0
#1  0x00007f872bfa6372 in ?? () from /lib/x86_64-linux-gnu/libglib-2.0.so.0
#2  0x00007f872bfa649c in g_main_context_iteration () from /lib/x86_64-linux-gnu/libglib-2.0.so.0
#3  0x00007f873398b94b in QEventDispatcherGlib::processEvents(QFlags<QEventLoop::ProcessEventsFlag>) () from /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
#4  0x00007f87339347ca in QEventLoop::exec(QFlags<QEventLoop::ProcessEventsFlag>) () from /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
#5  0x00007f873375dcd4 in QThread::exec() () from /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
#6  0x00007f872d7cbb75 in ?? () from /usr/lib/x86_64-linux-gnu/libQt5DBus.so.5
#7  0x00007f8733762989 in ?? () from /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
#8  0x00007f872d5a06ba in start_thread (arg=0x7f87159f6700) at pthread_create.c:333
#9  0x00007f8732e593dd in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:109

Thread 2 (Thread 0x7f87210ff700 (LWP 4993)):
#0  0x00007f8732e4d70d in poll () at ../sysdeps/unix/syscall-template.S:84
#1  0x00007f8731502c62 in ?? () from /usr/lib/x86_64-linux-gnu/libxcb.so.1
#2  0x00007f87315048d7 in xcb_wait_for_event () from /usr/lib/x86_64-linux-gnu/libxcb.so.1
#3  0x00007f8723447329 in ?? () from /usr/lib/x86_64-linux-gnu/libQt5XcbQpa.so.5
#4  0x00007f8733762989 in ?? () from /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
#5  0x00007f872d5a06ba in start_thread (arg=0x7f87210ff700) at pthread_create.c:333
#6  0x00007f8732e593dd in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:109

Thread 1 (Thread 0x7f87235798c0 (LWP 4992)):
[KCrash Handler]
#6  KoShape::parent (this=this@entry=0x0) at /home/wolthera/krita/src/libs/flake/KoShape.cpp:1177
#7  0x00007f873045d89c in KoShapeGroupCommandPrivate::init (this=0x7067a30, q=q@entry=0x7fff8196c3a0) at /home/wolthera/krita/src/libs/flake/commands/KoShapeGroupCommand.cpp:100
#8  0x00007f873045df2f in KoShapeGroupCommand::KoShapeGroupCommand (this=0x7fff8196c3a0, container=0x6dc8400, shapes=..., clipped=false, inheritTransform=true, shouldNormalize=<optimized out>, parent=0x0) at /home/wolthera/krita/src/libs/flake/commands/KoShapeGroupCommand.cpp:83
#9  0x00007f87304acb49 in SvgParser::addToGroup (this=this@entry=0x7fff8196ccc0, shapes=..., group=group@entry=0x6dc8400) at /home/wolthera/krita/src/libs/flake/svg/SvgParser.cpp:1165
#10 0x00007f87304acdc5 in SvgParser::parseGroup (this=this@entry=0x7fff8196ccc0, b=..., overrideChildrenFrom=...) at /home/wolthera/krita/src/libs/flake/svg/SvgParser.cpp:1305
#11 0x00007f87304af036 in SvgParser::parseSingleElement (this=this@entry=0x7fff8196ccc0, b=...) at /home/wolthera/krita/src/libs/flake/svg/SvgParser.cpp:1364
#12 0x00007f87304afc0c in SvgParser::parseContainer (this=this@entry=0x7fff8196ccc0, e=...) at /home/wolthera/krita/src/libs/flake/svg/SvgParser.cpp:1342
#13 0x00007f87304b220e in SvgParser::parseSvg (this=this@entry=0x7fff8196ccc0, e=..., fragmentSize=fragmentSize@entry=0x7fff8196cc10) at /home/wolthera/krita/src/libs/flake/svg/SvgParser.cpp:1226
#14 0x00007f87304d2a0d in KoSvgSymbolCollectionResource::loadFromDevice (this=0x66291d0, dev=<optimized out>) at /home/wolthera/krita/src/libs/flake/resources/KoSvgSymbolCollectionResource.cpp:135
#15 0x00007f87304d2015 in KoSvgSymbolCollectionResource::load (this=0x66291d0) at /home/wolthera/krita/src/libs/flake/resources/KoSvgSymbolCollectionResource.cpp:84
#16 0x00007f8730a1a93a in KoResourceServer<KoSvgSymbolCollectionResource, PointerStoragePolicy<KoSvgSymbolCollectionResource> >::loadResources (this=0x65f8c00, filenames=...) at /home/wolthera/krita/src/libs/widgets/KoResourceServer.h:199
#17 0x00007f8730a1384f in KoResourceLoaderThread::run (this=0x653d180) at /home/wolthera/krita/src/libs/widgets/KoResourceServerProvider.cpp:145
#18 0x00007f8730a14006 in KoResourceServerProvider::KoResourceServerProvider (this=0x7f8730c93e80 <_ZZN12_GLOBAL__N_116Q_QGS_s_instance13innerFunctionEvE6holder>) at /home/wolthera/krita/src/libs/widgets/KoResourceServerProvider.cpp:212
#19 0x00007f8730a141cc in (anonymous namespace)::Q_QGS_s_instance::Holder::Holder (this=0x7f8730c93e80 <_ZZN12_GLOBAL__N_116Q_QGS_s_instance13innerFunctionEvE6holder>) at /home/wolthera/krita/src/libs/widgets/KoResourceServerProvider.cpp:230
#20 (anonymous namespace)::Q_QGS_s_instance::innerFunction () at /home/wolthera/krita/src/libs/widgets/KoResourceServerProvider.cpp:230
#21 QGlobalStatic<KoResourceServerProvider, (anonymous namespace)::Q_QGS_s_instance::innerFunction, (anonymous namespace)::Q_QGS_s_instance::guard>::operator QGlobalStatic<KoResourceServerProvider, (anonymous namespace)::Q_QGS_s_instance::innerFunction, (anonymous namespace)::Q_QGS_s_instance::guard>::Type* (this=<synthetic pointer>) at /usr/include/x86_64-linux-gnu/qt5/QtCore/qglobalstatic.h:134
#22 KoResourceServerProvider::instance () at /home/wolthera/krita/src/libs/widgets/KoResourceServerProvider.cpp:234
#23 0x00007f8735487c50 in KisApplication::loadResources (this=this@entry=0x7fff8196d9e0) at /home/wolthera/krita/src/libs/ui/KisApplication.cpp:281
#24 0x00007f873548bb54 in KisApplication::start (this=this@entry=0x7fff8196d9e0, args=...) at /home/wolthera/krita/src/libs/ui/KisApplication.cpp:413
#25 0x0000000000404fd5 in main (argc=1, argv=<optimized out>) at /home/wolthera/krita/src/krita/main.cc:254
Comment 3 Mikael Rosbacke 2017-09-08 12:33:14 UTC 
Created attachment 107748 [details]
Diff to avoid having krita crash but will lose data in loaded files.
Comment 4 Mikael Rosbacke 2017-09-08 12:35:24 UTC 
Been investigating this as a step to get to know the Krita code base. Used master at 2017-09-08 and commit 91be09796c57f642020f2e75720e04eb318c96b4.

Can confirm the crash. Culprit is SVG parsing of the 'use' elements. The function KoShape* SvgParser::parseUse(const KoXmlElement &e) can return nullptr which gets inserted into the 'shapes' list and later generate the SIGSEGV.

Simple fix is to check for nullptr before inserting. However this is not a full fix. It would mean losing data as the use element refer to some element that has not yet been defined. 
What would be needed is to delay evaluation of the use element until the point where its reference is available. Another solution is to implement a 2-pass parsing. This is a bit much for an initial look at a new codebase. Thought I'd at least document my findings here.

The following snippet should fix the crash it but will lose data:
I've also attached a bit longer diff with some more asserts and debug output when problem arrives.


@@ -1404,7 +1428,10 @@ QList<KoShape*> SvgParser::parseSingleElement(const KoXmlElement &b)
         if (shape)
             shapes.append(shape);
     } else if (b.tagName() == "use") {
-        shapes += parseUse(b);
+        KoShape *shape = parseUse(b);
+        if (shape) {
+            shapes.append(shape);
+        }
     } else if (b.tagName() == "color-profile") {
         m_context.parseProfile(b);
     } else {
Comment 5 Halla Rempt 2017-09-08 14:00:31 UTC 
Checking for 0 is a good thing in any case, but, as you say, this really should be fixed at a deeper level.
Comment 6 Mikael Rosbacke 2017-09-09 11:34:42 UTC 
Created attachment 107771 [details]
Attempt to partially fix the use element.

Thought a bit more. In general I think use elements can refer to any other element in the svg, but in practice often it refers within the same group, just a bit later. In this case the context for the element is still the same on the context stack.
I've implemented separate storage that stores 'use' elements for the duration of their current group. For each new element scanned, this store is checked and 'use' elements are processed as their resources show up. Also some small fixes to remove error logs that doesn't seem like errors.

With this code, at least the first image can be read without losing 'use' elements. I get one lost element but inspecting that file shows that it is truly missing. But I havn't been able to actually look at the read data.
Feel free to apply, or use as an inspiration for a proper solution.

/ MR
Comment 7 Halla Rempt 2017-09-13 08:20:33 UTC 
Thanks! I'll point Dmitry at your patch -- he wrote the original parser, so he's much deeper into the code than I am.
Comment 8 Dmitry Kazakov 2017-09-25 18:53:22 UTC 
I have just pushed the proposed patch:
https://commits.kde.org/krita/d5418bb57535a3719e4de8565e439e03fd0f6002

It is almost correct, except that it reorders the shapes, which doesn't
conform to the standard