Opened 14 years ago

Closed 13 years ago

#367 closed defect (fixed)

Segmentation fault in distance() method.

Reported by: yabo Owned by: strk
Priority: blocker Milestone: 3.2.3
Component: Core Version: main
Severity: Critical Keywords:
Cc:

Description

I have a segmentation fault in Geometry::distance between these geometries (obviously the first one is the problem):

g1: 01060000000100000001030000000100000000000000
g2: 010100000000000000000000000000000000000000

g1 is a MULTIPOLYGON((EMPTY)) (this is what st_astext pretends).

The segmentation fault is produced here :

GeometryLocation (this=0x605bc0, newComponent=0x6048a0, newSegIndex=0, newPt=...) at GeometryLocation.cpp:37
pt = newPt;

This is because newPt is 0x0 (NULL). The derefencing of the NULL pointer occured here :

geos::operation::distance::ConnectedElementLocationFilter::filter_ro (this=0x7fffffffdcc0, geom=0x6048a0) at ConnectedElementLocationFilter.cpp:56
locations->push_back(new GeometryLocation(geom, 0, *(geom->getCoordinate())));

The faulty code is the *(geom->getCoordinate()). Indeed, g1 is loaded as an empty polygon. Polygon::getCoordinate() returns shell->getCoordinate() which in turns returns NULL if the shell is empty.

Note that this is not coherent with GeometryCollection::getCoordinate() which returns 'new Coordinate()' if empty.

Actually I find the GeometryCollection solution quite ugly. More than that, it leaks memory :( (and there is a FIXME from strk by the way).

I don't know what the best fix could be. Return 0 in distance ? Throw an exception ?

This is all with geos-trunk and geos-3.2.1.

Attachments (3)

test.cc (896 bytes ) - added by yabo 14 years ago.
minimal test case.
compilation.patch (7.9 KB ) - added by yabo 13 years ago.
Compilation fix.
distance.patch (785 bytes ) - added by yabo 13 years ago.
Segmentation fault fix.

Download all attachments as: .zip

Change History (16)

by yabo, 14 years ago

Attachment: test.cc added

minimal test case.

comment:1 by mloskot, 13 years ago

I added the attached test case to the unit tests r3213 FYI, I can't reproduce the problem with Visual C++ 10.0

comment:2 by strk, 13 years ago

Milestone: 3.3.0
Status: newassigned

I added the actual ->distance call with r3287 and still can't reproduce the crash. yabo, can you ?

comment:3 by strk, 13 years ago

Resolution: fixed
Status: assignedclosed

comment:4 by yabo, 13 years ago

Sorry, didn't see this.

Unfortunately yes I can still reproduce the bug.

I just ran the test.cc with g++ 4.6 and geos 3.2.2 :

Program received signal SIGSEGV, Segmentation fault.
0x00007ffff7af7307 in geos::algorithm::CGAlgorithms::distancePointLine(geos::geom::Coordinate const&, geos::geom::Coordinate const&, geos::geom::Coordinate const&) () from /usr/lib/libgeos-3.2.2.so
(gdb) bt
#0  0x00007ffff7af7307 in geos::algorithm::CGAlgorithms::distancePointLine(geos::geom::Coordinate const&, geos::geom::Coordinate const&, geos::geom::Coordinate const&) () from /usr/lib/libgeos-3.2.2.so
#1  0x00007ffff7b6f3a9 in geos::operation::distance::DistanceOp::computeMinDistance(geos::geom::LineString const*, geos::geom::Point const*, std::vector<geos::operation::distance::GeometryLocation*, std::allocator<geos::operation::distance::GeometryLocation*> >&) () from /usr/lib/libgeos-3.2.2.so
#2  0x00007ffff7b6f5d6 in geos::operation::distance::DistanceOp::computeMinDistanceLinesPoints(std::vector<geos::geom::LineString const*, std::allocator<geos::geom::LineString const*> > const&, std::vector<geos::geom::Point const*, std::allocator<geos::geom::Point const*> > const&, std::vector<geos::operation::distance::GeometryLocation*, std::allocator<geos::operation::distance::GeometryLocation*> >&) () from /usr/lib/libgeos-3.2.2.so
#3  0x00007ffff7b6fe38 in geos::operation::distance::DistanceOp::computeFacetDistance() () from /usr/lib/libgeos-3.2.2.so
#4  0x00007ffff7b7028d in geos::operation::distance::DistanceOp::distance() () from /usr/lib/libgeos-3.2.2.so
#5  0x00007ffff7b703a0 in geos::operation::distance::DistanceOp::distance(geos::geom::Geometry const*, geos::geom::Geometry const*) () from /usr/lib/libgeos-3.2.2.so
#6  0x00007ffff7b0be09 in geos::geom::Geometry::distance(geos::geom::Geometry const*) const () from /usr/lib/libgeos-3.2.2.so
#7  0x00000000004012ab in main ()
(gdb) 

comment:5 by yabo, 13 years ago

Oh, it doesn't SEGV with geos-svn r3275, so fixed I guess.

comment:6 by strk, 13 years ago

Milestone: 3.3.03.2.3
Resolution: fixed
Status: closedreopened

Let's keep a note for 3.2.3 then... Feel like providing a patch for that one ?

comment:7 by yabo, 13 years ago

If it can wait for next Wednesday I'll give it a try.

comment:8 by strk, 13 years ago

Sure, no rush

comment:9 by yabo, 13 years ago

I've worked out a patch, but what is the expected behaviour when computing the distance with an empty geometry ? Should it return 0 ? Throw an exception ? Something else ?

comment:10 by yabo, 13 years ago

Oh I found this in trunk :

DistanceOp.cpp:145

 if ( geom[0] == 0 || geom[1] == 0 )
  throw IllegalArgumentException("null geometries are not supported");

So I guess it answers the question. :)

comment:11 by yabo, 13 years ago

And the next line which is :

 if ( geom[0]->isEmpty() || geom[1]->isEmpty() ) return 0.0;

comment:12 by yabo, 13 years ago

So here are two patches. The first one is multiple compilation fix to make the 3.2.2 tag compile (missing includes, 'size_t' was used instead of 'std::size_t' and a fix for g++ 4.6 which now refuses non-initialised const). The second one fixes the segmentation fault by backporting the fix from trunk.

by yabo, 13 years ago

Attachment: compilation.patch added

Compilation fix.

by yabo, 13 years ago

Attachment: distance.patch added

Segmentation fault fix.

comment:13 by strk, 13 years ago

Resolution: fixed
Status: reopenedclosed

Fixed with r3445.

Note: See TracTickets for help on using tickets.