search

yeesian / ArchGDAL.jl

A high level API for GDAL - Geospatial Data Abstraction Library
https://yeesian.github.io/ArchGDAL.jl/stable/
Other
141 stars 27 forks source link

preparing geometry crashes with segfault (on M1 MacBook) #336

Closed henrik-wolf closed 1 year ago

henrik-wolf commented 1 year ago

When I am trying to prepare geometry using ArchGDAL.preparegeom, the Julia process crashes:

julia> ArchGDAL.has_preparedgeom_support()  # not running a custom version of GDAL, just checking if this is true
true

julia> p = ArchGDAL.fromWKT("POLYGON ((-1.18871433326168 52.9075475452531,-1.18874327518357 52.9074308810968,-1.18885830894734 52.9074415541226,-1.18882863220356 52.9075577642415,-1.18871433326168 52.9075475452531))")
Geometry: POLYGON ((-1.18871433326168 52.9075475452531,-1.18 ... 531))

julia> ArchGDAL.preparegeom(p)
Geometry: 
signal (11): Segmentation fault: 11
in expression starting at none:0
unknown function (ip: 0x0)
Allocations: 5232791 (Pool: 5228695; Big: 4096); GC: 6

The same thing happens with a linestring:

julia> line = ArchGDAL.createlinestring([(1.0, 5.8), (2.0, 2.3), (4.5, 9.3)])
Geometry: LINESTRING (1.0 5.8,2.0 2.3,4.5 9.3)

julia> ArchGDAL.preparegeom(line)
Geometry: 
signal (11): Segmentation fault: 11
in expression starting at none:0
unknown function (ip: 0x0)
Allocations: 4344893 (Pool: 4341145; Big: 3748); GC: 5

I am running julia 1.8.1 on a MacBookAir M1, and ArchGDAL version 0.9.2

julia> versioninfo()
Julia Version 1.8.1
Commit afb6c60d69a (2022-09-06 15:09 UTC)
Platform Info:
  OS: macOS (arm64-apple-darwin21.5.0)
  CPU: 8 × Apple M1
  WORD_SIZE: 64
  LIBM: libopenlibm
  LLVM: libLLVM-13.0.1 (ORCJIT, apple-m1)
  Threads: 1 on 4 virtual cores
visr commented 1 year ago

Hmm I can reproduce this on Windows on Julia 1.8.2. There are prepared geometry tests that fail similarly, https://github.com/yeesian/ArchGDAL.jl/blob/master/test/test_prepared_geometry.jl. I wonder what happened.

yeesian commented 1 year ago

I think it traces to the segfault in GDAL.jl when we call GDAL.ogr_g_exporttowkt(...) on a preparedgeometry. To reproduce:

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

julia> using ArchGDAL, GDAL

julia> p = ArchGDAL.fromWKT("POLYGON ((-1.18871433326168 52.9075475452531,-1.18874327518357 52.9074308810968,-1.18885830894734 52.9074415541226,-1.18882863220356 52.9075577642415,-1.18871433326168 52.9075475452531))")
Geometry: POLYGON ((-1.18871433326168 52.9075475452531,-1.18 ... 531))

julia> preparedgeom = ArchGDAL.preparegeom(p);

julia> wkt_ptr = Ref(Cstring(C_NULL))
Base.RefValue{Cstring}(Cstring(0x0000000000000000))

julia> GDAL.ogr_g_exporttowkt(preparedgeom.ptr, wkt_ptr)

signal (11): Segmentation fault: 11

It isn't obvious in https://github.com/yeesian/ArchGDAL.jl/issues/336#issue-1411939180 because the segfault only happens when we attempt to display the object in the terminal, which traces to the call to toWKT(...), which invokes the following code: https://github.com/yeesian/ArchGDAL.jl/blob/d6edcde5ff88d9eac837e5b41e2f77bb2303f3b5/src/ogr/geometry.jl#L319-L326

henrik-wolf commented 1 year ago

Ok. great! That means I can actually use prepared geometry, as long as I do not try to look at it. Is this something that can be fixed with relative ease? This seems to be (from what little I understand about wrapping) a problem in the wrapped GDAL library, rather than in the julia code. Maybe, we could implement our own toWKT method, dispatching on something like ArchGDAL.IPreparedGeometry? (Or rather ArchGDAL.AbstractPreparedGeometry)

visr commented 1 year ago

This seems to be (from what little I understand about wrapping) a problem in the wrapped GDAL library, rather than in the julia code.

Yeah it could be. I wonder though, why do the tests fail if ogr_g_exporttowkt is not called there? It would also be good to see if we run into the same issue using LibGEOS.jl. Then we know if the error is at GEOS or GDAL.

It might be helpful to temporarily add a method like this to your session to avoid segfaults:

ArchGDAL.toWKT(geom::ArchGDAL.AbstractPreparedGeometry) = error("trading a segfault for an error")

Though it's best to focus on getting this fixed upstream if that is the problem.

henrik-wolf commented 1 year ago

I wonder though, why do the tests fail if ogr_g_exporttowkt is not called there? It would also be good to see if we run into the same issue using LibGEOS.jl. Then we know if the error is at GEOS or GDAL.

I just ran all ArchGDAL tests on julia 1.8.1 and 1.8.2 and can't reproduce any failures. Also, I tried LibGEOS.jl, there preparing geometry seems to work:

julia> using LibGEOS

julia> poly = readgeom("POLYGON ((-1.18871433326168 52.9075475452531,-1.18874327518357 52.9074308810968,-1.18885830894734 52.9074415541226,-1.18882863220356 52.9075577642415,-1.18871433326168 52.9075475452531))")
Polygon(Ptr{Nothing} @0x0000600000a20ff0)

julia> prepareGeom(poly)
LibGEOS.PreparedGeometry{Polygon}(Ptr{Nothing} @0x0000600000d1f5a0, Polygon(Ptr{Nothing} @0x0000600000a20ff0))

Does this already show this is a GDAL problem? I can imagine that something like getting the WKT from a piece of prepared geometry is just not something you would usually do as it is somehow optimised for computational performance...

visr commented 1 year ago

Thanks for checking. I extended your LibGEOS snippet to print to WKT using libgeos only. We need to add a method to writegeom otherwise we get a MethodError. But if we do that, I believe we see the same error.

julia> using LibGEOS

julia> poly = readgeom("POLYGON ((-1.18871433326168 52.9075475452531,-1.18874327518357 52.9074308810968,-1.18885830894734 52.9074415541226,-1.18882863220356 52.9075577642415,-1.18871433326168 52.9075475452531))")
Polygon(Ptr{Nothing} @0x0000600000a20ff0)

julia> prep = prepareGeom(poly)
LibGEOS.PreparedGeometry{Polygon}(Ptr{Nothing} @0x0000600000d1f5a0, Polygon(Ptr{Nothing} @0x0000600000a20ff0))

julia> LibGEOS.writegeom(obj::LibGEOS.PreparedGeometry, context::LibGEOS.GEOSContext = LibGEOS._context) =
           LibGEOS._writegeom(obj.ptr, context)

julia> writegeom(prep)

Please submit a bug report with steps to reproduce this fault, and any error messages that follow (in their entirety). Thanks.
Exception: EXCEPTION_ACCESS_VIOLATION at 0x630a93b0 -- ZTIN4geos4geom4prep18PreparedLineStringE at d:\visser_mn\.julia\artifacts\c024ee55cc5bbe84dd4411a68b02dd9a3797fdcd\bin\libgeos.dll (unknown line)
in expression starting at REPL[13]:1
ZTIN4geos4geom4prep18PreparedLineStringE at d:\visser_mn\.julia\artifacts\c024ee55cc5bbe84dd4411a68b02dd9a3797fdcd\bin\libgeos.dll (unknown line)
Allocations: 9044897 (Pool: 9038697; Big: 6200); GC: 11

I don't know if there is a reason that GEOSWKTWriter_write_r doesn't support prepared geometries. Preparing is mainly meant for spatial operations, not serializing to WKT I guess. Wouldn't hurt to ask the on the GEOS issue tracker or mailing list however, I cannot directly find previous discussions about this.

LibGEOS.PreparedGeometry has an ownedby field with the unprepared geometry, so if you use that it can still work using LibGEOS.jl:

julia> LibGEOS.writegeom(obj::LibGEOS.PreparedGeometry, context::LibGEOS.GEOSContext = LibGEOS._context) =
           LibGEOS._writegeom(obj.ownedby.ptr, context)

julia> writegeom(prep)
"POLYGON ((-1.18871433326168 52.9075475452531, -1.18874327518357 52.9074308810968, -1.18885830894734 52.9074415541226, -1.18882863220356 52.9075577642415, -1.18871433326168 52.9075475452531))"

Perhaps we should add that method to LibGEOS :)

henrik-wolf commented 1 year ago

Mayb that is a good idea, but I guess that does not really fix the problem with ArchGDAL. So... just for me to double check if I understood everything correctly... GDAL is the larger library which, (if built correctly) has the GEOS library already built in. So I would expect all geometry operations done with ArchGDAL to eventually call the same GEOS code that gets called when we do these operations with LibGEOS.jl, although not through the julia interface defined on them but rather via a linked library (or something like that.) Is this at least conceptually correct?

From my short experiments over the last days I could find that working with prepared geometry is in general a fairly dicey operation, because (nearly?) every operation which I am not allowed to do with prepared geometry instantly results in a segfault, rather than in a error or a warning. (this is probably in the same line of thought as JuliaGeo/LibGEOS.jl#120, but for this package... So wouldn't it make sense to use a similar definition like the one in LibGEOS.jl for the prepared geometry and implement the 3 to 5 functions which actually work with prepared geometry separately? For everything else, we could either throw an error, or a warning and fall back to original geometry. As this bug seems to come from me trying to call stuff in GEOS, which was (I would guess deliberately) not intended to be called with the stuff I am trying to call it with.

Alternatively, would it be a bad idea (except of course for the amount of work it would take...) to drop GEOS support from the GDAL build and glue the libraries together in julia? Not sure if that would make any sense...

yeesian commented 1 year ago

Sorry for the slow response. Responding one-by-one:

Mayb that is a good idea, but I guess that does not really fix the problem with ArchGDAL. [...] Is this at least conceptually correct?

Yeah, to my understanding, it is conceptually correct. Their source code is also available on github, so you can trace how it'll be flowing through the respective libraries. Last I remember, GDAL converts the corresponding geometries into LibGEOS geometries via WKB.

So wouldn't it make sense to use a similar definition like the one in LibGEOS.jl for the prepared geometry and implement the 3 to 5 functions which actually work with prepared geometry separately? For everything else, we could either throw an error, or a warning and fall back to original geometry. As this bug seems to come from me trying to call stuff in GEOS, which was (I would guess deliberately) not intended to be called with the stuff I am trying to call it with.

Yes that makes sense, and we should have been more defensive about guarding against unsupported operations in this package, rather than leaving it to users to discover them via segfaults in the underlying C libraries. I was too liberal with generic programming and am slow to tighten the function arguments and am sorry about the experience.

Alternatively, would it be a bad idea (except of course for the amount of work it would take...) to drop GEOS support from the GDAL build and glue the libraries together in julia? Not sure if that would make any sense...

Yeah I think that's right, and there is nothing to stop you from doing so (apart from the corresponding work required; and corresponding burden of maintaining it with subsequent updates to GDAL).