Opened 6 months ago

Closed 6 months ago

#4814 closed defect (fixed)

GEOS roundtrip conversion drops empty components

Reported by: strk Owned by: strk
Priority: critical Milestone: PostGIS 3.1.1
Component: postgis Version: 2.5.x
Keywords: Cc:

Description

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

Change History (18)

comment:1 Changed 6 months ago by strk

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

comment:2 Changed 6 months ago by Sandro Santilli <strk@…>

In 846d74b/git:

Do not drop empty geometry components when converting to GEOS

References #4814 in master branch (3.1.0dev)

comment:3 Changed 6 months ago by Sandro Santilli <strk@…>

In 1c7c85c4/git:

Update unit test of geos_noop after previous commit

References #4814 in master branch (3.1.0dev)

comment:4 Changed 6 months ago by 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)' );

comment:5 Changed 6 months ago by strk

It still crashes GEOS-3.6.4dev

comment:6 Changed 6 months ago by 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...

comment:7 Changed 6 months ago by 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.

comment:8 Changed 6 months ago by 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

comment:9 Changed 6 months ago by strk

Resolution: fixed
Status: newclosed

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

comment:10 Changed 6 months ago by komzpa

Priority: mediumcritical
Resolution: fixed
Status: closedreopened

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)

comment:11 Changed 6 months ago by strk

Also crashes with GEOS 3.7.3-CAPI-1.11.3

comment:12 Changed 6 months ago by 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)

comment:13 Changed 6 months ago by Sandro Santilli <strk@…>

In d890e2c/git:

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

References #4814

comment:14 Changed 6 months ago by Sandro Santilli <strk@…>

In 2cbf963/git:

Add unit test for cleaning single empty component collection

References #4814

comment:15 Changed 6 months ago by 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

comment:16 Changed 6 months ago by pramsey

Milestone: PostGIS 3.1.0PostGIS 3.1.1

comment:17 Changed 6 months ago by pramsey

Owner: changed from pramsey to strk
Status: reopenednew

comment:18 Changed 6 months ago by strk

Resolution: fixed
Status: newclosed

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.

Note: See TracTickets for help on using tickets.