Ticket #738 (closed defect: fixed)

Opened 3 years ago

Last modified 1 year ago

It seems OGR uses reference counting inconsistently, what result in memory leak.

Reported by: mbrudka@aster.pl Assigned to: mloskot
Priority: normal Milestone: 1.4.2
Component: OGR_SRS Version: unspecified
Severity: normal Keywords:
Cc: warmerdam

Description (Last modified by warmerdam)

OGR uses reference counting for some object to manage the life cycle of objects as well as to decrease OGR memory requirements. In particular SpatialReferenceSystems? are reference counted. While reference counting is consistent in OGRProj4CT, namely OGRSpatialReference are deleted when counter falls below 1, in OGRGeometry eg. OGRGeometry::~OGRGeometry and OGRGeometry::assignSpatialReference a counter is released without checking if OGRSpatialReference should be deleted. This may result in memory leak.

SAMPLE FIX/WORKAROUND:

The quick fix is to check if reference counter falls below 1 and eventually delete the object eg.

OGRGeometry::~OGRGeometry()
{
   if( poSRS != NULL )
   {
       if ( poSRS->Dereference() <= 0 )
         delete poSRS;
   }
}

However, a better way to manage reference counted objects is to use consistently in GDAL intrusive smart pointers. If the dependency on boost libraries is accepted, then I propose to use boost::instrusive_ptr (http://www.boost.org/libs/smart_ptr/intrusive_ptr.html). If this dependency is for some reason undesirable, then a simple template (boost::instrusive_ptr is 273 lines long) for such pointers can be written. I strongly recommend such technique, as this is simple way to avoid related with reference counted memory management.

Change History

04/03/07 00:19:43 changed by warmerdam

  • severity changed from critical to normal.
  • cc set to warmerdam.
  • priority changed from high to normal.
  • milestone set to 1.4.2.
  • owner changed from warmerdam to mloskot.
  • description changed.

Matuesz,

Could you review this, and see if you can reproduce a reference counting bug? As you can imagine, I'm still not keen on the boost auto_ptr solution. :-)

04/17/07 22:54:51 changed by mloskot

  • status changed from new to assigned.

This bug report is quite old and as I see the issue has been solved already. The OGRGeometry dctor calls OGRSpatialReference::Release() if poSRS is non-NULL. Next, the Release() function does the proper check:

void OGRSpatialReference::Release()

{
    if( this && Dereference() <= 0 )
        delete this;
}

Certainly, testing if this pointer is NULL is unnecessary here because it's guaranteed that this cannot be null in a well-formed C++ program.

04/17/07 22:56:48 changed by mloskot

  • status changed from assigned to closed.
  • resolution set to fixed.

I believe it's reasonable to close this issue.

04/17/07 22:58:23 changed by mloskot

I only changed the comparison operator to make it a bit safer and removed redundant this != NULL test (r11280)

04/18/07 09:48:47 changed by warmerdam

  • status changed from closed to reopened.
  • resolution deleted.

Mateusz,

Please restore the 'this' test. If a NULL gets passed to a C cover function, then this can be NULL in the method.

Thanks,

04/18/07 10:58:49 changed by mloskot

  • status changed from reopened to closed.
  • resolution set to fixed.

Frank,

It seems I misunderstood the idea. I revoked this change (r11281).

Recent report about broken OGR and failing tests is directly related to the removal of this, but now it works.

04/18/07 15:30:09 changed by mloskot

The final fix has been ported to the stable branch 1.4 (r11282).