search

locationtech / jts

The JTS Topology Suite is a Java library for creating and manipulating vector geometry.
Other
1.94k stars 442 forks source link

Fix unexpected and unwrapped NullPointerException #994

Open arthurscchan opened 1 year ago

arthurscchan commented 1 year ago

This fixes an unwrapped NullPointerException where a null value could be returned from getcoordinate() method in modules/core/src/main/java/org/locationtech/jts/geom/Point.java.

The getCoordinate() method in the Point class will return a null value if there are no coordinates stored. This could cause potential NullPointerException as the return null has been used in many code without further null check. Indeed, further process on empty point with no coordinates are meaningless. For example, in https://github.com/locationtech/jts/blob/master/modules/core/src/main/java/org/locationtech/jts/operation/distance/DistanceOp.java#L423-L427, it calls the getCoordinate() method and could get a null object. Then the coordinate object is passed to the Distance.pointToSegment() method. The coordinate is then used in https://github.com/locationtech/jts/blob/master/modules/core/src/main/java/org/locationtech/jts/algorithm/Distance.java#L169 and cause a NullPointerException because no null checking is in place. There are many other places that have the similar situation, thus the best way to solve the possible NullPointerException in different locations is to throw an exception directly when the getCoordinate() method is called on an empty point instance. This is similar to the handling in the getX() and getY() method in the same Point class.

This PR wraps the possible NullPointerException by throwing an IllegalStateException when the getCoordinate() method is called on an empty point instance with no coordinates. This avoids further NullPointerException when code trying to use the null coordinate on some other logic.

We found this bug using fuzzing by way of OSS-Fuzz, where we recently integrated jts (https://github.com/google/oss-fuzz/pull/10745). OSS-Fuzz is a free service run by Google for fuzzing important open source software. If you'd like to know more about this then I'm happy to go into detail and also set up things so you can receive emails and detailed reports when bugs are found.

dr-jts commented 9 months ago

This seems to fall under the task of improving how JTS handles EMPTY geometries. In fact, the DistanceOp issue was fixed in #1010. If there are other places where empty geometries are not being handled correctly, then there should be unit tests added for those cases.

It seems to me to be mostly a matter of taste as to whether the problem is detected by an NPE or by an ISE.

In any case, if this change is to be made it should be done in all the Geometry classes so they have identical semantics for getCoordinate(). There's also a downstream implication for GEOS, which I'm not sure about.

dr-jts commented 9 months ago

It would be good if you can complete the Eclipse ECA so that CI can run on this.