Opened 2 years ago

Closed 2 years 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 by strk, 2 years ago

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

comment:2 by Sandro Santilli <strk@…>, 2 years ago

In 846d74b/git:

Do not drop empty geometry components when converting to GEOS

References #4814 in master branch (3.1.0dev)

comment:3 by Sandro Santilli <strk@…>, 2 years ago

In 1c7c85c4/git:

Update unit test of geos_noop after previous commit

References #4814 in master branch (3.1.0dev)

comment:4 by strk, 2 years ago

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 by strk, 2 years ago

It still crashes GEOS-3.6.4dev

comment:6 by strk, 2 years ago

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 by komzpa, 2 years ago

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 by strk, 2 years ago

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 by strk, 2 years ago

Resolution: fixed
Status: newclosed

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

comment:10 by komzpa, 2 years ago

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 by strk, 2 years ago

Also crashes with GEOS 3.7.3-CAPI-1.11.3

comment:12 by strk, 2 years ago

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 by Sandro Santilli <strk@…>, 2 years ago

In d890e2c/git:

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

References #4814

comment:14 by Sandro Santilli <strk@…>, 2 years ago

In 2cbf963/git:

Add unit test for cleaning single empty component collection

References #4814

comment:15 by Sandro Santilli <strk@…>, 2 years ago

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 by pramsey, 2 years ago

Milestone: PostGIS 3.1.0PostGIS 3.1.1

comment:17 by pramsey, 2 years ago

Owner: changed from pramsey to strk
Status: reopenednew

comment:18 by strk, 2 years ago

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.