[GRASS-dev] Re: problem with G_fatal_error

Hi developers,

here a request from the QGIS team... please group reply since
Martin isn't subscribed.

thanks
Markus

On Mon, Oct 22, 2007 at 11:04:30AM +0200, Martin Dobias wrote:

Hi Markus,

I'm currently working with Leonardo Lami on a crash in QGIS when user
tries to open corrupted vector layer. We've found out that it's a
regression from grass 6.2 and the problem happens only in 6.3.cvs.

The problem looks like this: QGIS calls Vect_open_old_head function -
that one finds out that the layer is corrupted and calls G_fatal_error
routine. Looking at that function I see that in 6.3.cvs the function
always calls exit() which is incorrect, because this takes also the
whole QGIS down. GRASS 6.2 has a condition "if ( ext_error ) return
0;" before the exit statement, so since QGIS sets extended error
handler, everything is okay. Please could you add that condition back?

Regards
Martin

Markus Neteler wrote:

here a request from the QGIS team... please group reply since
Martin isn't subscribed.

thanks
Markus

On Mon, Oct 22, 2007 at 11:04:30AM +0200, Martin Dobias wrote:
> Hi Markus,
>
> I'm currently working with Leonardo Lami on a crash in QGIS when user
> tries to open corrupted vector layer. We've found out that it's a
> regression from grass 6.2 and the problem happens only in 6.3.cvs.
>
> The problem looks like this: QGIS calls Vect_open_old_head function -
> that one finds out that the layer is corrupted and calls G_fatal_error
> routine. Looking at that function I see that in 6.3.cvs the function
> always calls exit() which is incorrect, because this takes also the
> whole QGIS down. GRASS 6.2 has a condition "if ( ext_error ) return
> 0;" before the exit statement, so since QGIS sets extended error
> handler, everything is okay. Please could you add that condition back?

No.

Code which calls G_fatal_error() is entitled to assume (and normally
does assume) that G_fatal_error() will not return.

If you don't want G_fatal_error() to terminate the process, you need
to ensure that your fatal error handler doesn't return, but e.g.
longjmp()s back to your code.

You will need to bear in mind that any variables used by the GRASS
libraries will be in an undefined state after this point, so the
effect of calling any GRASS function is undefined.

--
Glynn Clements <glynn@gclements.plus.com>

On 10/22/07, Glynn Clements <glynn@gclements.plus.com> wrote:

> > The problem looks like this: QGIS calls Vect_open_old_head function -
> > that one finds out that the layer is corrupted and calls G_fatal_error
> > routine. Looking at that function I see that in 6.3.cvs the function
> > always calls exit() which is incorrect, because this takes also the
> > whole QGIS down. GRASS 6.2 has a condition "if ( ext_error ) return
> > 0;" before the exit statement, so since QGIS sets extended error
> > handler, everything is okay. Please could you add that condition back?

No.

Code which calls G_fatal_error() is entitled to assume (and normally
does assume) that G_fatal_error() will not return.

If you don't want G_fatal_error() to terminate the process, you need
to ensure that your fatal error handler doesn't return, but e.g.
longjmp()s back to your code.

Hi Glynn,

don't you think it's very inconvenient for users of GRASS libraries
that stable API changes its semantics? And isn't that weird that a
library function calls exit() to close the process it is using it?

Btw. thinking about such incompatible changes, is there any resource
on internet or low-traffic mailing list which would announce any such
changes? Because this is not the first time that something got
changed... :-/

I'll take a look at longjmp()

Martin

Martin Dobias wrote:

> > > The problem looks like this: QGIS calls Vect_open_old_head function -
> > > that one finds out that the layer is corrupted and calls G_fatal_error
> > > routine. Looking at that function I see that in 6.3.cvs the function
> > > always calls exit() which is incorrect, because this takes also the
> > > whole QGIS down. GRASS 6.2 has a condition "if ( ext_error ) return
> > > 0;" before the exit statement, so since QGIS sets extended error
> > > handler, everything is okay. Please could you add that condition back?
>
> No.
>
> Code which calls G_fatal_error() is entitled to assume (and normally
> does assume) that G_fatal_error() will not return.
>
> If you don't want G_fatal_error() to terminate the process, you need
> to ensure that your fatal error handler doesn't return, but e.g.
> longjmp()s back to your code.

Hi Glynn,

don't you think it's very inconvenient for users of GRASS libraries
that stable API changes its semantics?

Its intended semantics haven't changed; it was a bug which has been
fixed.

It was only noticed when __attribute__((noreturn)) was added to the
function and gcc pointed out that it might actually return.

Note that the GRASS 6 Programmer's Manual has said the following since
at least 2004/02/20 (the point that it was moved into CVS):

  <P>
  int G_set_error_routine(int (*handler)()) change error handling
  
  ...
  
  <P> <B>Note.</B> The handler only provides a way to send the message
  somewhere other than to the error output. If the error is fatal, the
  module will exit after the handler returns.

And isn't that weird that a
library function calls exit() to close the process it is using it?

Not when the normal use of the function is to terminate the process.

It needs to be borne in mind that the GRASS libraries were (and are)
designed for GRASS, which is a collection of "one-shot" programs (i.e.
you run the program with arguments, it does its work, then exits). The
design of the libraries is fundamentally unsuitable for persistent
programs.

If you feel like re-writing the whole of GRASS to propagate all errors
up to the individual programs and handle them there (and cleaning up
any inconsistent state along the way), I'm sure that the effort will
be appreciated.

Btw. thinking about such incompatible changes, is there any resource
on internet or low-traffic mailing list which would announce any such
changes? Because this is not the first time that something got
changed... :-/

The only resource which documents every bug-fix is the grass-commit
list, and that certainly isn't low-traffic. Even if we had an
-announce list, there's no reason to suspect that this particular
change would have been announced there; it isn't as if anyone would
have known that you were relying upon this particular detail.

I'll take a look at longjmp()

Don't forget this part from my previous message:

You will need to bear in mind that any variables used by the GRASS
libraries will be in an undefined state after this point, so the
effect of calling any GRASS function is undefined.

longjmp() will "save" QGIS, but once something calls G_fatal_error(),
the GRASS libraries should be considered toast. Further calls to GRASS
functions may well result in a segfault, silent corruption of data,
etc.

--
Glynn Clements <glynn@gclements.plus.com>

Glynn Clements wrote:

Martin Dobias wrote:

The problem looks like this: QGIS calls Vect_open_old_head function -
that one finds out that the layer is corrupted and calls G_fatal_error
routine. Looking at that function I see that in 6.3.cvs the function
always calls exit() which is incorrect, because this takes also the
whole QGIS down. GRASS 6.2 has a condition "if ( ext_error ) return
0;" before the exit statement, so since QGIS sets extended error
handler, everything is okay. Please could you add that condition back?

No.

Maybe this one particular case could be patched up by making a copy of
GRASS' Vect_open_old_head function (like Vect_open_old_head_no_exit) and
using that to interface from QGIS?
I would expect that if there are more problems of that type, they should
all appear in calls of "open_something" or "close_someting" and maybe
some basic data retrieval functions, as the
QGIS interface probably doesn't need much more than these in order to
interface with GRASS and display GRASS maps?

Would it be possible to make a list of all GRASS lib functions that
QGIS calls directly, then look through them and make modified copies
of everyone that needs a different error handler (maybe putting them
altogether into a "GRASS_gui_handler" lib or sth. like that)?

It needs to be borne in mind that the GRASS libraries were (and are)
designed for GRASS, which is a collection of "one-shot" programs (i.e.
you run the program with arguments, it does its work, then exits). The
design of the libraries is fundamentally unsuitable for persistent
programs.

If you feel like re-writing the whole of GRASS to propagate all errors
up to the individual programs and handle them there (and cleaning up
any inconsistent state along the way), I'm sure that the effort will
be appreciated.

GRASS has earned our appreciation as a GUI-independent GIS toolbox, but
the fact remains that a GUI-integrated approach such as QGIS will open
it up to a multitude of new users and we need to support this in my
opinion.
Of course, GRASS' stability throughout the years has much relied on
the clean, Unix-based design that especially Glynn has been keeping
in a consistent state, but I am sure we can find bridges here.

Btw. thinking about such incompatible changes, is there any resource
on internet or low-traffic mailing list which would announce any such
changes? Because this is not the first time that something got
changed... :-/

The only resource which documents every bug-fix is the grass-commit
list, and that certainly isn't low-traffic. Even if we had an
-announce list, there's no reason to suspect that this particular
change would have been announced there; it isn't as if anyone would
have known that you were relying upon this particular detail.

Probably all true from a technical point of view but I sense that
there is some frustration building up among QGIS developers about
constantly having to catch-up with the results of the very
"different" nature and philosophy of GRASS development. We need
a stronger cooperation between both development teams to concentrate
on a Win32 GRASS "kernel" that can go into regular QGIS releases.

Benjamin

--
Benjamin Ducke, M.A.
Archäoinformatik
(Archaeoinformation Science)
Institut für Ur- und Frühgeschichte
(Inst. of Prehistoric and Historic Archaeology)
Christian-Albrechts-Universität zu Kiel
Johanna-Mestorf-Straße 2-6
D 24098 Kiel
Germany

Tel.: ++49 (0)431 880-3378 / -3379
Fax : ++49 (0)431 880-7300
www.uni-kiel.de/ufg

On 10/23/07 6:25 AM, "Benjamin Ducke" <benjamin.ducke@ufg.uni-kiel.de>
wrote:

GRASS has earned our appreciation as a GUI-independent GIS toolbox, but
the fact remains that a GUI-integrated approach such as QGIS will open
it up to a multitude of new users and we need to support this in my
opinion.
Of course, GRASS' stability throughout the years has much relied on
the clean, Unix-based design that especially Glynn has been keeping
in a consistent state, but I am sure we can find bridges here.

I couldn't agree more about the importance of a GUI to a large and growing
number of GRASS users. I can't help but point out that GRASS also has its
own GUI-integrated approach, that includes all available modules and is
actively under development (mature, stable TclTk and development wxPython).
Because it follows a different philosophy of integrating high-level GRASS
module commands rather than the lower-level GRASS libraries, we don't run
into this kind of problem as much.

Michael
__________________________________________
Michael Barton, Professor of Anthropology
Director of Graduate Studies
School of Human Evolution & Social Change
Center for Social Dynamics & Complexity
Arizona State University

phone: 480-965-6213
fax: 480-965-7671
www: http://www.public.asu.edu/~cmbarton

Michael Barton ha scritto:

I couldn't agree more about the importance of a GUI to a large and growing
number of GRASS users. I can't help but point out that GRASS also has its
own GUI-integrated approach, that includes all available modules and is
actively under development (mature, stable TclTk and development wxPython).
Because it follows a different philosophy of integrating high-level GRASS
module commands rather than the lower-level GRASS libraries, we don't run
into this kind of problem as much.

On the other side, we must admit for the average GIS user the native
GRASS GUIs are less smooth to use, and more surprising, when compared to
QGIS.
All the best.
pc

--
Paolo Cavallini, see: http://www.faunalia.it/pc

On 10/23/07 8:48 AM, "Paolo Cavallini" <cavallini@faunalia.it> wrote:

Michael Barton ha scritto:

I couldn't agree more about the importance of a GUI to a large and growing
number of GRASS users. I can't help but point out that GRASS also has its
own GUI-integrated approach, that includes all available modules and is
actively under development (mature, stable TclTk and development wxPython).
Because it follows a different philosophy of integrating high-level GRASS
module commands rather than the lower-level GRASS libraries, we don't run
into this kind of problem as much.

On the other side, we must admit for the average GIS user the native
GRASS GUIs are less smooth to use, and more surprising, when compared to
QGIS.

Having used both, I don't agree, though this is always a matter of opinion.

There are some features of the QGIS GUI that I very much like and others
that I find counter-intuitive. I'm sure that the same could be said of the
full GRASS GUI. In truth, no interface is really intuitive. The most
important question is how difficult is it to learn an interface given what
most people are used to.

Complicating any comparison is the fact the the native GRASS GUI includes
all GRASS modules, while QGIS is a hybrid of some specific QGIS features and
some GRASS features, making comparisons an apples vs. oranges sort of thing.
Also, while TclTk may *look* a little less 'modern' than Qt, it remains
highly functional as a GUI interface with a lot of bells and whistles. Of
course the wxPython GUI that is nearing completion *looks* more up-to-date
and seems functionally equivalent to Qt.

Michael

__________________________________________
Michael Barton, Professor of Anthropology
Director of Graduate Studies
School of Human Evolution & Social Change
Center for Social Dynamics & Complexity
Arizona State University

phone: 480-965-6213
fax: 480-965-7671
www: http://www.public.asu.edu/~cmbarton

Hi Benjamin,

On 10/23/07, Benjamin Ducke <benjamin.ducke@ufg.uni-kiel.de> wrote:

Maybe this one particular case could be patched up by making a copy of
GRASS' Vect_open_old_head function (like Vect_open_old_head_no_exit) and
using that to interface from QGIS?
I would expect that if there are more problems of that type, they should
all appear in calls of "open_something" or "close_someting" and maybe
some basic data retrieval functions, as the
QGIS interface probably doesn't need much more than these in order to
interface with GRASS and display GRASS maps?

I think copying the implementation could make it even worse as we
would need to watch any changes in GRASS implementation of such
functions. Or did you mean to copy it inside GRASS?

I've fixed the problem using setjmp/longjmp functions as Glynn suggested.

Would it be possible to make a list of all GRASS lib functions that
QGIS calls directly, then look through them and make modified copies
of everyone that needs a different error handler (maybe putting them
altogether into a "GRASS_gui_handler" lib or sth. like that)?

Even better would be to make a list of GRASS functions which could end
with fatal error. Now it's fixed only for that particular know case
where it was terminating QGIS, but I guess there might be more such
functions...

> It needs to be borne in mind that the GRASS libraries were (and are)
> designed for GRASS, which is a collection of "one-shot" programs (i.e.
> you run the program with arguments, it does its work, then exits). The
> design of the libraries is fundamentally unsuitable for persistent
> programs.
>
> If you feel like re-writing the whole of GRASS to propagate all errors
> up to the individual programs and handle them there (and cleaning up
> any inconsistent state along the way), I'm sure that the effort will
> be appreciated.

GRASS has earned our appreciation as a GUI-independent GIS toolbox, but
the fact remains that a GUI-integrated approach such as QGIS will open
it up to a multitude of new users and we need to support this in my
opinion.
Of course, GRASS' stability throughout the years has much relied on
the clean, Unix-based design that especially Glynn has been keeping
in a consistent state, but I am sure we can find bridges here.

The problem is that GRASS support for QGIS was in fact Radim's one-man
project. Now he's not active in development, so GRASS plugin doesn't
get enough attention from developers. We would really welcome anyone
interested in taking care of the GRASS plugin in QGIS.

Probably all true from a technical point of view but I sense that
there is some frustration building up among QGIS developers about
constantly having to catch-up with the results of the very
"different" nature and philosophy of GRASS development. We need
a stronger cooperation between both development teams to concentrate
on a Win32 GRASS "kernel" that can go into regular QGIS releases.

As written above, we need someone in QGIS team who understands GRASS
internals so that we could avoid more frustration from devs and
users...

Regards
Martin

Martin Dobias wrote:

> Would it be possible to make a list of all GRASS lib functions that
> QGIS calls directly, then look through them and make modified copies
> of everyone that needs a different error handler (maybe putting them
> altogether into a "GRASS_gui_handler" lib or sth. like that)?

Even better would be to make a list of GRASS functions which could end
with fatal error. Now it's fixed only for that particular know case
where it was terminating QGIS, but I guess there might be more such
functions...

The list would be most of GRASS.

E.g. G_malloc() calls G_fatal_error() if malloc() fails (although you
probably can't handle that case anyhow). Anything which uses the
environment (database, location, mapset, etc) will fail if $GISRC
isn't set, can't be opened, doesn't contain correct data, etc.

It might (literally) be simpler to list functions which can't call
G_fatal_error().

--
Glynn Clements <glynn@gclements.plus.com>

On 10/24/07, Glynn Clements <glynn@gclements.plus.com> wrote:

>
> Even better would be to make a list of GRASS functions which could end
> with fatal error. Now it's fixed only for that particular know case
> where it was terminating QGIS, but I guess there might be more such
> functions...

The list would be most of GRASS.

E.g. G_malloc() calls G_fatal_error() if malloc() fails (although you
probably can't handle that case anyhow). Anything which uses the
environment (database, location, mapset, etc) will fail if $GISRC
isn't set, can't be opened, doesn't contain correct data, etc.

It might (literally) be simpler to list functions which can't call
G_fatal_error().

Ah, so in that case we should use setjmp() before (nearly) every call
to GRASS routine, which is not a way to go. Thus I think we'll stick
with using setjmp() explicitly only at places known to produce fatal
errors. Or are there some more possibilities?

Martin

Martin Dobias wrote:

> > Even better would be to make a list of GRASS functions which could end
> > with fatal error. Now it's fixed only for that particular know case
> > where it was terminating QGIS, but I guess there might be more such
> > functions...
>
> The list would be most of GRASS.
>
> E.g. G_malloc() calls G_fatal_error() if malloc() fails (although you
> probably can't handle that case anyhow). Anything which uses the
> environment (database, location, mapset, etc) will fail if $GISRC
> isn't set, can't be opened, doesn't contain correct data, etc.
>
> It might (literally) be simpler to list functions which can't call
> G_fatal_error().

Ah, so in that case we should use setjmp() before (nearly) every call
to GRASS routine, which is not a way to go. Thus I think we'll stick
with using setjmp() explicitly only at places known to produce fatal
errors. Or are there some more possibilities?

Well, you would normally call setjmp() at the beginning of a
user-level "command", and just abandon the entire command if anything
fails.

E.g. in a GUI, you might call setjmp() from the event loop, so a
corresponding longjmp() would abandon further processing of that event
and move on to the next.

--
Glynn Clements <glynn@gclements.plus.com>

Martin Dobias wrote:

I think copying the implementation could make it even worse as we
would need to watch any changes in GRASS implementation of such
functions. Or did you mean to copy it inside GRASS?

Yes, that's what I was actually thinking about: an additional library
to be bundled with GRASS that provides "non-fatal" versions of the
functions a GUI most likely needs to call and handle.
Let's call it "GUI handler lib".

BUT, instead of duplicating code: make renamed copies of the functions
in question in the same C file that return an int instead of calling
G_fatal_error and replace the current G_open_* etc. with wrappers that
catch the return value, than exit as appropriate.
Then, put another wrapper into the "GUI handler lib" that provides a
non-exiting wrapper to be called by QGIS.

That way, QGIS developers will always be able to rely on the functions
inside "GUI handler lib" to not trigger fatal errors and we don't need
to keep two different files updated. We can keep adding functions to
the "GUI handler lib" as needed.

I've fixed the problem using setjmp/longjmp functions as Glynn suggested.

So does that give you any bad side-effects? It sounds rather like a
hack.

Which leads to option 2: create a global variable that controls whether
fatal errors should trigger an exit() or not and make all GRASS C API
functions that potentially call G_fatal_error() respect this.
However, I think this may create a lot of unpredictable behaviour (!?)

Maybe it would be enough to modify G_fatal_error() in that case?

Even better would be to make a list of GRASS functions which could end
with fatal error. Now it's fixed only for that particular know case
where it was terminating QGIS, but I guess there might be more such
functions...

That should be a simple grep job :wink:

The problem is that GRASS support for QGIS was in fact Radim's one-man
project. Now he's not active in development, so GRASS plugin doesn't
get enough attention from developers. We would really welcome anyone
interested in taking care of the GRASS plugin in QGIS.

Yes, I realize that. I am very interested in developing the QGIS GRASS
interface further. My current job does not leave me enough time to do
that. However, next spring I will start a new contract where I can
exclusively focus on OS GIS development more or less full-time.
I am very excited about this and hope I will be able to contribute
more to both GRASS and QGIS then.

For now, we'll have to do with the small resources we have, be patient
and above all work together. The prospect of having a perfectly stable
GRASS kernel running underneath a slick cross-platform desktop GUI is
just too important to lose sight of over design issues.

Best,

Benjamin

--
Benjamin Ducke, M.A.
Archäoinformatik
(Archaeoinformation Science)
Institut für Ur- und Frühgeschichte
(Inst. of Prehistoric and Historic Archaeology)
Christian-Albrechts-Universität zu Kiel
Johanna-Mestorf-Straße 2-6
D 24098 Kiel
Germany

Tel.: ++49 (0)431 880-3378 / -3379
Fax : ++49 (0)431 880-7300
www.uni-kiel.de/ufg

On 10/25/07, Benjamin Ducke <benjamin.ducke@ufg.uni-kiel.de> wrote:

Martin Dobias wrote:

>
> I think copying the implementation could make it even worse as we
> would need to watch any changes in GRASS implementation of such
> functions. Or did you mean to copy it inside GRASS?

Yes, that's what I was actually thinking about: an additional library
to be bundled with GRASS that provides "non-fatal" versions of the
functions a GUI most likely needs to call and handle.
Let's call it "GUI handler lib".

BUT, instead of duplicating code: make renamed copies of the functions
in question in the same C file that return an int instead of calling
G_fatal_error and replace the current G_open_* etc. with wrappers that
catch the return value, than exit as appropriate.
Then, put another wrapper into the "GUI handler lib" that provides a
non-exiting wrapper to be called by QGIS.

That way, QGIS developers will always be able to rely on the functions
inside "GUI handler lib" to not trigger fatal errors and we don't need
to keep two different files updated. We can keep adding functions to
the "GUI handler lib" as needed.

That sounds reasonably, I guess that also other 3rd party tools which
use GRASS would appreciate this.

> I've fixed the problem using setjmp/longjmp functions as Glynn suggested.

So does that give you any bad side-effects? It sounds rather like a
hack.

I think if one takes care, there are no bad side effects. However
using setjmp and longjmp generally makes the code harder to understand
as it changes usual execution. In C++ you must be even more careful
about the jumps because longjmp skips destructors of C++ classes
created on stack, so this may lead to some bad behaviour.

Which leads to option 2: create a global variable that controls whether
fatal errors should trigger an exit() or not and make all GRASS C API
functions that potentially call G_fatal_error() respect this.
However, I think this may create a lot of unpredictable behaviour (!?)

Maybe it would be enough to modify G_fatal_error() in that case?

No idea about this. However until now (versions < 6.3) when there was
custom error routine, G_fatal_error didn't call exit() and everything
was fine (at least it seemed so).

> The problem is that GRASS support for QGIS was in fact Radim's one-man
> project. Now he's not active in development, so GRASS plugin doesn't
> get enough attention from developers. We would really welcome anyone
> interested in taking care of the GRASS plugin in QGIS.
>

Yes, I realize that. I am very interested in developing the QGIS GRASS
interface further. My current job does not leave me enough time to do
that. However, next spring I will start a new contract where I can
exclusively focus on OS GIS development more or less full-time.
I am very excited about this and hope I will be able to contribute
more to both GRASS and QGIS then.

That would be great!

For now, we'll have to do with the small resources we have, be patient
and above all work together. The prospect of having a perfectly stable
GRASS kernel running underneath a slick cross-platform desktop GUI is
just too important to lose sight of over design issues.

True, true :slight_smile:

Martin

Benjamin Ducke wrote:

Which leads to option 2: create a global variable that controls whether
fatal errors should trigger an exit() or not and make all GRASS C API
functions that potentially call G_fatal_error() respect this.
However, I think this may create a lot of unpredictable behaviour (!?)

Maybe it would be enough to modify G_fatal_error() in that case?

Have a look at G_set_error_routine() in lib/gis/error.c

but G_fatal_error() still ends with exit(EXIT_FAILURE).

Hamish

__________________________________________________
Do You Yahoo!?
Tired of spam? Yahoo! Mail has the best spam protection around
http://mail.yahoo.com