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

[strk at refractions.net]

As said in previous mail, I'd drop createLinearRing().
Every intention to create an EMPTY geometry should be
expressed using the function I added for this purpose:
createEmptyGeometry().
This should reduce error possibilities.

--strk;

On Thu, Mar 30, 2006 at 05:25:38PM +0200, Mateusz Å?oskot wrote:
> 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
> _______________________________________________
> geos-devel mailing list
> geos-devel at geos.refractions.net
> http://geos.refractions.net/mailman/listinfo/geos-devel

-- 

 /"\    ASCII Ribbon Campaign
 \ /    Respect for low technology.
  X     Keep e-mail messages readable by any computer system.
 / \    Keep it ASCII.