Re: [Inkscape-devel] Crash in 2geom ellipse parsing
![](https://secure.gravatar.com/avatar/214fe9a0ecb4aed8994e8618ade6f5a8.jpg?s=120&d=mm&r=g)
In any case, bad data should be handled gracefully, not with a killer assertion.
The general rule is to be forgiving on input, strict on output.
bob
-----Original Message-----
From: Marco <mrcekets@...400...> Sent: Jun 26, 2008 11:12 AM To: Marco <mrcekets@...400...>, "Jon A. Cruz" <jon@...18...> Cc: "inkscape-devel@lists.sourceforge.net" inkscape-devel@lists.sourceforge.net, "lib2geom-devel@lists.sourceforge.net" lib2geom-devel@lists.sourceforge.net, njh <njh@...1927...> Subject: Re: [Inkscape-devel] Crash in 2geom ellipse parsing
On Thu, 26 Jun 2008 17:49:38 +0200, Marco <mrcekets@...400...> wrote:
On Thu, 26 Jun 2008 17:02:00 +0200, Jon A. Cruz <jon@...18...> wrote:
We've gotten bug #243232 that is due to an assertion in 2geom being triggered.
terminate called after throwing an instance of 'Geom::RangeError' what(): lib2geom exception: there is no ellipse that satisfies the given constraints (2geom/elliptical-arc.cpp:616)
https://bugs.launchpad.net/inkscape/+bug/243232
Can someone familiar with that area take a look?
Could it be a parsing error ? I tried to create EllipticalArc instances using the data presents in the path command that caused the assertion failure and all seem to work fine.
- Marco
No sorry I was wrong! There is really a problem with the passed data. The following line is the command that causes the assertion: "M 9.0625 10.21875 A 0.6384567 0.6384567 0 1 0 8.1875 11.15625"
I'm analizing the problem.
- Marco
-- Using Opera's revolutionary e-mail client: http://www.opera.com/mail/
Check out the new SourceForge.net Marketplace. It's the best place to buy or sell services for just about anything Open Source. http://sourceforge.net/services/buy/index.php _______________________________________________ Inkscape-devel mailing list Inkscape-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/inkscape-devel
![](https://secure.gravatar.com/avatar/01c2dd03530db1afb305df3aaad0fed8.jpg?s=120&d=mm&r=g)
Bob Jamison wrote:
In any case, bad data should be handled gracefully, not with a killer assertion.
The general rule is to be forgiving on input, strict on output.
I agree but I don't think the solution is to make our parser less strict. If we are parsing our own sloppy invalid svgd (as I believe is happening in this case) we should fail so that a bug will get filed and we can fix it. During input of an unknown user file we should catch the exception on a failed parse and alert the user that their file is corrupted or invalid and offer to attempt to repair it.
Aaron Spike
![](https://secure.gravatar.com/avatar/15f5e6abf26f57e1838c29a8356ce7f8.jpg?s=120&d=mm&r=g)
-----Original Message----- From: inkscape-devel-bounces@lists.sourceforge.net [mailto:inkscape-devel-bounces@lists.sourceforge.net] On Behalf Of Bob Jamison Sent: donderdag 26 juni 2008 19:07 To: Marco; Jon A. Cruz Cc: inkscape-devel@lists.sourceforge.net; lib2geom-devel@lists.sourceforge.net; njh Subject: Re: [Inkscape-devel] Crash in 2geom ellipse parsing
In any case, bad data should be handled gracefully, not with a killer assertion.
The general rule is to be forgiving on input, strict on output.
I agree. The reason I have it crash now is to find out where things go wrong. (which is why I have Inkscape output the to-be-parsed path string and then rethrow the exception)
Johan
![](https://secure.gravatar.com/avatar/eb3fe37da4a199eb4e3b479d8a57f808.jpg?s=120&d=mm&r=g)
On Thu, 2008-06-26 at 12:06 -0500, Bob Jamison wrote:
In any case, bad data should be handled gracefully, not with a killer assertion.
The general rule is to be forgiving on input, strict on output.
It is certainly true that assertions should not be used as tools to validate external input. Instead, the reason we have assertions is to uncover bugs, which is what occurred in this case. The crash was a manifestation of a bug in the elliptical arc curve type, such that it did not correctly handle data permitted by the SVG standard.
As has been demonstrated in this and many other cases, using hard assertions during development is the best way to guarantee that bugs are found and fixed quickly.
-mental
![](https://secure.gravatar.com/avatar/bb65b6b3a109d97cf9f8d6c014ede042.jpg?s=120&d=mm&r=g)
On Thu, Jun 26, 2008 at 10:49 PM, MenTaLguY <mental@...3...> wrote:
As has been demonstrated in this and many other cases, using hard assertions during development is the best way to guarantee that bugs are found and fixed quickly.
These particular crashes are found and fixed, yes. But how can you be sure that there are no other assert-crashes that are missed by our sketchy developer-testing, get released, and make Inkscape feel crashy and flakey to the users - who therefore may feel disinclined to even report them?
![](https://secure.gravatar.com/avatar/dc940f48c5635785f32941f1fbe6b601.jpg?s=120&d=mm&r=g)
On Jun 26, 2008, at 8:33 PM, bulia byak wrote:
On Thu, Jun 26, 2008 at 10:49 PM, MenTaLguY <mental@...3...> wrote:
As has been demonstrated in this and many other cases, using hard assertions during development is the best way to guarantee that bugs are found and fixed quickly.
These particular crashes are found and fixed, yes. But how can you be sure that there are no other assert-crashes that are missed by our sketchy developer-testing, get released, and make Inkscape feel crashy and flakey to the users - who therefore may feel disinclined to even report them?
This seems to be where we get into the subtle choices about assertions, warnings, exceptions, etc.
Generally an assertion should be a "this should never, ever, ever happen" thing in the code. For development and testing builds assertions can be "turned on", and then for release they can be "turned off"
Warnings should be for "I don't *think* this will happen, but if it does I really want to know about it. just in case"
Exceptions should be for "normally this bit of code will work, but when an unusual case hits we'll want to stop and recover"
and then return value error codes should probably be for "this call will not succeed in the normal course of things, so I need to check whether or not it did"
For Inkscape, I'm not familiar with the state of assertions and if they should be assert(), ASSERT(), g_assert(), etc. It's likely that we probably want to avoid bare assert() or g_assert() calls, and perhaps use some macro. Runaway memory, failure to malloc, etc. might fall into this case. (Most projects in C or C++ I've been on that have used asserts have wrapped them in turnon/turnoff macros)
If we have some code that gracefully handles some error condition and recovers from it, then that sounds like the time for a g_warning() call to be added at that point. Those allow operation to continue, but then developers can run with --g-fatal-warnings and get an exception thrown at the end of warning display.
For something low-level, throwing an exception might be the best there. So for our current issue the lib itself should optionally do a g_warning() (depending on precise code needs, etc) and then throw an exception. Up higher a calling routine (that happens to know more of the big picture, and more of what would be an appropriate recovery action) can catch any exception, log it, and then recover.
![](https://secure.gravatar.com/avatar/eb3fe37da4a199eb4e3b479d8a57f808.jpg?s=120&d=mm&r=g)
On Thu, 2008-06-26 at 23:33 -0400, bulia byak wrote:
These particular crashes are found and fixed, yes. But how can you be sure that there are no other assert-crashes that are missed by our sketchy developer-testing, get released, and make Inkscape feel crashy and flakey to the users - who therefore may feel disinclined to even report them?
Without assertions, invariant failures still tend to eventually result in crashes, *especially* in C/C++. Removing assertions would certainly help to obscure the cause of a crash, but crashes would not be entirely prevented, only rendered less predictable (and therefore less likely to be caught by our sketchy developer-testing).
Shooting the messenger does not really fix the problem.
-mental
![](https://secure.gravatar.com/avatar/dc940f48c5635785f32941f1fbe6b601.jpg?s=120&d=mm&r=g)
On Jun 26, 2008, at 11:24 PM, MenTaLguY wrote:
Without assertions, invariant failures still tend to eventually result in crashes, *especially* in C/C++. Removing assertions would certainly help to obscure the cause of a crash, but crashes would not be entirely prevented, only rendered less predictable (and therefore less likely to be caught by our sketchy developer-testing).
Shooting the messenger does not really fix the problem.
Yes. We need to allow the messengers in...
... then drop a two-ton anvil on whatever monster chased them out to begin with.
(Yes, I'm a graduate of the Chunk Jones school of coding)
If we start squishing the messengers, then they'll stop running to us, and the big daddy monsters will stop following them into our traps.
![](https://secure.gravatar.com/avatar/15f5e6abf26f57e1838c29a8356ce7f8.jpg?s=120&d=mm&r=g)
-----Original Message----- From: lib2geom-devel-bounces@lists.sourceforge.net [mailto:lib2geom-devel-bounces@lists.sourceforge.net] On Behalf Of MenTaLguY Sent: vrijdag 27 juni 2008 8:24 To: bulia byak Cc: Jon A. Cruz; lib2geom-devel@lists.sourceforge.net; inkscape-devel@lists.sourceforge.net Subject: Re: [Lib2geom-devel] [Inkscape-devel] Crash in 2geom ellipse parsing
On Thu, 2008-06-26 at 23:33 -0400, bulia byak wrote:
These particular crashes are found and fixed, yes. But how
can you be
sure that there are no other assert-crashes that are missed by our sketchy developer-testing, get released, and make Inkscape
feel crashy
and flakey to the users - who therefore may feel
disinclined to even
report them?
Without assertions, invariant failures still tend to eventually result in crashes, *especially* in C/C++. Removing assertions would certainly help to obscure the cause of a crash, but crashes would not be entirely prevented, only rendered less predictable (and therefore less likely to be caught by our sketchy developer-testing).
This is why I have proposed to replace assertions with exceptions in lib2geom (and already did a few). Then we don't have to remove/disable the assertions, and we end up with a system that can recover on error. For LPEs, exceptions instead of asserts have proven to be much easier to work with. Instead of getting a bug report: "when i wiggle this line somewhere special, it crashes, don't know when". I get: "for the attached SVG, I get an LPE error". (LPE 2geom exceptions are handled by simply aborting the calculation and showing the original path, so it is very easy to see when an error happens and because Inkscape doesn't crash, you can save the data and attach it to a bugreport)
Do we have a flag in Inkscape for release/debug that can be used to #ifdef some code? For example, the parser calling code now catches the 2geom exception, outputs path string to screen and then rethrows the exception. For release only, instead of rethrowing it might want to return an empty path instead of throwing an exception.
Johan
![](https://secure.gravatar.com/avatar/eb3fe37da4a199eb4e3b479d8a57f808.jpg?s=120&d=mm&r=g)
On Fri, 2008-06-27 at 12:41 +0200, J.B.C.Engelen@...1578... wrote:
This is why I have proposed to replace assertions with exceptions in lib2geom (and already did a few). Then we don't have to remove/disable the assertions, and we end up with a system that can recover on error.
My primary concern is whether or not assertion failures get hidden from the end user. Exceptions are adequate in principle for indicating invariant failures, but what usually happens is that people start writing things like:
try { // ... } catch (AnyLibraryException e) { return some_default_value; }
For "normal" exceptions, that may not always be an unreasonable approach, but exceptions resulting from the violation of an invariant should not be lumped in with "normal" exceptions.
At minimum, a failed invariant means that there is a significant bug which may result in undefined behavior, and not infrequently in C++, failure to observe an invariant is either the result or the cause of memory corruption.
For LPEs, exceptions instead of asserts have proven to be much easier to work with. Instead of getting a bug report: "when i wiggle this line somewhere special, it crashes, don't know when". I get: "for the attached SVG, I get an LPE error". (LPE 2geom exceptions are handled by simply aborting the calculation and showing the original path, so it is very easy to see when an error happens and because Inkscape doesn't crash, you can save the data and attach it to a bugreport)
The most important thing is that assertion failures are visible to the user. Crashing is a rather blunt instrument approach to doing this, so I am not opposed to more graceful ways in principle (particularly if they facilitate the collection of debugging information).
Do we have a flag in Inkscape for release/debug that can be used to #ifdef some code?
Let's please not start #ifdeffing things. The less difference there is between the release and debug codebases, the better.
For example, the parser calling code now catches the 2geom exception, outputs path string to screen and then rethrows the exception. For release only, instead of rethrowing it might want to return an empty path instead of throwing an exception.
This would be exactly the sort of sweeping under the carpet that I am concerned about. At minimum you should distinguish between simple parse errors (e.g. due to invalid input data), which *can* sometimes be reasonably treated relatively silently in this sort of way, from assertion failures, which should *always* be propagated up to the UI somehow.
If the invariant failure happens to be associated with memory corruption, you are not doing the user any favors if you hide the fact that Inkscape is now unstable and they are putting their work at risk by not saving and exiting right then.
-mental
![](https://secure.gravatar.com/avatar/82c0f6eed0ee59676eb45aadd66dac57.jpg?s=120&d=mm&r=g)
MenTaLguY wrote:
On Fri, 2008-06-27 at 12:41 +0200, J.B.C.Engelen@...1578... wrote:
For example, the parser calling code now catches the 2geom exception, outputs path string to screen and then rethrows the exception. For release only, instead of rethrowing it might want to return an empty path instead of throwing an exception.
This would be exactly the sort of sweeping under the carpet that I am concerned about. At minimum you should distinguish between simple parse errors (e.g. due to invalid input data), which *can* sometimes be reasonably treated relatively silently in this sort of way, from assertion failures, which should *always* be propagated up to the UI somehow.
BTW, if the parser encounters an error in the path data it should return the path upto and including the last parsed node (see the SVG spec).
![](https://secure.gravatar.com/avatar/bb65b6b3a109d97cf9f8d6c014ede042.jpg?s=120&d=mm&r=g)
On Fri, Jun 27, 2008 at 2:24 AM, MenTaLguY <mental@...3...> wrote:
Without assertions, invariant failures still tend to eventually result in crashes, *especially* in C/C++. Removing assertions would certainly help to obscure the cause of a crash, but crashes would not be entirely prevented, only rendered less predictable (and therefore less likely to be caught by our sketchy developer-testing).
With the assert-crashes, on the other hand, right now I am forced to go back to an old build in order to complete a design project, because these constant crashes are really driving me nuts. Now whose win is that?
You can print a megabyte of debug dump. You can beep. You can flash. You can draw a big green monster on my screen. Just please, PLEASE don't crash, OK? Is it too much to ask for?
![](https://secure.gravatar.com/avatar/15f5e6abf26f57e1838c29a8356ce7f8.jpg?s=120&d=mm&r=g)
-----Original Message----- From: lib2geom-devel-bounces@lists.sourceforge.net [mailto:lib2geom-devel-bounces@lists.sourceforge.net] On Behalf Of bulia byak Sent: vrijdag 27 juni 2008 18:26 To: MenTaLguY Cc: Jon A. Cruz; lib2geom-devel@lists.sourceforge.net; inkscape-devel@lists.sourceforge.net Subject: Re: [Lib2geom-devel] [Inkscape-devel] Crash in 2geom ellipse parsing
On Fri, Jun 27, 2008 at 2:24 AM, MenTaLguY <mental@...3...> wrote:
Without assertions, invariant failures still tend to
eventually result
in crashes, *especially* in C/C++. Removing assertions would certainly help to obscure the cause of a crash, but crashes
would not
be entirely prevented, only rendered less predictable (and
therefore
less likely to be caught by our sketchy developer-testing).
With the assert-crashes, on the other hand, right now I am forced to go back to an old build in order to complete a design project, because these constant crashes are really driving me nuts. Now whose win is that?
I am sorry for disturbing your project :-(
You can print a megabyte of debug dump. You can beep. You can flash. You can draw a big green monster on my screen. Just please, PLEASE don't crash, OK? Is it too much to ask for?
Let's try and group 2geoms exceptions like is done in exception.h: rangeerror and logicalerror. Range error = user mistake (Inkscape's code fault), logicalerror = 2geom's fault. There are some difficult cases, but at least it gives us some guideline. Then, when something goes wrong, we can catch only RangeErrors and handle it gracefully, and catch LogicalErrors with a popup dialog:
... happened ... bla bla...
< continue > < emergency save and exit >
Does that sound reasonable?
Cheers, Johan
![](https://secure.gravatar.com/avatar/eb3fe37da4a199eb4e3b479d8a57f808.jpg?s=120&d=mm&r=g)
On Fri, 2008-06-27 at 12:26 -0400, bulia byak wrote:
You can print a megabyte of debug dump. You can beep. You can flash. You can draw a big green monster on my screen. Just please, PLEASE don't crash, OK? Is it too much to ask for?
Johan and I talked about this at length last night; while there are some issues which need to be thought through and dealt with (e.g. not losing backtraces), popping up a dialog seems like a reasonable compromise for an invariant failure. However, as noted before I can't completely guarantee that Inkscape wouldn't still crash on its own (or worse, silently corrupt/lose data) in those cases. The ultimate problem is that there are real bugs that have to be fixed.
Something which may be of more immediate practical help to you is that we've identified some assertions which need not really be assertions, since some of the checks because of their context effectively become checks of input data rather than data structure integrity. With some work, those can be separated out and treated a little more "softly". If we're really lucky, those are predominantly the ones you are hitting right now.
-mental
![](https://secure.gravatar.com/avatar/bb65b6b3a109d97cf9f8d6c014ede042.jpg?s=120&d=mm&r=g)
On Fri, Jun 27, 2008 at 7:37 PM, MenTaLguY <mental@...3...> wrote:
Johan and I talked about this at length last night; while there are some issues which need to be thought through and dealt with (e.g. not losing backtraces), popping up a dialog seems like a reasonable compromise for an invariant failure.
Sounds like a perfect solution to me.
However, as noted before I can't completely guarantee that Inkscape wouldn't still crash on its own (or worse, silently corrupt/lose data) in those cases. The ultimate problem is that there are real bugs that have to be fixed.
Of course. I'm warned, let me decide for myself whether to quit and reload or not :)
participants (7)
-
unknown@example.com
-
Aaron Spike
-
Bob Jamison
-
bulia byak
-
Jasper van de Gronde
-
Jon A. Cruz
-
MenTaLguY