Opened 5 weeks ago

Closed 5 weeks ago

Last modified 5 weeks ago

#994 closed defect (fixed)

Segfault in UnaryUnionOp (::buildGeometry, something with C++ strings!)

Reported by: strk Owned by: geos-devel@…
Priority: blocker Milestone: 3.9.0
Component: Default Version: master
Severity: Unassigned Keywords:
Cc:

Description (last modified by strk)

See backtrace:

#0  0x00007f52990da583 in std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::_M_dispose() (this=0x7ffde4bb7940)
    at /usr/include/c++/7/bits/basic_string.h:220
#1  0x00007f52990da583 in std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::~basic_string() (this=0x7ffde4bb7940, __in_chrg=<optimized out>) at /usr/include/c++/7/bits/basic_string.h:647
#2  0x00007f52990da583 in geos::geom::GeometryFactory::buildGeometry<__gnu_cxx::__normal_iterator<geos::geom::Point const**, std::vector<geos::geom::Point const*, std::allocator<geos::geom::Point const*> > > >(__gnu_cxx::__normal_iterator<geos::geom::Point const**, std::vector<geos::geom::Point const*, std::allocator<geos::geom::Point const*> > >, __gnu_cxx::__normal_iterator<geos::geom::Point const**, std::vector<geos::geom::Point const*, std::allocator<geos::geom::Point const*> > >) const (toofar=..., from=..., this=<optimized out>)
    at ../../../include/geos/geom/GeometryFactory.h:391
#3  0x00007f52990da583 in geos::operation::geounion::UnaryUnionOp::Union() (this=0x7ffde4bb7940) at UnaryUnionOp.cpp:85

Attachments (2)

geos_geometry.txt (14.4 KB) - added by Algunenano 5 weeks ago.
Geometry as output by g->toString()
issue-geos-994.xml (15.2 KB) - added by dbaston 5 weeks ago.
failing xml test

Download all attachments as: .zip

Change History (20)

comment:1 Changed 5 weeks ago by strk

Description: modified (diff)

comment:2 Changed 5 weeks ago by strk

Valgrind view of the matter:

==26567== Invalid read of size 8
==26567==    at 0x25936383: geos::operation::geounion::OverlapUnion::unionBuffer(geos::geom::Geometry const*, geos::geom::Geometry const*) (OverlapUnion.cpp:133)
==26567==    by 0x259364F2: geos::operation::geounion::OverlapUnion::unionFull(geos::geom::Geometry const*, geos::geom::Geometry const*) (OverlapUnion.cpp:119)
==26567==    by 0x25936F18: geos::operation::geounion::OverlapUnion::doUnion() (OverlapUnion.cpp:55)
==26567==    by 0x259331AC: geos::operation::geounion::CascadedPolygonUnion::unionActual(geos::geom::Geometry*, geos::geom::Geometry*) (CascadedPolygonUnion.cpp:243)
==26567==    by 0x25933242: geos::operation::geounion::CascadedPolygonUnion::unionSafe(geos::geom::Geometry*, geos::geom::Geometry*) (CascadedPolygonUnion.cpp:234)
==26567==    by 0x25933343: geos::operation::geounion::CascadedPolygonUnion::binaryUnion(geos::operation::geounion::GeometryListHolder*, unsigned long, unsigned long) (CascadedPolygonUnion.cpp:191)
==26567==    by 0x259336DF: geos::operation::geounion::CascadedPolygonUnion::unionTree(geos::index::strtree::ItemsList*) (CascadedPolygonUnion.cpp:168)
==26567==    by 0x2593349B: geos::operation::geounion::CascadedPolygonUnion::reduceToGeometries(geos::index::strtree::ItemsList*) (CascadedPolygonUnion.cpp:205)
==26567==    by 0x259336D1: geos::operation::geounion::CascadedPolygonUnion::unionTree(geos::index::strtree::ItemsList*) (CascadedPolygonUnion.cpp:167)
==26567==    by 0x2593349B: geos::operation::geounion::CascadedPolygonUnion::reduceToGeometries(geos::index::strtree::ItemsList*) (CascadedPolygonUnion.cpp:205)
==26567==    by 0x259336D1: geos::operation::geounion::CascadedPolygonUnion::unionTree(geos::index::strtree::ItemsList*) (CascadedPolygonUnion.cpp:167)
==26567==    by 0x259337FC: geos::operation::geounion::CascadedPolygonUnion::Union() (CascadedPolygonUnion.cpp:156)
==26567==  Address 0x18 is not stack'd, malloc'd or (recently) free'd

comment:3 Changed 5 weeks ago by strk

Can be reproduced with QGIS testsuite: ctest -R 'QgisAlgorithmsTest$' --output-on-failure

(tested in 3.4 stable branch of QGIS, and in master branch of QGIS)

comment:4 Changed 5 weeks ago by strk

Milestone: 3.8.0

comment:5 Changed 5 weeks ago by pramsey

Can you turn it into a unit or XML test, for those of us who don't have a source build of QGIS lying around?

comment:6 Changed 5 weeks ago by Paul Ramsey <pramsey@…>

In b54786f/git:

Random guess at what the problem is
References #994

comment:7 Changed 5 weeks ago by pramsey

Without a reproduction on this, we need to punt it to 3.8.1 so we can have a release.

Last edited 5 weeks ago by pramsey (previous) (diff)

comment:8 Changed 5 weeks ago by robe

you meant punt to 3.9 :)

comment:9 Changed 5 weeks ago by robe

or 3.8.1

comment:10 Changed 5 weeks ago by strk

Milestone: 3.8.03.9.0

Punt it, I get the same segfault with GEOS-3.6 so this must be some issue in either my setup or QGIS...

comment:11 Changed 5 weeks ago by Algunenano

I can reproduce this too with:

"/usr/bin/cmake" "-P" "/home/raul/dev/public/QGIS/build/python/plugins/processing/tests/ProcessingQgisAlgorithmsTest.cmake"

Is there an easy way to access and export the input of GEOSUnaryUnion_r? Would g->toString() be enough?

Changed 5 weeks ago by Algunenano

Attachment: geos_geometry.txt added

Geometry as output by g->toString()

comment:12 Changed 5 weeks ago by Algunenano

I've extracted the geometry and created an unit test that also reproduces the crash in my system. PR with the test: https://github.com/libgeos/geos/pull/237

Changed 5 weeks ago by dbaston

Attachment: issue-geos-994.xml added

failing xml test

comment:13 Changed 5 weeks ago by Daniel Baston <dbaston@…>

Resolution: fixed
Status: newclosed

In 3a9ce54/git:

Avoid accessing pointer after move

Fixes #994

comment:14 Changed 5 weeks ago by dbaston

Raúl, did you use a Docker file or similar to run the QGIS tests against GEOS master? If so, would you mind passing it along? It might well come in handy again.

comment:15 Changed 5 weeks ago by Algunenano

Raúl, did you use a Docker file or similar to run the QGIS tests against GEOS master?

No, I've got GEOS master installed in the system and built QGIS from source. Since it's linked dynamically I've replaced the GEOS package with one that printed the input to a file (https://github.com/Algunenano/geos/commit/a81133bd808e40d3a31bc60bce53a20aa1aec8fe)

comment:16 Changed 5 weeks ago by strk

Wow, good catch ! I'm pretty sure I saw this with GEOS-3.6 too so please consider backporting to wherever it is needed

comment:17 Changed 5 weeks ago by dbaston

OverlapUnion is new to 3.8 and std::move is new to 3.7, so if there is an issue in 3.6 it must be something else.

comment:18 Changed 5 weeks ago by strk

I confirm the fix in master branch. And I confirm my view on 3.6 was wrong, because make install of updated GEOS did not fix the crash at first, but required me to rm -f /usr/local/libgeos* first

I guess it was an effect of the bug fixed with [d47ea8c428da1ed4957916c49268740b362db9fb/git]

Note: See TracTickets for help on using tickets.