Bug Hunting Session
Bug 99994 - Crash on insert SVG file svgio::svgreader::SvgCharacterNode::createSimpleTextPrimitive
Summary: Crash on insert SVG file svgio::svgreader::SvgCharacterNode::createSimpleText...
Status: RESOLVED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: filters and storage (show other bugs)
Version:
(earliest affected)
4.4.0.3 release
Hardware: All All
: medium major
Assignee: Not Assigned
URL:
Whiteboard: target:5.3.0 target:5.1.5 target:5.2.0.1
Keywords: bibisected, bisected, filter:svg, patch, regression
Depends on:
Blocks:
 
Reported: 2016-05-22 18:29 UTC by sam tygier
Modified: 2016-10-25 18:55 UTC (History)
6 users (show)

See Also:
Crash report or crash signature:


Attachments
test.svg (1.56 KB, image/svg+xml)
2016-05-22 18:29 UTC, sam tygier
Details
0001-tdf-99994-Avoid-invalid-access-by-reusing-getFontFam.patch (1.50 KB, patch)
2016-05-22 18:57 UTC, sam tygier
Details
test2.svg (211 bytes, image/svg+xml)
2016-05-23 18:40 UTC, sam tygier
Details
4 testfiles (1.89 KB, application/x-zip-compressed)
2016-05-23 21:43 UTC, Regina Henschel
Details
test2.svg (193 bytes, image/svg+xml)
2016-05-24 06:13 UTC, sam tygier
Details

Note You need to log in before you can comment on or make changes to this bug.
Description sam tygier 2016-05-22 18:29:26 UTC
Created attachment 125233 [details]
test.svg

On inserting the attached svg file Libreoffice crashes. Happens with 5.0, 5.1 and 5.2dev (not test with any earlier versions). Happens in impress and writer.

The original SVG came from matplotlib, with edits in inkscape. I cut it down to something fairly minimal.

Steps:
1) New impress or writer document
2) Drag test.svg into the page
3) Crash

Thread 1 "soffice.bin" received signal SIGSEGV, Segmentation fault.
Python Exception <class 'gdb.MemoryError'> Cannot access memory at address 0x0: 
0x00007fffc3f89f82 in rtl::OUString::OUString (this=0x7fffffff3280, 
    str=<error reading variable: Cannot access memory at address 0x0>)
    at /usr/local/src/libreoffice/include/rtl/ustring.hxx:129
129	        pData = str.pData;


Backtrace:

#0  0x00007fffc3f89f82 in rtl::OUString::OUString (this=0x7fffffff3280, 
    str=<error reading variable: Cannot access memory at address 0x0>)
    at /usr/local/src/libreoffice/include/rtl/ustring.hxx:129
#1  0x00007fffc3f8856d in svgio::svgreader::SvgCharacterNode::createSimpleTextPrimitive (this=0x2a4b250, 
    rSvgTextPosition=..., rSvgStyleAttributes=...)
    at /usr/local/src/libreoffice/svgio/source/svgreader/svgcharacternode.cxx:241
#2  0x00007fffc3f893a1 in svgio::svgreader::SvgCharacterNode::decomposeTextWithStyle (this=0x2a4b250, rTarget=..., 
    rSvgTextPosition=..., rSvgStyleAttributes=...)
    at /usr/local/src/libreoffice/svgio/source/svgreader/svgcharacternode.cxx:507
#3  0x00007fffc3f8974b in svgio::svgreader::SvgCharacterNode::decomposeText (this=0x2a4b250, rTarget=..., 
    rSvgTextPosition=...) at /usr/local/src/libreoffice/svgio/source/svgreader/svgcharacternode.cxx:575
#4  0x00007fffc3fbc175 in svgio::svgreader::SvgTextNode::DecomposeChild (this=0x2a4b410, rCandidate=..., rTarget=..., 
    rSvgTextPosition=...) at /usr/local/src/libreoffice/svgio/source/svgreader/svgtextnode.cxx:120
#5  0x00007fffc3fbc40d in svgio::svgreader::SvgTextNode::DecomposeChild (this=0x2a4b410, rCandidate=..., rTarget=..., 
    rSvgTextPosition=...) at /usr/local/src/libreoffice/svgio/source/svgreader/svgtextnode.cxx:173
#6  0x00007fffc3fbc87a in svgio::svgreader::SvgTextNode::decomposeSvgNode (this=0x2a4b410, rTarget=...)
    at /usr/local/src/libreoffice/svgio/source/svgreader/svgtextnode.cxx:245
#7  0x00007fffc3fa45c5 in svgio::svgreader::SvgNode::decomposeSvgNode (this=0x2a4ae10, rTarget=..., bReferenced=false)
    at /usr/local/src/libreoffice/svgio/source/svgreader/svgnode.cxx:540
#8  0x00007fffc3f9a352 in svgio::svgreader::SvgGNode::decomposeSvgNode (this=0x2a4ae10, rTarget=..., bReferenced=false)
    at /usr/local/src/libreoffice/svgio/source/svgreader/svggnode.cxx:112
#9  0x00007fffc3fa45c5 in svgio::svgreader::SvgNode::decomposeSvgNode (this=0x2a49870, rTarget=..., bReferenced=false)
    at /usr/local/src/libreoffice/svgio/source/svgreader/svgnode.cxx:540
#10 0x00007fffc3fb8da4 in svgio::svgreader::SvgSvgNode::decomposeSvgNode (this=0x2a49870, rTarget=..., bReferenced=false)
    at /usr/local/src/libreoffice/svgio/source/svgreader/svgsvgnode.cxx:307
#11 0x00007fffc3fd3bfe in svgio::svgreader::XSvgParser::getDecomposition (this=0x2220e10, 
    xSVGStream=uno::Reference to (comphelper::SequenceInputStream *) 0x201f428, 
    aAbsolutePath="file:///home/sam/bugs/libreoffice/svg_crash/test3.svg")
    at /usr/local/src/libreoffice/svgio/source/svguno/xsvgparser.cxx:160
#12 0x00007ffff00cf299 in SvgData::ensureSequenceAndRange (this=0x2a3ea40)
    at /usr/local/src/libreoffice/vcl/source/gdi/svgdata.cxx:120
#13 0x00007ffff00cf8ec in SvgData::getRange (this=0x2a3ea40) at /usr/local/src/libreoffice/vcl/source/gdi/svgdata.cxx:189
#14 0x00007fffeffb2911 in ImpGraphic::ImplGetPrefSize (this=0x2a40e60)
    at /usr/local/src/libreoffice/vcl/source/gdi/impgraph.cxx:662
#15 0x00007fffeffa9824 in Graphic::GetPrefSize (this=0x7fffffff4900)
    at /usr/local/src/libreoffice/vcl/source/gdi/graph.cxx:390
#16 0x00007fffccf4635a in sd::View::InsertGraphic (this=0x1bec4e0, rGraphic=..., rAction=@0x7fffffff4980: 2 '\002', 
    rPos=Point = {...}, pObj=0x0, pImageMap=0x0) at /usr/local/src/libreoffice/sd/source/ui/view/sdview4.cxx:190
#17 0x00007fffccf47b16 in sd::View::DropInsertFileHdl (this=0x1bec4e0)
    at /usr/local/src/libreoffice/sd/source/ui/view/sdview4.cxx:429
#18 0x00007fffccf47733 in sd::View::LinkStubDropInsertFileHdl (instance=0x1bec4e0, data=0x1becc38)
    at /usr/local/src/libreoffice/sd/source/ui/view/sdview4.cxx:394
#19 0x00007fffefc6df81 in Link<Idle*, void>::Call (this=0x1becc58, data=0x1becc38)
    at /usr/local/src/libreoffice/include/tools/link.hxx:84
#20 0x00007ffff01131bd in Idle::Invoke (this=0x1becc38) at /usr/local/src/libreoffice/vcl/source/app/idle.cxx:25
#21 0x00007ffff0116b12 in ImplSchedulerData::Invoke (this=0x24d5470)
    at /usr/local/src/libreoffice/vcl/source/app/scheduler.cxx:45
#22 0x00007ffff0116f6d in Scheduler::ProcessTaskScheduling (bTimerOnly=false)
    at /usr/local/src/libreoffice/vcl/source/app/scheduler.cxx:177
#23 0x00007ffff0136cae in ImplYield (i_bWait=false, i_bAllEvents=false, nReleased=0)
    at /usr/local/src/libreoffice/vcl/source/app/svapp.cxx:523
#24 0x00007ffff0132df6 in Application::Yield () at /usr/local/src/libreoffice/vcl/source/app/svapp.cxx:556
#25 0x00007ffff0132c70 in Application::Execute () at /usr/local/src/libreoffice/vcl/source/app/svapp.cxx:473
#26 0x00007ffff7811251 in desktop::Desktop::DoExecute () at /usr/local/src/libreoffice/desktop/source/app/app.cxx:1320
#27 0x00007ffff78122f9 in desktop::Desktop::Main (this=0x7fffffff5470)
    at /usr/local/src/libreoffice/desktop/source/app/app.cxx:1645
#28 0x00007ffff013be7a in ImplSVMain () at /usr/local/src/libreoffice/vcl/source/app/svmain.cxx:170
#29 0x00007ffff013bfbc in SVMain () at /usr/local/src/libreoffice/vcl/source/app/svmain.cxx:208
#30 0x00007ffff7856b8e in soffice_main () at /usr/local/src/libreoffice/desktop/source/app/sofficemain.cxx:135
#31 0x0000000000400815 in sal_main () at /usr/local/src/libreoffice/desktop/source/app/main.c:48
#32 0x00000000004007fb in main (argc=2, argv=0x7fffffff57a8) at /usr/local/src/libreoffice/desktop/source/app/main.c:47
Comment 1 sam tygier 2016-05-22 18:48:54 UTC
The bug occurs in SvgCharacterNode::createSimpleTextPrimitive() in the section:

    OUString aFontFamily = rSvgStyleAttributes.getFontFamily().empty() ?
        OUString("Times New Roman") :
        rSvgStyleAttributes.getFontFamily()[0];

The second clause gets called, and tries to access the zeroth element of an empty vector. The "empty()" test is not effective at preventing this as getFontFamily() returns a different vector on the second call. (Fun Heisenbug as putting "cout << rSvgStyleAttributes.getFontFamily().size()" or similar above changes the behaviour.)

The problem is that getFontFamily() can call SvgStyleAttributes::setCssStyleParent() and so the style can be changed.

The simple fix is to just reuse the result from the first call to getFontFamily(). That way the test is effective and preventing the invalid access.

I guess proper fix is to make getFontFamily() deterministic. I have not yet quite understood the code well enough to understand how. Perhaps it needs to be made so that that all the style resolution that can call the the 'set' methods occurs before createSimpleTextPrimitive() gets called.
Comment 2 sam tygier 2016-05-22 18:57:38 UTC
Created attachment 125234 [details]
0001-tdf-99994-Avoid-invalid-access-by-reusing-getFontFam.patch

This is the quick fix, but maybe a more in depth approach is needed.
Comment 3 Aron Budea 2016-05-22 19:47:50 UTC
Thanks for the bug report and the analysis, Sam.
Reproduced in current versions, and in 4.4.0.3, not reproduced in 4.3.0.4, so it's a regression.
Comment 4 Aron Budea 2016-05-23 00:21:24 UTC
(In reply to sam tygier from comment #2)
> This is the quick fix, but maybe a more in depth approach is needed.

Yes, I agree. While the patch solves the crash, the bug that caused two consecutive getFontFamily() calls to return different font families would remain hidden, and might cause issues elsewhere.
Comment 5 sam tygier 2016-05-23 18:40:46 UTC
Created attachment 125249 [details]
test2.svg

Here as more minimal example file. I think the required conditions are: A text element, with a parent which has a style attribute containing a font, and a top level css style element with a "*" rule that does not contain font.

In this case I thing the first call to getFontFamily() gives the parent's font, and the second call gives the empty list from the style element.
Comment 6 Regina Henschel 2016-05-23 21:43:23 UTC
Created attachment 125251 [details]
4 testfiles

Your test2.svg does not crash for me. But you are right in your analysis, the problem is the combination of the * in the style element and a font-family rule on the parent.
Comment 7 sam tygier 2016-05-24 06:13:47 UTC
Created attachment 125255 [details]
test2.svg

I uploaded the wrong test2.svg. here is the crashy version.
Comment 8 raal 2016-05-25 20:13:06 UTC
This seems to have begun at the below commit.
Adding Cc: to Armin ; Could you possibly take a look at this one?
Thanks

1014698a0bea59d60f4e092c7dbd35a2e1473371 is the first bad commit
commit 1014698a0bea59d60f4e092c7dbd35a2e1473371
Author: Matthew Francis <mjay.francis@gmail.com>
Date:   Sun Mar 15 03:52:44 2015 +0800

    source-hash-e17a730c0076b10678c860ae3285bc8a98282415
    
    commit e17a730c0076b10678c860ae3285bc8a98282415
    Author:     Armin Le Grand <alg@apache.org>
    AuthorDate: Thu Oct 9 15:03:55 2014 +0000
    Commit:     Caolán McNamara <caolanm@redhat.com>
    CommitDate: Thu Oct 9 17:30:13 2014 +0100
    
        Resolves: #i125329# Take care of Css selector '*'
    
        (cherry picked from commit f73813d9e0f13e3bdf735f8626dbf540701a1900)
    
        Conflicts:
        	svgio/source/svgreader/svgnode.cxx
    
        Change-Id: Ifc5df8bed47d69709ef590eced19635b6b9580d0
Comment 9 sam tygier 2016-05-29 15:11:04 UTC
Without that commit the "*" selector is not being used, so the bug would not be triggered.

In the process of debugging this i'd like to write some tests, but I am having trouble with the "assertXPath()" method in SvgImportTest. Is there a way to show the xmlDocPtr so that I can work out what the paths should be for a given svg file? I keep hitting "In <>, XPath '/primitive2D/transform/text' number of nodes is incorrect" messages.
Comment 10 Xisco Faulí 2016-05-30 19:18:23 UTC
it looks like the problem is cause by * embedded by style. Working on a solution...
Comment 11 Xisco Faulí 2016-05-30 20:37:00 UTC
Hi Sam, I've analysed your patch and it seems correct to me. The problem is that getFontFamily() is a recursive call,thus we were getting different results for rSvgStyleAttributes.getFontFamily().empty() and rSvgStyleAttributes.getFontFamily()[0].

Could you please summit your patch to gerrit (1) ? Meanwhile, I will create a unittest for this issue.

(1) https://wiki.documentfoundation.org/Development/gerrit
Comment 12 Xisco Faulí 2016-05-30 20:39:54 UTC
Unittest created: https://gerrit.libreoffice.org/#/c/25690/1
Comment 13 Aron Budea 2016-05-30 21:56:51 UTC
Xisco, please see my comment 4.
While I have no idea what's going on inside, I have a very strong suspicion getFontFamily() is not supposed to return different results on subsequent calls.
If that's the case, it's much easier to fix the underlying issue when it causes a straightforward crash, compared to obscure unexpected behavior that might resurface some other time.
Comment 14 Xisco Faulí 2016-05-30 23:05:02 UTC
Yes, things become complecated when there're more than one cssstyle affecting one element as the cssstylesvector in http://opengrok.libreoffice.org/xref/core/svgio/source/svgreader/svgnode.cxx#165 is not always build in the proper order. I've reproduced crashes myself with other files, for instance, adding the local attributes to the vector before the selectors, which by the way, I don't recommend you to try as it will hang you operative system at build time unless you disable the unittests.
Anyway, recently I've been taking a look at it when I've had some time and I hope to have a solution soon, however, it's not that easy to solve ( at least for me ), so I believe we can merge Sam's patch for the moment mainly for two reasons:
1. It fixes a crash which is a major problem from the QA point of view and we can port it to libreoffice 5.1 and 5.2.
2. I'm adding an unittest. Specially helpful when investigating the problem described above.
Comment 15 Aron Budea 2016-05-31 00:03:09 UTC
Okay, I can stand behind the reasoning that getting rid of the crash is very important.

What's the unit test result after the quick fix? My main concern is that the underlying issue should remain reproducible afterwards in some way, but if the unit test does that, my concern is no more.
Comment 16 sam tygier 2016-05-31 07:27:33 UTC
Patch pushed to gerrit https://gerrit.libreoffice.org/#/c/25704/ (my first time, so let me know if there is any mistake).

Even once a full fix is in, I think this patch is still useful because getFontFamily() is a slow function (it must walk up the style hierarchy) so its best to only call it once.

In terms of a test to find the underlying issue, I suspect there are cases where the style resolution will do the wrong thing when confronted with conflicting styles. The current approach is pretty hard to reason about because works be walking up through the parents, but also reordering the parent list in http://opengrok.libreoffice.org/xref/core/svgio/source/svgreader/svgnode.cxx#207

There is a lot ways any element could have style applied, https://www.w3.org/TR/SVG11/styling.html#UsingPresentationAttributes, https://www.w3.org/TR/SVG11/styling.html#StylingWithCSS and https://www.w3.org/TR/SVG11/styling.html#Inheritance also the selectors must follow https://www.w3.org/TR/2008/REC-CSS2-20080411/cascade.html#q12
Comment 17 Armin Le Grand 2016-05-31 10:12:28 UTC
Hi, I will try to keep an eye on thhis one, it seems that the orig merge caused conflicts and that it was merged incomplete/uncorrect - I'll have to check what exactly I'd done there. I remember that it was about styles support, demanded/requested by Regina (who is already on CC)...
Comment 18 Xisco Faulí 2016-05-31 10:19:09 UTC
Hi Armin,
Just for the record, your commit also introduced bug 99115, which looks to be related to the css style hierachy too
Comment 19 Aron Budea 2016-06-01 00:58:56 UTC
I thought about what should happen with this report after the fix is in, and suggest to keep it open with the specific guide for the developer to temporarily undo the fix in their system, take care of the underlying issue until the crash is gone, and publish their fix (while keeping Sam's fix as well).
It's a bit inconvenient, but the steps are straightforward, and it won't get forgotten.
Comment 20 Commit Notification 2016-06-01 08:28:25 UTC
Sam Tygier committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=68ccab350ca5b907f185c729e94a14df15fedc23

tdf#99994 Avoid invalid access by reusing getFontFamily() result

It will be available in 5.3.0.

The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 21 Commit Notification 2016-06-01 12:06:50 UTC
Xisco Fauli committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=9bf1dac4be55f8591d5fd2b8aa93c9369191c02c

tdf#99994: Add unittest

It will be available in 5.3.0.

The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 22 Commit Notification 2016-06-02 10:34:38 UTC
Sam Tygier committed a patch related to this issue.
It has been pushed to "libreoffice-5-1":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=e687739eb4849014f7f2657e94d8ab67e4d74625&h=libreoffice-5-1

tdf#99994 Avoid invalid access by reusing getFontFamily() result

It will be available in 5.1.5.

The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 23 Commit Notification 2016-06-02 10:42:24 UTC
Sam Tygier committed a patch related to this issue.
It has been pushed to "libreoffice-5-2":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=08c8d094390c7ccedde7d9c04c503a62ed907ae2&h=libreoffice-5-2

tdf#99994 Avoid invalid access by reusing getFontFamily() result

It will be available in 5.2.0.1.

The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 24 Caolán McNamara 2016-06-02 11:54:43 UTC
can we close this as fixed now ?
Comment 25 sam tygier 2016-06-02 12:17:08 UTC
There is still an underlying issue that is not fixed (style resolution is sometimes (maybe always) wrong in some cases). The merged patch just means that we don't crash in that case (also reduces unneeded work).

So it would be good to have real fix, that works without the current patch, even though I think the current patch should remain.

I have test case in progress to show the wrong style attributes being picked, and I suspect that a sensible solution that passes the test probably resolves the underlying issue.

So, do we continue that work here (as per comment 19), or leave this bug for the crash, and start a new bug for the deeper issue?
Comment 26 Xisco Faulí 2016-06-02 12:48:47 UTC
I'd rather to close this one and open a new one, but that's just my 2 cents
Comment 27 sam tygier 2016-06-02 22:24:18 UTC
I opened bug 100198 for the underlying issue, so we can close this.