[geos-devel] Bug #81 - Memory access violation in LineString::getStartPoint()

[Mateusz Łoskot]

Hi,

Forgive me that I may post redundant messages to the list but I think
this one is very important.
Below I pasted Bug Report #81 I've just submitted:
http://postgis.refractions.net/bugs/bug.php?op=show&bugid=81&pos=4

I'd be very glad if we could discuss this report in details,
verify if this is really a bug, but no my mistake and then
let's try to take some decisions about GEOS design in relation to JTS
that would help GEOS get stable and mature C++ code.

Please, forgive me every hard word I give but this bug made me
really bad ;-)))

OK, let's start the show.

Today, when I was woring on Unit Tests I found a pretty serious bug
causing memory access violation error. Here is the story:

// Factory object preparation
const int srid = 1;
geos::geom::PrecisionModel pm(1.0);
geos::geom::GeometryFactory factory(&pm, srid),

// Use factory to create empty LinearRing
LinearRingPtr ring = factory.createLinearRing();

// ring is empty, but let's see what will happen
PointPtr x = ring->getStartPoint(); <--- BUM!

The line marked with BUM! causes memory access violation error.

Here is descriptive backtrace with complete call chain (lines marked
with <--- are executed during step-by-step debugging):

1.

PointPtr x = ring->getStartPoint(); <---

1.1.

Point* LineString::getStartPoint() const {
   if (isEmpty()) {
      return new Point(NULL, NULL); // <----
   }
   return getPointN(0);
}

2. Point(NULL, NULL); <---

Parameters:
newCoord = NULL
factory = NULL

2.1.

Point::Point(CoordinateSequence *newCoords, const GeometryFactory *factory)
: Geometry(factory) <---
{
//...
}

3.

Geometry(factory)

factory = NULL

3.1.

Geometry::Geometry(const GeometryFactory* newFactory)
{
   factory=newFactory; <--- [1]
   SRID=factory->getSRID(); <--- [2] BUUUUM!
   envelope=NULL;
   userData=NULL;
}

[1]
newFactory = NULL, so factory gets becomes null pointer

[2] BUUUM!
factory is a null pointer and calling member on a null pointer value
causes undefined behaviour!

SRID=factory->getSRID();

4. Finally, GeometryFactory::getSRID() throws
   MEMORY ACCESS VIOLATION ERROR!


Summary:
- the Geometry constructor is used incorrectly
- or this constructor is incorrectly designed to handle all valid cases
- This is another example of ***JAVISMS***.

JTS version of getStartPoint is defined as follows:

public Point getStartPoint() {
   if (isEmpty()) {
      return null; // <--- [3]
   }
   return getPointN(0);
}

[3] returning null reference is completely different semantic of
returning pointer to newly allocated object, what happens in GEOS'
version of getStartPoint!!!

GEOS' version of getStartPoint is buggy in two ways:
1. Causes memory access violation
2. There is logical bug: how can user expect to get Point object
returned from *empty* LinearRing??? Such behaviour does not make sense.
It's also, what is most important, incompatible behaviour with that in
JTS version!

Cheers
-- 
Mateusz Łoskot
http://mateusz.loskot.net