[PATCH] GML TopoSurface concatenation results in invalid geometries due to point duplication

[esseffe]

looks very similar to ticket #4451.

while parsing GML-Topology many TopoSurfaces re-aggregated from their corresponding Edges will be invalid due to point duplication.

this bug only occurs when specifying GML_FACE_HOLE_NEGATIVE YES and affects both modes GML_SKIP_RESOLVE_ELEMS NONE|HUGE indifferently.

you could eventually replicate this issue by processing the attached GML-Topo sample (you'll find duplicated points into the "suolo" table):

ogr2ogr -f "SQLite" dupli.sqlite topogml_sample.xml -dsco SPATIALITE=yes --config GML_SKIP_RESOLVE_ELEMS HUGE --config GML_FACE_HOLE_NEGATIVE YES the attached patch (closely derived from the one already adopted for ticket #4451) seems to effectively resolve this issue.

bye Sandro

[Even Rouault]

Hum, before applying the patch for the issue you're trying to fix, I encounter quite a few preliminary issues :

$ rm topogml_sample.resolved.gml
$ rm topogml_sample.sqlite
$ gdb --args ogrinfo topogml_sample.xml --config GML_SKIP_RESOLVE_ELEMS HUGE --config GML_FACE_HOLE_NEGATIVE YES -al -ro
[...]
ERROR 1: sqlite3_step() failed:
  SQL logic error or missing database
ERROR 1: Edge gml:id="Edge_32_34129": invalid Node-to
ERROR 1: Edge gml:id="Edge_32_34096": invalid Node-to
ERROR 1: Edge gml:id="Edge_32_34203": invalid Node-from
terminate called after throwing an instance of 'std::logic_error'
  what():  basic_string::_S_construct NULL not valid

Program received signal SIGABRT, Aborted.
0x00007fffec904b25 in *__GI_raise (sig=<value optimized out>) at ../nptl/sysdeps/unix/sysv/linux/raise.c:64
64	../nptl/sysdeps/unix/sysv/linux/raise.c: Aucun fichier ou dossier de ce type.
	in ../nptl/sysdeps/unix/sysv/linux/raise.c
(gdb) bt
#0  0x00007fffec904b25 in *__GI_raise (sig=<value optimized out>) at ../nptl/sysdeps/unix/sysv/linux/raise.c:64
#1  0x00007fffec908670 in *__GI_abort () at abort.c:92
#2  0x00007fffecf3a8c5 in __gnu_cxx::__verbose_terminate_handler() () from /usr/lib/libstdc++.so.6
#3  0x00007fffecf38cf6 in ?? () from /usr/lib/libstdc++.so.6
#4  0x00007fffecf38d23 in std::terminate() () from /usr/lib/libstdc++.so.6
#5  0x00007fffecf38e1e in __cxa_throw () from /usr/lib/libstdc++.so.6
#6  0x00007fffeced4987 in std::__throw_logic_error(char const*) () from /usr/lib/libstdc++.so.6
#7  0x00007fffecf16911 in ?? () from /usr/lib/libstdc++.so.6
#8  0x00007fffecf16a93 in std::basic_string<char, std::char_traits<char>, std::allocator<char> >::basic_string(char const*, std::allocator<char> const&) ()
   from /usr/lib/libstdc++.so.6
#9  0x00007ffff6bf9faa in CPLString (this=0x65e590, pszStr=0x0) at /home/even/gdal/svn/1.10/gdal/port/cpl_string.h:231
#10 0x00007ffff720b8a5 in gmlHugeSetHrefGmlText (helper=0x7fffffffc0a0, pszGmlId=0x639ce8 "Edge_32_34096", pszGmlText=0x0) at hugefileresolver.cpp:1391
#11 0x00007ffff720bce0 in gmlHugeResolveEdges (helper=0x7fffffffc0a0, psNode=0x6642f0, hDB=0x62e278) at hugefileresolver.cpp:1508
#12 0x00007ffff720c7a1 in gmlHugeFileWriteResolved (helper=0x7fffffffc0a0, pszOutputFilename=0x61ef30 "../../trunk/gdal/topogml_sample.resolved.gml", pReader=0x62ea80, 
    m_bSequentialLayers=0x62eb50) at hugefileresolver.cpp:1776
#13 0x00007ffff720d102 in GMLReader::ParseXMLHugeFile (this=0x62ea80, pszOutputFilename=0x61ef30 "../../trunk/gdal/topogml_sample.resolved.gml", bSqliteIsTempFile=1, 
    iSqliteCacheMB=0) at hugefileresolver.cpp:1999
#14 0x00007ffff720d204 in GMLReader::HugeFileResolver (this=0x62ea80, pszFile=0x61ef30 "../../trunk/gdal/topogml_sample.resolved.gml", bSqliteIsTempFile=1, iSqliteCacheMB=0)
    at hugefileresolver.cpp:2030
#15 0x00007ffff73590b5 in OGRGMLDataSource::Open (this=0x62e740, pszNameIn=0x61f610 "../../trunk/gdal/topogml_sample.xml", bTestOpen=1) at ogrgmldatasource.cpp:603
#16 0x00007ffff735e108 in OGRGMLDriver::Open (this=0x61f170, pszFilename=0x61f610 "../../trunk/gdal/topogml_sample.xml", bUpdate=0) at ogrgmldriver.cpp:69
#17 0x00007ffff745f7f2 in OGRSFDriverRegistrar::Open (pszName=0x61f610 "../../trunk/gdal/topogml_sample.xml", bUpdate=0, ppoDriver=0x7fffffffdb30)
    at ogrsfdriverregistrar.cpp:226
#18 0x00000000004032b5 in main (nArgc=4, papszArgv=0x61f6c0) at ogrinfo.cpp:188

I've tried the following patch :

$ svn diff ogr/ogrsf_frmts/gml
Index: ogr/ogrsf_frmts/gml/hugefileresolver.cpp
===================================================================
--- ogr/ogrsf_frmts/gml/hugefileresolver.cpp	(révision 26408)
+++ ogr/ogrsf_frmts/gml/hugefileresolver.cpp	(copie de travail)
@@ -1388,7 +1388,7 @@
         {
             if( pItem->gmlText != NULL)
                 delete pItem->gmlText;
-            pItem->gmlText = new CPLString( pszGmlText );
+            pItem->gmlText = pszGmlText ? new CPLString( pszGmlText ) : NULL;
             return;
         }
         pItem = pItem->pNext;

which fixes the segfault, but there's still the sqlite3_step() failure that occurs at line 853 of hugefileresolver.cpp, due to a constraint violation (probably a duplicated primary key)

[Even Rouault]

Feedback from Sandro:

I've cloned yet again the current GDAL sources from the SVN, then
I've downloaded my own patch an testcase from Trac, so to be
absolutely sure to replicate your local configuration as close
as possible.

First surprise: on my Debian Wheezy anything works in the cleanest
way; I'm completely unable to see any error or warning, and for sure
I get no crash at all.
This seems to be fully consistent with my previous test performed
immediately before committing my patch.
Anyway it strongly contrasts with your own findings.

So I've patiently set up a full Valgrind session, in order to
identify any possible memory corruption.
And here comes the second surprise:

==24147== Invalid read of size 4
==24147==    at 0x4EFA318: ??? (in 
/usr/local/lib/libspatialite.so.5.1.0)
==24147==    by 0x5CE47C0: ??? (in 
/usr/lib/i386-linux-gnu/libsqlite3.so.0.8.6)
==24147==    by 0x5CC837D: sqlite3_step (in 
/usr/lib/i386-linux-gnu/libsqlite3.so.0.8.6)
==24147==    by 0x5CCE7DE: sqlite3_exec (in 
/usr/lib/i386-linux-gnu/libsqlite3.so.0.8.6)
==24147==    by 0x8057CA8: SetupTargetLayer(OGRDataSource*, OGRLayer*, 
OGRDataSource*, char**, char const*, OGRSpatialReference*, int, char**, 
int, int, int, int, int, int, char**, int, int, char const*, char**, 
char const*) (ogr2ogr.cpp:2681)
==24147==    by 0x8056E26: main (ogr2ogr.cpp:2202)
==24147==  Address 0x740b5bc is 4 bytes inside a block of size 6 
alloc'd
==24147==    at 0x4028308: malloc (in 
/usr/lib/valgrind/vgpreload_memcheck-x86-linux.so)
==24147==    by 0x4EFA09C: ??? (in 
/usr/local/lib/libspatialite.so.5.1.0)
==24147==    by 0x5CE47C0: ??? (in 
/usr/lib/i386-linux-gnu/libsqlite3.so.0.8.6)
==24147==    by 0x5CC837D: sqlite3_step (in 
/usr/lib/i386-linux-gnu/libsqlite3.so.0.8.6)
==24147==    by 0x5CCE7DE: sqlite3_exec (in 
/usr/lib/i386-linux-gnu/libsqlite3.so.0.8.6)
==24147==    by 0x8057CA8: SetupTargetLayer(OGRDataSource*, OGRLayer*, 
OGRDataSource*, char**, char const*, OGRSpatialReference*, int, char**, 
int, int, int, int, int, int, char**, int, int, char const*, char**, 
char const*) (ogr2ogr.cpp:2681)
==24147==    by 0x8056E26: main (ogr2ogr.cpp:2202)
==24147==

after all there is some memory corruption, and this could
easily explain all errors and the final crash you detected;
and anything points my suspects into the direction of some
nasty mismatch affecting sqlite/spatialite (sqlite3_exec).

after further debugging I was finally able to identify the
exact point where this "invalid read" happens:

ogrsqlitedatasource.cpp (near line 2036)
rc = sqlite3_exec( hDB, osCommand, NULL, NULL, &pszErrMsg );
if( rc != SQLITE_OK )
{
     CPLError( CE_Failure, CPLE_AppDefined,
               "Unable to add %s table to geometry_columns:\n%s",
               pszLayerName, pszErrMsg );
     sqlite3_free( pszErrMsg );
     return FALSE;
}

the memory issue materializes while executing these SQL statements:

SELECT AddGeometryColumn('lineeelementari', 'GEOMETRY', 0, 
'MULTILINESTRING', 2)
SELECT AddGeometryColumn('suolo', 'GEOMETRY', 0, 'POLYGON', 2)

the first invocation passes absolutely clean; but the second
one certainly encounters some heavily corrupted memory block,
as clearly reported by Valgrind.
the specific component causing this memory corruption seems to
be exactly libspatialite itself.
GDAL seems to be absolutely extraneous to all this, and the
same is for my proposed patch affecting GML Topo Surface.

once I've reached this first preliminary conclusion then I've soon
updated yet again libspatialite by cloning the most recent shapshot
available from the Fossil repo (I use a different Fedora platform
for any SpatiaLite development, so the Debian Wheezy I was using
to test GDAL was a little bit out of sync)

and here comes the third surprise: ANYTHING WORKS NICELY
Valgrind doesn't raises any longer any complaint, ever the
slightest one.

MY FINAL CONCLUSIONS:
---------------------
1) the proposed patch is crystal clear
2) as is the attached testcase
3) the specific version of libspatialite 4.1.2-devel available
    few weeks ago contained some undetected internal memory
    issue (*); I'm pretty sure about this, because I've corrected
    and fixed a couple of related issues affecting the internal
    connection cache in the afterwhile.
4) such memory issues caused memory corruption in critical areas
    accessed by sqlite/spatialite (sqlite3_exec).
    for some odd reason this passed practically unnoticed on my
    Debian/Fedora, but caused many troubles and a final crash on
    your system (I suppose we are using different versions of libsqlite3
    and may be of the C runtime, and this could probably be the
    real cause explaining any observed difference; dirty memory
    blocks are rather extravagant in their practical consequences)
5) the current snapshot of libspatialite 4.1.2-devel seems to
    be finally immune from any memory related issue; and under
    these conditions both the proposed patch and testcase work
    exactly as expected.

(*) just a short technical note
now the more recent spatialite's versions support two different
initializing APIs:

- void spatialite_init ( int verbose );
- void spatialite_init_ex ( sqlite3 *db_handle, void *p_cache, int 
verbose );

the first form is now obsolete and just preserved for long term
back compatibility.
the second form was introduced so to allow a more specific
handling of each single connection (much better suited for
effective multi-threading, because each connection now have
its own externally allocated cache)

both spatialite CLI and GUI now uniquely use the second form;
unhappily GDAL/OGR still continued to use the legacy form,
who was actually plagued by several memory issues fixed
only last week (badly allocated default connection cache).
and this really is the end of this long and cumbersome story :-D

[Even Rouault]

Sandro,

your above problems are related to spatialite, but my test does not involve spatialite at all. It is just ogrinfo on the GML file, so it just requires sqlite3 support.

Did you really test with the sample file attached to the ticket ? By replacing sqlite3_prepare by sqlite3_prepare_v2, I got a more precise error message and when looking at the file, I can indeed see that "Edge_32_34096" is defined twice in the file (line 197 and 224), which causes a duplicated primary key issue. And there are other problems too with edges Edge_32_34129, Edge_32_34096 and Edge_32_34203.

[esseffe]

Hi Even,

you are absolutely right; I had two different versions of the GML testcase on two different directories, and I stupidly attached the first preliminary version instead of the final right one. Few days ago I was pretty sure to had used for my latest tests exactly the sample downloaded from GDAL Trac, but it's now rather obviously that I finished yet again by testing my local copy.

please see the attached "good" testcase

sorry for any inconvenience, Sandro

[Even Rouault]

trunk r26559 and branches/1.10 r26560 "GML TopoSurface: fix duplicated points in GML_FACE_HOLE_NEGATIVE=YES mode (patch by Sandro Furieri, #5230)"

trunk r26557 "GML huge file resolver: initialize members of huge_tag structure, use sqlite3_prepare_v2 instead of sqlite3_prepare, and add more precise error message in gmlHugeFileSQLiteInsert()"

trunk r26558 "GML huge file resolver: fix crash and mem leak"