Possible memory leaks caused by LineString constructor

[mateusz@…]

There is a high possibility that constructor of LineString class will cause memory leaks.

Here is small snipped with current version of this ctor:

LineString::LineString(CoordinateSequence* newCoords, const GeometryFactory *factory)
   : Geometry(factory)
{
   if (newCoords==NULL) {
      points=factory->getCoordinateSequenceFactory()->create(NULL);
      return;
   }

   if (newCoords->getSize()==1) {
      throw util::IllegalArgumentException("point array must contain 0 or >1 elementsn");
   }

   // 'points' WILL LEAK WHEN EXCEPTION ABOVE IS THROWN
   points=newCoords; // <--- [1]
}

Now, the small piece of code with how I create LineString object in one of my test.
This test tries to create one-point linestring intentionaly.

void test()
{
   // Single-element sequence of coordiantes
   CoordinateArraySequence* pseq = new CoordinateArraySequence();
   pseq->add(geos::geom::Coordinate(0, 0, 0));

   try
   {
      // Create incomplete linstring
      // 'ls' IS ASSUMED TO TAKE OWNERSHIP OF 'pseq' SEQUENCE
      geos::geom::LineString ls(pseq, &amp;factory_); // <--- [2]

      // WE EXPECT THE EXCEPTION
   }
   catch (geos::util::IllegalArgumentException const&amp; e)
   {
      // [3] <--- BUM! BUM! BUM!

      // [4] <--- REQUIRED TO AVOID BUM!
      delete pseq;

      std::cout << e.what() << std::endl;
   }
}


Now, the explanation:

1. New coordiante sequence is created with only one point
2. New linestringis created with one-point sequence that is expected to be invalid
3. LineString ctor recognizes this sequence as invalid and throws IllegalArgumentException
4. At this moment, the line [1] is not called, so the sequence previously created does not become a part of LineString object and LineString::points member is left as null
5. When exception is thrown in line [3], the LineString destructor is called but 'points' is null, so the sequence is not deallocated (BUM!)
6. Finally, sequnce 'pseq' leaks
7. To avoid the memory leaks, temporary solution is to deallocate it on the client side, when exception is catched, in line [4].

The main problem is that LineString ctor does not promises (prepares LineString object well) that all resources LineString is responsible for will be deallocated by dctor in case of problems.

The simplest possible solution is to move line [1]:
points=newCoords; // <--- [1]

to the beginning of the LineString::LineString().
This won't be a problem if newCoords is null pointer, because further checks will do the job well.
If newCoords is not a null pointer, then the sequence will be assigned to points member and in case of exception,
LineString destructor will deallocated it properly.