Opened 11 years ago

Closed 20 months ago

#461 closed defect (fixed)

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

Reported by: msieczka Owned by: grass-dev@…
Priority: major Milestone: 7.0.7
Component: Vector Version: svn-trunk
Keywords: v.external, v.to.db Cc: martinl
CPU: x86-64 Platform: Linux

Description (last modified by martinl)

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

Change History (18)

comment:1 Changed 11 years ago by 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

comment:2 in reply to:  1 Changed 11 years ago by msieczka

Summary: v.db.update crashes with updating shapefile connected with v.externalv.to.db crashes on a shapefile connected with v.external

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
  1. try to populate the shapefile's datatable with v.to.db - crash

comment:3 Changed 11 years ago by 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.

comment:4 Changed 10 years ago by neteler

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

comment:5 Changed 10 years ago by 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

comment:6 in reply to:  5 ; Changed 10 years ago by 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);

comment:7 Changed 10 years ago by 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.

comment:8 in reply to:  6 ; Changed 10 years ago by 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

comment:9 in reply to:  8 Changed 10 years ago by 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

comment:10 Changed 10 years ago by 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

comment:11 Changed 10 years ago by 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

comment:12 Changed 10 years ago by hamish

Keywords: v.external v.to.db added
Milestone: 6.4.07.0.0
Priority: criticalmajor
Version: svn-develbranch6svn-trunk

comment:13 Changed 7 years ago by 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.

comment:14 Changed 7 years ago by martinl

Cc: martinl added

comment:15 Changed 4 years ago by martinl

Milestone: 7.0.07.0.5

comment:16 Changed 4 years ago by neteler

Milestone: 7.0.57.0.6

comment:17 Changed 2 years ago by neteler

Milestone: 7.0.67.0.7

comment:18 Changed 20 months ago by martinl

Description: modified (diff)
Resolution: fixed
Status: newclosed

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

Note: See TracTickets for help on using tickets.