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.

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.

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.

(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.

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.

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.

Created attachment 125255 [details]
test2.svg

I uploaded the wrong test2.svg. here is the crashy version.

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

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.

it looks like the problem is cause by * embedded by style. Working on a solution...

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

Unittest created: https://gerrit.libreoffice.org/#/c/25690/1

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.

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.

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.

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

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)...

Hi Armin,
Just for the record, your commit also introduced bug 99115, which looks to be related to the css style hierachy too

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.

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.

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.

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.

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.

can we close this as fixed now ?

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?

I'd rather to close this one and open a new one, but that's just my 2 cents

I opened bug 100198 for the underlying issue, so we can close this.