Debug assertion failure when buiding GEOS under windows with MSVC /MTd flag

[julius6]

Hello,

I use geos as part of a suite of applications, also using gdal spatialite, libpq, etc. Each project is statically linking the MSVC runtime (built with /MTd in debug mode, /MT in release). I currently use MSVC 2015 if that matters. The issue I'm reporting happens only with /MTd in debug mode and GEOS > 3.8.x. Everything was working up to GEOS 3.6.2.

With recent GEOS, on windows the build system is now cmake, so I modified CMakeLists.txt to build with /MTd The build is successful and from cmake logs I see that /MTd is actually used to build both geos_c.dll and geos.dll. Any other exe and dll in the tree is also /MTd, checked with dependency walker. When I try to run an application in debug, I see the error "Debug assertion failed!: acrt_first_block == header".

Doing some research on the internet, it looks like when using /MTd, each dll uses a private heap, and when memory allocated by a dll gets freed by some other dll, the above error pops up. As many suggest, the problem would be solved by switching to /MDd, but that is unfortunately not easy to do, the solution includes many projects, and they must stay /MTd.

To trigger the error, it's enough to run a query against a spatialite dataset, for example through GDAL using GDALDataset::ExecuteSQL like this:

pDataset->ExecuteSQL("select st_isvalid(buildmbr(10,40,20,50,4326)) as valid", NULL, NULL);

Through spatialite, this triggers a call to GEOSGeom_createPolygon_r, in geos_ts.cpp, which is inside geos_c.dll. In the body of the function, a smart pointer is used:

auto vholes = geos::detail::make_unique<std::vector<LinearRing*>>(nholes);

As a side note, GEOSGeom_createPolygon_r is the only geos_c api which makes use of smart pointers. As another note, spatialite calls GEOSGeom_createPolygon_r, but GDAL does not, this is why this issue does not happen when GDAL directly calls into geos_c.

Later in the code, the function calls:

return gf->createPolygon(nshell, vholes.release());

i.e. it releases ownership of the smart pointer and calls this fashion of createPolygon:

Polygon* createPolygon(LinearRing* shell, std::vector<LinearRing*>* holes) const;

More deeply in the call stack this happens

Polygon::Polygon(LinearRing* newShell, std::vector<LinearRing*>* newHoles, const GeometryFactory* newFactory)
{
	...
	delete newHoles; // debug assertion happens here
}

I believe it's because that object was allocated by geos_c.dll and now destroyed by geos.dll.

I have managed to find a workaround in GEOSGeom_createPolygon_r:

// return gf->createPolygon(nshell, vholes.release());
Geometry* p = gf->createPolygon(*nshell, *vholes);
// free vector elements
for (auto& h : *vholes) {
	delete h;
}
return p;

Doing this way, this fashion of createPolygon is called:

Polygon* createPolygon(const LinearRing& shell, const std::vector<LinearRing*>& holes) const;

which does not delete the vector anymore.

Being not so knowledgeable about smart pointers and how geos classes are implemented, I had to add cleanup code (free vector elements). Perhaps it could be solved more elegantly than this..

I wonder if you guys could accept my fix, so to avoid a check and repatch anytime I integrate a new geos.

With many thanks in advance

Regards, Alessandro

[robe]

Which version of CMake are you using? I recall some complaints with older versions of CMake with VS.

[robe]

Also I think it would help if you can provide your suggestion as a patch (diff) to existing code.

[julius6]

Hi, I am using cmake 3.17. About the patch, I'm not sure it helps. The issue is present on multiple geos versions > 3.6.2. There was a lot of refactoring on geos_ts.cpp, if you guys could try to understand the core reason of the problem, that would be great. Mine was just a workaround.

Thanks, Alessandro

[julius6]

As requested, added patch files. CMakeLists.txt.patch is to compile with /MTd. In case you need to reproduce the problem, I'm using cmake -G "NMake Makefiles" -DCMAKE_BUILD_TYPE=Debug <path-to-geos-dir>

Thanks, Alessandro

[julius6]

Hi,

anyone had the chance to look into this issue?

Thanks, Alessandro

[pramsey]

Have a look at this PR, I *think* it will fix your issue. ​https://github.com/libgeos/geos/pull/359

[Paul Ramsey <pramsey@…>]

In a798ab8/git:

Fix memory management quirk for Win32 in CAPI (closes #1050)

[Paul Ramsey <pramsey@…>]

In 5677545/git:

Clean up Win32 memory issue, references #1050

[Paul Ramsey <pramsey@…>]

In a69e00f/git:

Clean up Win32 memory issue, references #1050