v.to.db crashes on a shapefile connected with v.external

[msieczka]

Backtrace:

GRASS 6.5.svn (javier):~ > gdb v.to.db
GNU gdb 6.8-debian
Copyright (C) 2008 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
and "show warranty" for details.
This GDB was configured as https://trac.osgeo.org/grass/ticket/461#no3"x86_64-linux-gnu"...
(gdb) run map=gshhs_shp opt=area units=k col=AREA
Starting program: /usr/local/grass-6.5.svn/bin/v.to.db map=gshhs_shp opt=area units=k col=AREA
[Thread debugging using libthread_db enabled]
warning: Lowest section in /usr/lib/libicudata.so.38 is .hash at 0000000000000120
[New Thread 0x7fe8946c9710 (LWP 9814)]
Reading areas...
 100%

Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0x7fe8946c9710 (LWP 9814)]
0x00007fe89418f539 in db_get_column_sqltype (column=0x0) at column.c:107
107	    return column->sqlDataType;
(gdb) bt
#0  0x00007fe89418f539 in db_get_column_sqltype (column=0x0) at column.c:107
#1  0x00007fe8938f480d in db_select_int (driver=0x9237ba0, tab=0x98bb7f0 "gshhs", col=0x98b8ba0 "", where=0x0, pval=0x7fff9c7ff858) at select.c:138
#2  0x00000000004069f2 in update (Map=0x7fff9c7ff8c0) at update.c:53
#3  0x00000000004043e4 in main (argc=5, argv=0x7fff9c7fffd8) at main.c:84

[mlennert]

The title of this bug concerns v.db.update, but the backtrace is on v.to.db.

Maciej, could you detail this a bit more ?

Moritz

[msieczka]

Replying to mlennert:

The title of this bug concerns v.db.update, but the backtrace is on v.to.db.

My bad. Correcting the title.

Maciej, could you detail this a bit more ?

1. run v.external on some shapefile

2. try to populate the shapefile's datatable with v.to.db - crash

[neteler]

The manual ​http://grass.osgeo.org/grass64/manuals/html64_user/v.external.html says: Creates a new vector as a read-only link to OGR layer. Apparently some error trapping is missing which tells the user that v.external is a read-only link.

[neteler]

Do we have a function in order to test if an attribute table is open for writing?

[hamish]

[copied from the ML]

-> still valid?

Markus Metz wrote:

I think so because attribute queries are not possible on OGR vectors connected with v.external (most OGR vector layers don't have a grass-like key column). I think Martin L fixed that in grass7.

...

Attribute table operations (query, upload values) are still not possible in grass7 for OGR vector layers that don't have a key column, i.e. for most OGR-supported formats. Still valid for both v.external and direct OGR link.

We may not be able to add the requested functionality for the next release, but can we do anything about replacing the segfault with a G_fatal_error()?

Hamish

[glynn]

Replying to hamish:

We may not be able to add the requested functionality for the next release, but can we do anything about replacing the segfault with a G_fatal_error()?

The following is based upon the backtrace, and is completely untested:

--- lib/db/dbmi_client/select.c	(revision 40258)
+++ lib/db/dbmi_client/select.c	(working copy)
@@ -134,6 +134,8 @@
 
     table = db_get_cursor_table(&cursor);
     column = db_get_table_column(table, 0);	/* first column */
+    if (!column)
+	G_fatal_error(_("Table has no columns"));
     value = db_get_column_value(column);
     type = db_get_column_sqltype(column);
     type = db_sqltype_to_Ctype(type);

[neteler]

The suggested patch looks good:

# NC dataset
v.external medfacs.shp out=medfacs layer=medfacs

# unpatched:
GRASS 6.4.0svn (nc_spm_08):~/books/kluwerbook/data3rd/wake > v.to.db medfacs option=area columns=AREA
Reading areas...
Segmentation fault

# patched:
GRASS 6.4.0svn (nc_spm_08):~/books/kluwerbook/data3rd/wake > v.to.db medfacs option=area columns=AREA
Reading areas...
ERROR: Table has no columns

It may not be immediate to derive the problem in this particular case but a healthy test. Please submit and I'll backport.

[mmetz]

Replying to glynn:

Replying to hamish:

We may not be able to add the requested functionality for the next release, but can we do anything about replacing the segfault with a G_fatal_error()?

The following is based upon the backtrace, and is completely untested:

 --- lib/db/dbmi_client/select.c	(revision 40258)
 +++ lib/db/dbmi_client/select.c	(working copy)
 @@ -134,6 +134,8 @@
  
      table = db_get_cursor_table(&cursor);
      column = db_get_table_column(table, 0);	/* first column */
 +    if (!column)
 +	G_fatal_error(_("Table has no columns"));
      value = db_get_column_value(column);
      type = db_get_column_sqltype(column);
     type = db_sqltype_to_Ctype(type);

Going a bit further to the root of the problem: in this case const char *col passed to db_select_int() in lib/db/dbmi_client/select.c is a zero length string, you could do for 'col' the same test that's done for 'where' and exit with an error.

Even further down, the question is, what to do if OGR_L_GetFIDColumn() doesn't return FID name, to cite a comment from Vect_read_dblinks().

Markus M

[mmetz]

Replying to mmetz:

Going a bit further to the root of the problem: in this case const char *col passed to db_select_int() in lib/db/dbmi_client/select.c is a zero length string, you could do for 'col' the same test that's done for 'where' and exit with an error.

The following patch works for me

Index: select.c
===================================================================
--- select.c	(revision 40416)
+++ select.c	(working copy)
@@ -115,6 +115,9 @@
 
     G_debug(3, "db_select_int()");
 
+    if (col == NULL || strlen(col) == 0)
+	G_fatal_error(_("Missing column name"));
+
     /* allocate */
     alloc = 1000;
     val = (int *)G_malloc(alloc * sizeof(int));
@@ -203,6 +206,12 @@
     dbValue *value;
     dbTable *table;
 
+    if (key == NULL || strlen(key) == 0)
+	G_fatal_error(_("Missing key column name"));
+
+    if (col == NULL || strlen(col) == 0)
+	G_fatal_error(_("Missing column name"));
+
     G_zero(val, sizeof(dbValue));
     sprintf(buf, "SELECT %s FROM %s WHERE %s = %d\n", col, tab, key, id);
     db_init_string(&stmt);
@@ -259,6 +268,12 @@
 
     G_debug(3, "db_select_db_select_CatValArray ()");
 
+    if (key == NULL || strlen(key) == 0)
+	G_fatal_error(_("Missing key column name"));
+
+    if (col == NULL || strlen(col) == 0)
+	G_fatal_error(_("Missing column name"));
+
     db_init_string(&stmt);
 
     sprintf(buf, "SELECT %s, %s FROM %s", key, col, tab);

Problem is when a module gives a broken sql string to db_execute_immediate(), hopefully that aborts with an error and not a segfault.

Glynn's patch could be useful in other circumstances too.

Markus M

[mmetz]

Regarding the recent discussion about GRASS libraries and G_fatal_error() as well as the description of these functions, maybe rather issue a warning instead of an error and return -1? That would work because (at least) v.to.db checks the return value of db_select_int() and exits with an error if db_select_int() returns -1.

Markus M

[mmetz]

segfault fixed in r40568 (relbr6), r40569 (devbr6), r40570 (trunk).

v.to.db as all modules working with attribute tables still doesn't work with OGR-linked vectors, I guess that will only get fixed in grass7 if ever. I suggest to change milestone to 7.0 and priority to major.

Markus M

[mlennert]

What is the status of this bug ? Any chance v.external will ever offer write access to attribute tables of OGR-linked vectors ? With a recent grass7 I still get the "Missing column name" message. I actually do not find that message very clear. My first reaction was to check whether I had forgotten to indicate the column I wanted to upload to. Maybe some hint about linked maps would be helpful.

[martinl]

Originally reported bug seems to be fixed. Free free to reopen if needed.