Add managed pointer types for GEOS objects

[dbaston]

Feature description

GEOS objects are currently managed using raw pointers in OGR. This can require cumbersome error handling to make sure that e.g. GEOSGeom_destroy is always called. Switching to a managed pointer type with a custom deleter would simplify code that accesses GEOS. Such functionality has been implemented multiple times in different libraries, e.g

QGIS: https://github.com/qgis/QGIS/blob/1ae7a8d43803042ec7a7519f52d997b8389327c4/src/core/geometry/qgsgeos.h#L112 DuckDB spatial: https://github.com/duckdb/duckdb_spatial/blob/70437d6a36860e33a532b0a7b04f428e4c41f2b3/spatial/include/spatial/geos/geos_wrappers.hpp#L8 sf: https://github.com/r-spatial/sf/blob/main/src/geos.cpp#L149 exactextract: https://github.com/isciences/exactextract/blob/master/src/geos_utils.h#L37

Perhaps a comprehensive stand-alone header would allow one implementation of the wrappers to be useful for several projects.

Additional context

No response

[ctoney]

Hi, Is this possibly related to clang-asan reporting "alloc-dealloc-mismatch (operator new vs free)" from something like:

#include "ogr_api.h"
#include "ogr_core.h"
// ...
hBufferGeom = OGR_G_Buffer(hGeom, dist, quad_segs);
// ...
OGR_G_DestroyGeometry(hBufferGeom);
OGR_G_DestroyGeometry(hGeom);

I'm not sure whether it is on my end or could be coming from the library. I have not seen any adverse impacts. The ASAN output begins with:

==5955==ERROR: AddressSanitizer: alloc-dealloc-mismatch (operator new vs free) on 0x5040004f6250
    #0 0x55b9cd457306 in free (/opt/R/devel-asan/lib/R/bin/exec/R+0xc6306) (BuildId: cb254f3d04cf7922c4e1fdd6fa95d7744719b059)
    #1 0x7f982e1d8bdb in std::range_error::~range_error() (/usr/lib/x86_64-linux-gnu/libc++abi.so.1+0x27bdb) (BuildId: 4c4f8ebb4694263c348f11485d58cebc88198ac4)
    #2 0x7f97fb3e3b08 in geos::operation::buffer::BufferOp::bufferOp(geos::geom::Geometry const*, double, int, int) (/usr/lib/x86_64-linux-gnu/libgeos.so.3.10.2+0x121b08) (BuildId: 1016ca8006a56f14d37db933b3b207ed301bed07)
    #3 0x7f97fb371b96 in geos::geom::Geometry::buffer(double, int) const (/usr/lib/x86_64-linux-gnu/libgeos.so.3.10.2+0xafb96) (BuildId: 1016ca8006a56f14d37db933b3b207ed301bed07)
    #4 0x7f97ff714eff in GEOSBuffer_r (/usr/lib/x86_64-linux-gnu/libgeos_c.so.1+0x1beff) (BuildId: b850b673345ff974ddf599ecd2c11fec376c0dc2)
    #5 0x7f98001a089e in OGRGeometry::Buffer(double, int) const (/lib/libgdal.so.30+0x3a589e) (BuildId: 8fa26281300dd3ccf4056f46ce99a701951bfe84)
    #6 0x7f98015f2119 in _g_buffer(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>, double, int) /__w/gdalraster/gdalraster/check/gdalraster.Rcheck/00_pkg_src/gdalraster/src/geos_wkt.cpp:649:19
    #7 0x7f98015090ce in _gdalraster__g_buffer
...

[dbaston]

It's about avoiding reducing the amount of logic inside GDAL for making sure GEOS objects are cleaned up after use. An example is OGRGeometry::Centroid where much of the logic and branching comes from cleaning up GEOS objects.

https://github.com/OSGeo/gdal/blob/5573b18034fc8c6b84c709bef92a4658c7d738af/ogr/ogrgeometry.cpp#L6036

I think your example is probably showing something different, though I'm not sure what. I don't know why GEOS is throwing a range_error in that case or why there would be an error in its destructor. I'd be interested to know if it happens only with certain data inputs, or if it can be reproduced outside of GDAL (for example, with the geosop commandline). If you want to tag me on a gdalraster issue I can try to help figure it out.

[ctoney]

Thanks! I'll try to narrow it down with regard to certain inputs and follow up on a gdalraster issue.

[abellgithub]

Is anyone working on this?

[dbaston]

Not currently. It's included in a funding proposal I submitted earlier this month.