Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix unexpected and unwrapped NullPointerException #994

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

arthurscchan
Copy link

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 (google/oss-fuzz#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.

Signed-off-by: Arthur Chan <[email protected]>
@dr-jts
Copy link
Contributor

dr-jts commented Nov 22, 2023

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
Copy link
Contributor

dr-jts commented Nov 22, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants