CDR import crashes after lib2geom update (rev >= 14226)

[su_v]

Internal import of CDR files crashes since update of lib2geom in rev 14226.

terminate called throwing an exception
Program received signal SIGABRT, Aborted.
0x0000000104446ce2 in __pthread_kill ()
(gdb) bt
#0 0x0000000104446ce2 in __pthread_kill ()
#1 0x00000001043057d2 in pthread_kill ()
#2 0x00000001042f6a7a in abort ()
#3 0x00000001049b07bc in abort_message ()
#4 0x00000001049adfcf in default_terminate ()
#5 0x00000001047db1cd in _objc_terminate ()
#6 0x00000001049ae001 in safe_handler_caller ()
#7 0x00000001049ae05c in std::terminate ()
#8 0x00000001049af152 in __cxa_throw ()
#9 0x0000000100808835 in Geom::Path::do_append ()
#10 0x00000001007df270 in Geom::BezierCurveN<3u>::feed ()
#11 0x0000000100834f50 in Geom::SVGPathParser::_curveTo ()
#12 0x000000010083392a in Geom::SVGPathParser::_parse ()
#13 0x00000001003afc0e in sp_svg_read_pathv ()
#14 0x0000000100137a49 in SPPath::set ()
#15 0x0000000100137767 in SPPath::build ()
#16 0x0000000100131605 in SPObject::invoke_build ()
#17 0x000000010013195a in SPObject::build ()
#18 0x000000010013ed5f in SPRoot::build ()
#19 0x0000000100131605 in SPObject::invoke_build ()
#20 0x0000000100038465 in SPDocument::createDoc ()
#21 0x0000000100038fe7 in SPDocument::createNewDocFromMem ()
#22 0x0000000100208410 in Inkscape::Extension::Internal::CdrInput::open ()
#23 0x00000001001fd7ee in Inkscape::Extension::Input::open ()
#24 0x00000001001fb4c4 in Inkscape::Extension::open ()
#25 0x0000000100048a33 in sp_file_open ()
#26 0x0000000100049aec in sp_file_open_dialog ()
<...>

Reproduced with Inkscape 0.91+devel r14341 on
- Ubuntu 14.04 (inkscape PPA as well as local build)
- OS X 10.7.5 (local builds)

Not reproduced with
- Inkscape 0.91 r13725, Ubuntu 14.04
- Inkscape 0.91+devel rev <= 14224 (archived builds), OS X 10.7.5

Simple test case from bug #442010:
https://bugs.launchpad.net/inkscape/+bug/442010/+attachment/771958/+files/Frenchcurve.cdr

More complex test case from sK1 project (UniConvertor):
http://sk1project.org/files/terra.cdr

[su_v (suv-lp)]

Backtrace from attempt to open 'Frenchcurve.cdr' with Inkscape 0.91+devel r14341 (debug build, 64bit, OS X 10.7.5).

[Alvin Penner (apenner)]

not reproduced using Windows XP, Inkscape 0.91+devel r14338 (Sep 4 2015)
and the file FrenchCurve.cdr

[Alvin Penner (apenner)]

reproduced using Windows XP, Inkscape 0.91+devel r14338 (Sep 4 2015)
 and the file terra.cdr

unfortunately, I was not able to produce a backtrace, the program just hangs indefinitely while slowly using more memory

[Alvin Penner (apenner)]

actually it was a bit more complicated than that. Under devel r14338 (Sep 4 2015)
  and the file terra.cdr, the file actually does load successfully, but it takes about a minute or more. The loaded file appears to look correct, and it can also be saved as svg, but the saved svg file is 33 MB, so it was not included here.

[su_v (suv-lp)]

On 2015-09-04 17:26 (+0200), Alvin Penner wrote:
> not reproduced using Windows XP, Inkscape 0.91+devel r14338 (Sep 4 2015)

I can confirm that current Inkscape trunk builds compiled against older versions of libcdr (0.0.10, 0.0.14) open both test cases without crash (these build enviroments use older versions of all dependencies, including boost) - the Windows devlibs 32bit currently include libcdr 0.0.14 (according to the pc file); thus these results seem to match.

The Inkscape trunk builds on OS X which crash on CDR import are compiled against librevenge-based libcdr 0.1.1 (git master, pulled and rebuilt today). The commandline tool of libcdr (git master) converts the file without error (xhtml version with embedded SVG of Frenchcurve.cdr attached).

Ubuntu 14.04.3 LTS has libcdr 0.0.15 installed. Both files crash inkscape on load (same backtrace as on OS X).

[su_v (suv-lp)]

The SVG file extracted from the xhtml file generated with the command line tool 'cdr2xhtml' from libcdr 0.1.1 crashes Inkscape trunk (r14341) on load too.

[su_v (suv-lp)]

The SVG file extracted from xhtml file generated with command line tool 'cdr2xhtml' of libcdr 0.0.15 on Ubuntu 14.04 also crashes Inkscape trunk on load.

[su_v (suv-lp)]

The SVG file extracted from xhtml file generated with command line tool 'cdr2xhtml' from libcdr 0.0.14 on OS X 10.7.5 opens in current trunk without crash (attached).

The problem seems to be differences in libcdr's SVG output (libcdr >= 0.0.15), and how latest trunk parses it (?).

[su_v (suv-lp)]

The trigger of Inkscape's crash (rev >= 14226) with the simpler test case seems to be an (erroneous (?)) repeated 'Z' path command in libcdr's output:

$ diff -u 442010-Frenchcurve-libcdr-0.0.1{4,5}.xhtml.svg
--- 442010-Frenchcurve-libcdr-0.0.14.xhtml.svg 2015-09-04 18:47:36.000000000 +0200
+++ 442010-Frenchcurve-libcdr-0.0.15.xhtml.svg 2015-09-04 19:03:02.000000000 +0200
@@ -24,6 +24,7 @@
 C166.6063,239.0322 165.6869,217.7318 173.5338,206.2086
 C193.3502,191.1311 246.2469,179.8439 284.2312,174.0744
 Z
+Z
 M342.5271,231.3037
 C382.1765,223.2296 430.8154,209.9802 474.0071,218.5390
 C508.5851,225.3916 544.9724,242.0104 567.4143,268.1390
$

After removing one of the two 'Z' path commands, the file opens ok in latest Inkscape trunk.

Note that Firefox, Chromium, Squiggle (Batik 1.7) as well as rsvg-view-3 (librsvg 2.40.10) render both versions of the SVG file as expected.

[Alvin Penner (apenner)]

confirmed, the file 442010-Frenchcurve-libcdr-0.0.15.xhtml.svg crashes with the exit messages:

C:\InkscapeBZR>inkscape\inkscape
terminate called after throwing an instance of 'Geom::ContinuityError'
  what(): lib2geom exception: Non-contiguous path (src/2geom/path.cpp:987)

Emergency save activated!
Emergency save completed. Inkscape will close now.
If you can reproduce this crash, please file a bug at www.inkscape.org
with a detailed description of the steps leading to the crash, so we can fix it.

**
ERROR:src/sp-namedview.cpp:1057:SPNamedView* sp_document_namedview(SPDocument*,
const gchar*): assertion failed: (nv != NULL)

[su_v (suv-lp)]

The diff for the second test case (terra.cdr) is too big to be meaningfully used for further triage (41MB). It does seem to contain quite a large number of such repeated 'Z' commands:

$ grep '+Z' 1492153-terra.cdr-libcdr-0.0.14-0.0.15.diff | wc -l
    5657
$

[jazzynico (jazzynico)]

Just ran into the same issue when testing new devlibs with librevenge-0.0.3 and libcdr-0.1.2. Some CDR files import correctly, but most of what I've tested so far fails.

[jazzynico (jazzynico)]

Seems to be closely related to Bug #1474359 "Non-contiguous path exception in Geom::Path::do_append" (same bt).

[Patrick Storz (ede123)]

I'll join the club with devlibs64 using librevenge-0.0.4 and libcdr-0.1.2.

[Patrick Storz (ede123)]

Here's a suggested patch which fixes the issue for me (trunk build of Inkscape r14630 on Windows 7 with gcc 5.3 branch of devlibs64 [1] including librevenge-0.0.4 and libcdr-0.1.2)

It simply replaces consecutive "closepath" commands with a single "Z".
Obviously it's more of a workaround at this point but I think it's a workable solution for the time being.

In the long run we'd have to figure out if consecutive "closepath" commands are valid to start with (the SVG specification is not really helpful here [2]).
Depending on the answer we can then go on and
a) change lib2geom to handle this correctly and/or
b) notify the libcdr authors about the issue so they can change the behaviour

[1] https://code.launchpad.net/~inkscape.dev/inkscape-devlibs64/5.3
[2] https://www.w3.org/TR/SVG/paths.html#PathDataClosePathCommand

[su_v (suv-lp)]

On 2016-02-06 19:57 (+0100), Eduard Braun wrote:
> In the long run we'd have to figure out if consecutive "closepath"
> commands are valid to start with (…)

Independent of the result of such an investigation - IMvHO inkscape (and/or 2geom) should not crash when encountering invalid, redundant or inconsistent path commands (it could instead stop rendering the path at that point, or stop rendering the drawing content with this element).

[jazzynico (jazzynico)]

Inkscape still crashes on Windows XP (32-bit), Inkscape 14648 (after the 2geom update rev. 14639, see http://bazaar.launchpad.net/~inkscape.dev/inkscape/trunk/revision/14639). Same error message.

[Patrick Storz (ede123)]

I submitted the workaround in revision 14650 to fix the CDR import.

I'll file a separate report for 2geom to investigate the underlying problem further.

[Patrick Storz (ede123)]

See https://bugs.launchpad.net/lib2geom/+bug/1545489 for the follow-up.

[su_v (suv-lp)]

On 2016-02-14 18:56 (+0100), Eduard Braun wrote:
> I submitted the workaround in revision 14650 to fix the CDR import.

JFYI - on OS X 10.7.5, I get compiler warnings (form GCC 4.6.3 as well
as clang (based on LLVM 3.2svn)) about «unknown escape sequence '\s'».

--
<opinion>
Personally (as original reporter) I disagree with "fixes" which only
hide or disguise the real underlying issue [1], and don't help with
other possible occurrences of similar SVG content in third-party files
(for example files which have been converted to SVG externally (see
earlier comments 6-9 with sample SVG test cases) still trigger a crash
on open or import, with or without your "fix"). You can also easily
trigger the same kind of crash from within Inkscape trunk by editing a
path 'd' attribute in the XML Editor, (mis-)typing two consecutive 'Z'
path commands and hit the 'Set' button.

To me such kind of fixes might make sense if there's not real solution
available yet, or only e.g. within a recent major 2geom update in
unstable trunk, yet a new stable release is pending, and the "fix" would
be limited (as known and commented workaround) to the stable release
branch only.
</opinion>

[1] in your comment in the 2geom bug tracker (didn't lib2geom move to
github, btw?) you even explicitly lower the importance of the issue
(«not a high priority»).

[Patrick Storz (ede123)]

> «unknown escape sequence '\s'»

Thanks, I didn't escape the character class properly (explains why '\s' on it's own didn't work before - it simply got stripped by the compiler for not being a known escape sequence)

> You can also easily trigger the same kind of crash from within Inkscape trunk by editing a path 'd' attribute in the XML Editor, (mis-)typing two consecutive 'Z' path commands and hit the 'Set' button.

Actually it seems you *can't* (at least not on Windows. For example I tried typing 'Z Z' in the XML editor as well as manipulating an SVG file... In both cases the 'd' attribute get's rewritten in a way that avoids the bug. As of now I can't think of any other way to reproduce this issue during normal Inkscape operation besides the CDR import which is the subject of this bug.

As this is therefore the only manifestation of the issue I'm aware of I figured a quick workaround to get CDR import working again in trunk builds might be a workable solution (as explained before) and since this eliminates the only real-world manifestation of this issue I'm aware of I honestly don't think that it needs a high priority in 2geom right now. If you feel differently just comment on that bug.

Obviously I don't suggest to forget about this completely and hope it get's fixed in 2geom eventually. Then we can also revert the workaround (although it shouldn't hurt either way).

[su_v (suv-lp)]

On 2016-02-14 21:48 (+0100), Eduard Braun wrote:
> Actually it seems you *can't* (at least not on Windows. For example I
> tried typing 'Z Z' in the XML editor as well as manipulating an SVG
> file... In both cases the 'd' attribute get's rewritten in a way that
> avoids the bug. As of now I can't think of any other way to reproduce
> this issue during normal Inkscape operation besides the CDR import which
> is the subject of this bug.

Additional hints to reproduce the crash: draw a new compound path (with
two sub-paths), and make sure that the second sub-path path (verify
order in node tool by stepping through nodes with <TAB>) has more than
two nodes (it does not appear to make a difference whether the nodes
have extracted node handles or not). Insert the consecutive 'Z' (or 'z')
commands in the XML editor before the 'M' (or 'm') command defining the
second sub-path. Still no crash on Windows?

[Patrick Storz (ede123)]

> Still no crash on Windows?
No, that finally "does the trick":
    (inkscape.exe:3844): glibmm-ERROR **:
    unhandled exception (type std::exception) in signal handler:
    what: lib2geom exception: Non-contiguous path (src/2geom/path.cpp:1043)

How'd you figure that one out? Took me some effort to reproduce correctly even with your instructions. ;-)

I assume we should carry on at
https://bugs.launchpad.net/lib2geom/+bug/1545489 ?

[su_v (suv-lp)]

JFTR (last comment here as original reporter): my opinion has not
changed (see comment 17, footnote in comment 21); I am disappointed by
the committed “fix” (a regex-based band-aid for the initially
encountered and reported _symptom_) and the report being closed (despite
having provided SVG files as part of the initial triage which trigger
the same underlying issue on load or import).

Anyway, it's yours now - good luck with getting the regression (of yet
unknown extent) in 2geom fully investigated, addressed upstream in
lib2geom, backported to and sufficiently tested in Inkscape in time for
the next major release (or not). I personally won't participate in
further discussion (here or elsewhere).

[jazzynico (jazzynico)]

@Eduard - su_v is right, it is now more difficult to investigate the bug. While it can be a good idea to patch a branch just before a release, I'd keep the trunk with the original bug (but it's ok to keep the patch as a file in the report).
Would you agree to revert your patch for now?

[Patrick Storz (ede123)]

Sorry, but in my opinion reverting the temporary fix for the sake of reopening this bug would be the wrong decision, for a whole lot of reasons:
- The underlying issue was determined to be in lib2geom, therefore has nothing to do with CDR input per se. This bug describes only one symptom (among many), not the actual cause, as acknowledged by su_v.
- Therefore I put an upstream bug in in place at [1] that outlines the issue in it's general context. Feel free to add any missing information. If anybody feels the visibility is too low we can additionally add Inkscape as project for the bug.
- In the upstream bug there is a much simpler way to reproduce the crash that does not involve CDR input and therefore makes it actually *easier* to investigate the bug.
- My temporary fix (that su_v is not getting tired of badmouthing - not a very nice way) patches CDR input, therefore providing a working CDR input in the trunk. If we revert it we break CDR input in trunk at the same time, making it impossible to discover other issues that are actually related to CDR input (let alone investigating or fixing them) and/or working on CDR import functionality*.

In conclusion I don't see how breaking CDR import on purpose would make it any more likely to get the issue in lib2geom fixed while it certainly would have several downsides...
However if any of the older project members still feels that reverting would be a better way to go feel free to do so**, in the end you obviously can choose to overrule me.

[1] https://bugs.launchpad.net/lib2geom/+bug/1545489

* As an example I would have never been able to update/test librevenge based import filters in the devlibs64 without patching this issue in some way. How would I've been supposed to check if CDR import was working if it was broken by another issue completely unrelated to the actual import functionality?

** However please be aware that that the general course of such an action would also have some unfavorable implications for the Inkscape community in general. This is probably not the place to discuss it, but feel free to contact me per mail about it.

[Liam P. White (liampwhite)]

<off-topic>
Since I wasn't a part of the earlier discussion, and I realize that my opinion is likely to be seen as aggressive and unpopular, I'll be brief:

- Using a regex-based hack to preprocess a non-regular language is pretty much universally a bad idea; you need to fix it in the parser code, which leads me to...
- Quite a few Inkscape developers also have access to the 2geom code, and any change to our internal copy of 2geom would also be applied to the external repositories within at most a few days.

As for the "badmouthing" comment, it seems as though you either don't push hard enough and nothing gets done, or push too hard and knock someone over. Regardless of being "nice," this is about getting the underlying cause fix — something I have myself neglected plenty of times before.
</off-topic>

[Liam P. White (liampwhite)]

Really fixed in rev 14686.

[Patrick Storz (ede123)]

Verified working (Inkscape 0.91+devel_64bit r14687 on Windows 7 x64).

Thanks for your constructive work on this Liam! (not so much for the far less constructive taunting comments here and elsewhere...)