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

interiorRing crash #128

Closed
skygering opened this issue Sep 2, 2022 · 2 comments · Fixed by #131
Closed

interiorRing crash #128

skygering opened this issue Sep 2, 2022 · 2 comments · Fixed by #131
Labels

Comments

@skygering
Copy link
Contributor

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

rafaqz commented Sep 3, 2022

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

@yeesian yeesian added the bug label Sep 3, 2022
@yeesian
Copy link
Member

yeesian commented Sep 3, 2022

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

# Return NULL on exception, Geometry must be a Polygon.
# Returned object is a pointer to internal storage: it must NOT be destroyed directly.
function interiorRing(ptr::GEOSGeom, n::Integer, context::GEOSContext = _context)
result = GEOSGetInteriorRingN_r(context.ptr, ptr, n - 1)
if result == C_NULL
error("LibGEOS: Error in GEOSGetInteriorRingN")
end
cloneGeom(result, context)
end

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.

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

Successfully merging a pull request may close this issue.

3 participants