JuliaGeo / LibGEOS.jl

Julia package for manipulation and analysis of planar geometric objects
MIT License
72 stars 23 forks source link

interiorRing crash #128

Closed skygering closed 2 years ago

skygering commented 2 years ago

I was experimenting with the functionality for getting interior and exterior rings. I first created a polygon with a hole, which I call hpoly = LibGEOS.readgeom("POLYGON((0 0, 4 0, 4 4, 0 4, 0 0), (1 1, 1 2, 2 2, 2 1, 1 1))") I can get that there are two geometries using GeoInterface.ngeom(hpoly). Then, I can get pointers to the exterior ring with GeoInterface.getgeom(hpoly, 1) and to the interior ring with GeoInterface.getgeom(hpoly, 2). However, if I call GeoInterface.getgeom(hpoly, 3) it crashes my terminal. Obviously, there is no second interior ring, but it seems like the out of bounds error should not crash the terminal. This lead me to investigate and LibGEOS.interiorRing(hpoly.ptr, 2) also crashes the terminal, so I think the root issue is within LibGEOS, not the GeoInterface.

Something else I noticed is that calling both LibGEOS.interiorRings(hpoly) and LibGEOS.exteriorRing(hpoly) work, however, LibGEOS.interiorRing(hpoly, 1) does not work and needs to be called like LibGEOS.interiorRing(hpoly.ptr, 1). This really isn't a problem, but thought that the difference in syntax was worth noting.

Thank you so much for your help!

rafaqz commented 2 years ago

Hmm, nothing should ever crash your terminal using Julia... we seem to be accessing some memory we shouldn't be.

yeesian commented 2 years ago

However, if I call GeoInterface.getgeom(hpoly, 3) it crashes my terminal.

Yeah I can reproduce:

  | | |_| | | | (_| |  |  Version 1.8.0 (2022-08-17)
 _/ |\__'_|_|_|\__'_|  |  Official https://julialang.org/ release
|__/                   |

julia> using LibGEOS, GeoInterface

julia> hpoly = LibGEOS.readgeom("POLYGON((0 0, 4 0, 4 4, 0 4, 0 0), (1 1, 1 2, 2 2, 2 1, 1 1))")
Polygon(Ptr{Nothing} @0x0000600003e495e0)

julia> GEOSGetInteriorRingN_r
ERROR: UndefVarError: GEOSGetInteriorRingN_r not defined

julia> LibGEOS.GEOSGetInteriorRingN_r(LibGEOS._context.ptr, hpoly.ptr, 2)
Ptr{Nothing} @0x00007740e674af70

julia> LibGEOS.GEOSGetInteriorRingN_r(LibGEOS._context.ptr, hpoly.ptr, 4)
Ptr{Nothing} @0x00007740e674af80

julia> LibGEOS.GEOSGetInteriorRingN_r(LibGEOS._context.ptr, hpoly.ptr, 10)
Ptr{Nothing} @0x00007740e674afb0

julia> LibGEOS.cloneGeom(ans, LibGEOS._context)

The issue is this package assumes that the following function call will return C_NULL when the index is out-of-bounds. However, it was returning pointers to memory we should not access, and hence the session crashes when we call cloneGeom in https://github.com/JuliaGeo/LibGEOS.jl/blob/ce017e7cd730bd200074bb47a3a49a8cdfdb1487/src/geos_functions.jl#L1102-L1110

Given the behavior of the LibGEOS C library, We should check bounds using 0 < n <= numInteriorRings(ptr, context) before calling GEOSGetInteriorRingN_r(context.ptr, ptr, n - 1).

LibGEOS.interiorRing(hpoly, 1) does not work and needs to be called like LibGEOS.interiorRing(hpoly.ptr, 1)

Yes, the package lacks an implementation for it in https://github.com/JuliaGeo/LibGEOS.jl/blob/master/src/geos_operations.jl (and possibly isn't comprehensive in the coverage of all the functions in https://github.com/JuliaGeo/LibGEOS.jl/blob/master/src/geos_functions.jl just yet. Thank you for reporting the inconsistency.