Fix segfault in SimplePointInAreaLocator caused by casting MultiPolyg… #334

sumeerbhola · 2020-09-02T14:06:34Z

…on to Polygon

References https://trac.osgeo.org/geos/ticket/1047
References cockroachdb/cockroach#53254

This change is

otan · 2020-09-02T15:01:33Z

src/algorithm/locate/SimplePointInAreaLocator.cpp

-            }
+        auto poly = dynamic_cast<const Polygon*>(geom->getGeometryN(0));
+        if (poly) {
+            return locatePointInPolygon(p, dynamic_cast<const Polygon*>(geom->getGeometryN(0)));


maybe use poly as the second argument?

otan · 2020-09-02T15:01:50Z

tests/unit/algorithm/PointLocatorTest.cpp

@@ -111,5 +120,15 @@ void object::test<5>
                 "MULTIPOINT ((1 1), (0 0))");
 }

+// 6 - TestPointLocator Point inside GeometryCollection containing MultiPolygon, using SimplePointInAreaLocator.


maybe worth adding a clarification it is a single Polygon inside a MULTIPOLYGON

dr-jts · 2020-09-02T18:06:01Z

In JTS at least, spatial predicates do not support GEOMETRYCOLLECTIONs as inputs. So this is a case of erroneous input. But should not be allowed to cause a segfault, for sure.

dr-jts · 2020-09-02T18:15:39Z

The idea of generalizing SimplePointInAreaLocator to handle any kind of collection isn't a bad one. But the fact that this allows intersects to handle a geometry like

GEOMETRYCOLLECTION (MULTIPOLYGON (((9223372036854776000 -9223372036854776000, 9223372036854776000 9223372036854776000, -9223372036854776000 9223372036854776000, -9223372036854776000 9223372036854776000, 9223372036854776000 -9223372036854776000))))

(as per the original ticket GEOS-1047) is just a lucky accident. You should review your upstream code and ensure it meets the contract for GEOS spatial predicates.

…on to Polygon References https://trac.osgeo.org/geos/ticket/1047 References cockroachdb/cockroach#53254

otan · 2020-09-02T18:26:18Z

You should review your upstream code and ensure it meets the contract for GEOS spatial predicates.

This is for a database (i.e. CockroachDB), where the users can specify the kinds of shapes coming in. We're just the messenger. This affects PostGIS as well (https://trac.osgeo.org/geos/ticket/1042).

We don't control the shapes; this was one we happened to bump into whilst randomized testing a variety of different unions that led to this shape. By chance, it also affects intersects.

However, the seg fault as it stands is definitely not a good situation for us as it is not easily recoverable.

dr-jts · 2020-09-02T18:30:48Z

I also note that the original CockroachDB issue involved computing an aggregate union of two geometry collections (one heterogeneous). This seems to be supported in PostGIS, but not sure how GEOS is being used to perform that.

dr-jts · 2020-09-02T18:33:48Z

You should review your upstream code and ensure it meets the contract for GEOS spatial predicates.

This is for a database (i.e. CockroachDB), where the users can specify the kinds of shapes coming in. We're just the messenger. This affects PostGIS as well (https://trac.osgeo.org/geos/ticket/1042).

We don't control the shapes; this was one we happened to bump into whilst randomized testing a variety of different unions that led to this shape. By chance, it also affects intersects.

However, the seg fault as it stands is definitely not a good situation for us as it is not easily recoverable.

If you're using a library, it's your responsibility to ensure that you are not providing inputs to the library which are out of contract for it.

As said, I agree that the lib should not segfault on bad inputs. But the answer is not necessarily to hack a "fix" deep inside the codebase with no knowledge of how that might affect other code in the library. The library should detect known situations which are not handled and report that via standard error mechanism.

otan · 2020-09-02T19:04:07Z

As said, I agree that the lib should not segfault on bad inputs. But the answer is not necessarily to hack a "fix" deep inside the codebase with no knowledge of how that might affect other code in the library. The library should detect known situations which are not handled and report that via standard error mechanism.

It may be more helpful if you gave a pointer as to where / what layer you prefer changed in this situation.

It sounds like if you want GEOMETRYCOLLECTION spatial predicates to not work entirely, it will break existing cases. This seems like it already works with MULTIPOLYGON inside GEOMETRYCOLLECTION if the MULTIPOLYGON has >= 2 shapes within it. Sounds like that would break existing PostGIS cases as well?

edit: eh, this breaks PostGIS too (but seems like it may miss if you hit the PreparedIntersects path):

otan=# select st_intersects('01020000000200000000000000000000000000000000000000000000000000F03F0000000000000000', '0107000000010000000106000000010000000103000000010000000500000008690b3a70a9c347540a11d15993eec7e04b9e4d8fd2cb4702907c2929abe847d02aef5652e3cec72c1b57867da3dd47bbf47698d501eac7ccc0d287b380d44708690b3a70a9c347540a11d15993eec7');
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: Failed.

dr-jts · 2020-09-02T19:24:17Z

It may be more helpful if you gave a pointer as to where / what layer you prefer changed in this situation.

I'm not sure at the moment. In JTS GeometryCollection input to predicates are simply disallowed. I'm not sure why this isn't implemented in GEOS.

It sounds like if you want GEOMETRYCOLLECTION spatial predicates to not work entirely, it will break existing cases. This seems like it already works with MULTIPOLYGON inside GEOMETRYCOLLECTION if the MULTIPOLYGON has >= 2 shapes within it. Sounds like that would break existing PostGIS cases as well?

Are there existing PostGIS unit tests that rely on this?

If a GC with a single MultiPolygon works, that is just a happy accident. But I suppose it could be made explicit behaviour. I'd prefer that was done more explicitly, however, not just as a lucky side-effect of a sub-module of the overlay algorithm. (I.e. explicitly code for unwrapping GCs containing a single element which is itself not a GC).

dr-jts · 2020-09-02T19:48:53Z

Whatever the way this is handled, there needs to be a unit test covering this case. Otherwise this risks a regression failure if and when the predicate code is modified or replaced.

sumeerbhola · 2020-09-02T19:56:33Z

there needs to be a unit test covering this case

This PR includes a change to PointLocatorTest.cpp. Should there be more tests?

dr-jts · 2020-09-02T20:44:21Z

there needs to be a unit test covering this case

This PR includes a change to PointLocatorTest.cpp. Should there be more tests?

Yes, but perhaps they should not be part of this PR. If the ultimate goal is to have GEOS support GeometryCollection inputs to predicates (or overlay operations), then there needs to be tests at that level. As mentioned before, if there aren't unit tests at the level of the actual operation, then there is a risk of future development causing a regression.

This will need to be based on a decision by the GEOS team about how to handle this. Or not - currently GC inputs sometimes work, sometimes probably return the wrong answer, and sometimes cause topology exceptions. To my mind this means they basically aren't handled in a meaningful way.

strk · 2020-11-06T19:20:59Z

Thanks all, this was merged in master as 07e6046 - backports are in progress

sumeerbhola mentioned this pull request Sep 2, 2020

GEOS: panic for single MULTIPOLYGON within a GEOMETRYCOLLECTION cockroachdb/cockroach#53254

Closed

otan reviewed Sep 2, 2020

View reviewed changes

Fix segfault in SimplePointInAreaLocator caused by casting MultiPolyg…

6cc8529

…on to Polygon References https://trac.osgeo.org/geos/ticket/1047 References cockroachdb/cockroach#53254

sumeerbhola force-pushed the mpcast_master_fix branch from 3e86b69 to 6cc8529 Compare September 2, 2020 18:21

otan deleted the mpcast_master_fix branch September 8, 2020 15:13

otan restored the mpcast_master_fix branch September 8, 2020 15:22

strk closed this Nov 6, 2020

pramsey mentioned this pull request Nov 14, 2022

3.11.1: test suite is failing in test_docs unit #734

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix segfault in SimplePointInAreaLocator caused by casting MultiPolyg… #334

Fix segfault in SimplePointInAreaLocator caused by casting MultiPolyg… #334

sumeerbhola commented Sep 2, 2020 •

edited

Loading

otan Sep 2, 2020

sumeerbhola Sep 2, 2020

otan Sep 2, 2020

dr-jts commented Sep 2, 2020

dr-jts commented Sep 2, 2020

otan commented Sep 2, 2020

dr-jts commented Sep 2, 2020

dr-jts commented Sep 2, 2020 •

edited

Loading

otan commented Sep 2, 2020 •

edited

Loading

dr-jts commented Sep 2, 2020

dr-jts commented Sep 2, 2020

sumeerbhola commented Sep 2, 2020

dr-jts commented Sep 2, 2020 •

edited

Loading

strk commented Nov 6, 2020

Fix segfault in SimplePointInAreaLocator caused by casting MultiPolyg… #334

Fix segfault in SimplePointInAreaLocator caused by casting MultiPolyg… #334

Conversation

sumeerbhola commented Sep 2, 2020 • edited Loading

otan Sep 2, 2020

Choose a reason for hiding this comment

sumeerbhola Sep 2, 2020

Choose a reason for hiding this comment

otan Sep 2, 2020

Choose a reason for hiding this comment

dr-jts commented Sep 2, 2020

dr-jts commented Sep 2, 2020

otan commented Sep 2, 2020

dr-jts commented Sep 2, 2020

dr-jts commented Sep 2, 2020 • edited Loading

otan commented Sep 2, 2020 • edited Loading

dr-jts commented Sep 2, 2020

dr-jts commented Sep 2, 2020

sumeerbhola commented Sep 2, 2020

dr-jts commented Sep 2, 2020 • edited Loading

strk commented Nov 6, 2020

sumeerbhola commented Sep 2, 2020 •

edited

Loading

dr-jts commented Sep 2, 2020 •

edited

Loading

otan commented Sep 2, 2020 •

edited

Loading

dr-jts commented Sep 2, 2020 •

edited

Loading