Possible memory leaks caused by LineString constructor

[mloskot]

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.

[strk@…]

Fixed using your suggested solution

[mateusz@…]

Unfortunately my simple solution does not solve the problem. LineString will still leak when coordinates sequence is of size = 1. This is because when ctor throws exception, destructor is never called, so 'points' sequence is not deallocated.

I'm preparing some deeper explanation with solution proposal. I'll send it on the mailing list first, subject "Exception-safety". So, please, refer to this post.

I changed status of this Bug Report to Reopened + Deferred.

[mateusz@…]

I cleaned this bug from some spam. Looks like we're experiencing frequent spam attacks.

[mateusz@…]

Removed spam from the comment.

[pramsey]

Still an issue?

[strk]

Ever been ? Mat: do you have a testcase exploiting the leak ?

[mloskot]

I believe there is no need for a test case. The code I outline quoted clearly shows there is lack of use of RAII.

[strk]

I'm asking because I did try to create a LineString with a CoordinateSequece of size 1 and valgrind didn't give me the CoordinateSequence as leaking. Instead it gave me some std::string leaks which seems unrelated (the message passed to the exception thrown)

[mloskot]

OK

[strk]

Well, I couldn't handle to reproduce and so I'll take for fixed.