GEOS roundtrip conversion drops empty components

[strk]

strk=# select ST_AsText(postgis_geos_noop('GEOMETRYCOLLECTION(POINT EMPTY, POINT(1 1))'));
           st_astext
--------------------------------
 GEOMETRYCOLLECTION(POINT(1 1))
(1 row)

strk=# select ST_AsText('GEOMETRYCOLLECTION(POINT EMPTY, POINT(1 1))');
                 st_astext                  
--------------------------------------------
 GEOMETRYCOLLECTION(POINT EMPTY,POINT(1 1))
(1 row)

It looks our postgis->geos->postgis conversion is dropping EMPTY components. I think it shouldn't.

Found during work on #4813

[strk]

Version 2.5 did not have postgis_geos_noop function so I'm not sure it happens there as well

[Sandro Santilli <strk@…>]

In 846d74b/git:

Do not drop empty geometry components when converting to GEOS

References #4814 in master branch (3.1.0dev)

[Sandro Santilli <strk@…>]

In 1c7c85c4/git:

Update unit test of geos_noop after previous commit

References #4814 in master branch (3.1.0dev)

[strk]

Letting EMPTY components get to GEOS revealed some bugs in GEOS. For example, this query crashes when run against GEOS-3.6.2:

SELECT ST_Intersects( 'LINESTRING(10 0,0 10)', 'MULTILINESTRING((10 -10,-10 10), EMPTY)' );

[strk]

It still crashes GEOS-3.6.4dev

[strk]

I filed ​https://trac.osgeo.org/geos/ticket/1085 for the crash. We'd have to decide if it's safer NOT to pass those EMPTY to GEOS yet, if that opens such can of worms...

[komzpa]

It was actually my very first bug fix for PostGIS. The code can be removed after we stop supporting GEOS 3.8 and before, if the bug fixes will go into 3.9 - it's an explosive set of crashers.

[strk]

Thanks Komzpa, your skipping EMPTY in #3579 was probably a good short-term solution but in the long run we want those crashes fixed upstream, in GEOS. I could revert [1c7c85c4/git] and [846d74b/git] to re-introduce the skip but this would then end up having different results from MakeValid? depending on whether or not we use GEOS to perform the operation - see mail on ​https://lists.osgeo.org/pipermail/postgis-devel/2020-December/028710.html

[strk]

This was done. GEOS-3.6.5 was released to fix the crash there.

[komzpa]

Ubuntu Groovy + POSTGIS="3.1.0 5e2af69" [EXTENSION] PGSQL="130" GEOS="3.8.1-CAPI-1.13.3" SFCGAL="1.3.7" PROJ="7.1.0" GDAL="GDAL 3.1.3, released 2020/09/01" LIBXML="2.9.10" LIBJSON="0.15" LIBPROTOBUF="1.3.3" WAGYU="0.5.0 (Internal)" TOPOLOGY RASTER

Also tested on GEOS 3.9 and 3.10 snapshots:

02:00:17 [gis] > select ST_MakeValid('0106000020110F000000000000');
server closed the connection unexpectedly
        This probably means the server terminated abnormally
        before or while processing the request.
The connection to the server was lost. Attempting reset: Succeeded.
Time: 9361,394 ms (00:09,361)

[strk]

Also crashes with GEOS 3.7.3-CAPI-1.11.3

[strk]

Looks like being a PostGIS, not GEOS, issue:

==12253== Invalid free() / delete / delete[] / realloc()
==12253==    at 0x4C32D3B: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==12253==    by 0x18D89A58: lwcollection_make_geos_friendly (lwgeom_geos_clean.c:345)
==12253==    by 0x18D8A47A: lwgeom_make_valid (lwgeom_geos_clean.c:906)
==12253==    by 0x18CAC1D1: ST_MakeValid (lwgeom_geos_clean.c:71)
==12253==    by 0x348E55: ??? (in /usr/lib/postgresql/10/bin/postgres)
==12253==    by 0x3F02EB: ??? (in /usr/lib/postgresql/10/bin/postgres)
==12253==    by 0x3F1BB0: ??? (in /usr/lib/postgresql/10/bin/postgres)
==12253==    by 0x3F25D9: ??? (in /usr/lib/postgresql/10/bin/postgres)
==12253==    by 0x393960: expression_tree_mutator (in /usr/lib/postgresql/10/bin/postgres)
==12253==    by 0x3F2401: ??? (in /usr/lib/postgresql/10/bin/postgres)
==12253==    by 0x39405A: expression_tree_mutator (in /usr/lib/postgresql/10/bin/postgres)
==12253==    by 0x3F2401: ??? (in /usr/lib/postgresql/10/bin/postgres)
==12253==  Address 0x18908018 is 104 bytes inside a block of size 8,192 alloc'd
==12253==    at 0x4C31B0F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==12253==    by 0x5BA367: ??? (in /usr/lib/postgresql/10/bin/postgres)
==12253==    by 0x5BF7AD: palloc (in /usr/lib/postgresql/10/bin/postgres)
==12253==    by 0x18D65A49: lwcollection_from_gserialized2_buffer (gserialized2.c:1413)
==12253==    by 0x18D65A49: lwgeom_from_gserialized2_buffer (gserialized2.c:1492)
==12253==    by 0x18D673C2: lwgeom_from_gserialized2 (gserialized2.c:1529)
==12253==    by 0x18CAC1BD: ST_MakeValid (lwgeom_geos_clean.c:49)
==12253==    by 0x348E55: ??? (in /usr/lib/postgresql/10/bin/postgres)
==12253==    by 0x3F02EB: ??? (in /usr/lib/postgresql/10/bin/postgres)
==12253==    by 0x3F1BB0: ??? (in /usr/lib/postgresql/10/bin/postgres)
==12253==    by 0x3F25D9: ??? (in /usr/lib/postgresql/10/bin/postgres)
==12253==    by 0x393960: expression_tree_mutator (in /usr/lib/postgresql/10/bin/postgres)
==12253==    by 0x3F2401: ??? (in /usr/lib/postgresql/10/bin/postgres)

[Sandro Santilli <strk@…>]

In d890e2c/git:

Do not free 0-allocated geoms vector for made-valid empty collections

References #4814

[Sandro Santilli <strk@…>]

In 2cbf963/git:

Add unit test for cleaning single empty component collection

References #4814

[Sandro Santilli <strk@…>]

In 6d74ddcf/git:

Do not free 0-allocated geoms vector for made-valid empty collections

References #4814 in 3.1 branch
Includes unit test and NEWS entry

[strk]

I think this is still fixed, not sure why it was reopened rather than filing a new bug for the crash (now also fixed). Please file a new ticket (maybe referencing this one) if there's another crash.