Fixing a few valgrind problems

[chaircrusher]

This came up in a 'hackathon' yesterday for Slicer 4 (http://www.slicer.org).

Bill Lorensen saw some runtime errors on program exit and tracked them down with valgrind. It can be applied with "git am"

This is a patch to fix the problem. It is relative to an earlier revision, but I verified that it applies cleanly to the current head revision.

Code: Select all

From af9551b63b6c7db360a7c75320daca8d25bfa039 Mon Sep 17 00:00:00 2001
From: Bill Lorensen <bill.lorensen@gmail.com>
Date: Thu, 6 Dec 2012 08:04:48 -0500
Subject: [PATCH] BUG: Deleting memory more than once in destructor

After deleting memory for a static variable, the code failed to set
the memory container to NULL.

The bug caused crashes on exit when running Slicer built with ITKv4
and DCMTK enabled within ITKv4. With CTK and ITKv4 both using DCMTK,
the destructors were trying to delete the same memory twice. This
caused Slicer to crash on exit. It also caused some Slicer CLI modules
to crash during the module discovery process.

The double delete's were uncovered using valgrind.

NOTE: applied against ab844899a92f46e2d880c38c85ce098933533aef
---
 ofstd/libsrc/oflist.cc   |    5 ++++-
 ofstd/libsrc/ofthread.cc |   25 +++++++++++++++++++------
 2 files changed, 23 insertions(+), 7 deletions(-)

diff --git a/ofstd/libsrc/oflist.cc b/ofstd/libsrc/oflist.cc
index 80a35eb..51d5779 100644
--- a/ofstd/libsrc/oflist.cc
+++ b/ofstd/libsrc/oflist.cc
@@ -51,7 +51,10 @@ OFListBase::~OFListBase()
 {
     base_clear();
     if (afterLast)
-        delete afterLast;
+      {
+      delete afterLast;
+      afterLast = NULL;
+      }
 }
 
 OFListLinkBase * OFListBase::base_insert(OFListLinkBase * pos,
diff --git a/ofstd/libsrc/ofthread.cc b/ofstd/libsrc/ofthread.cc
index 9726176..ff18b3d 100644
--- a/ofstd/libsrc/ofthread.cc
+++ b/ofstd/libsrc/ofthread.cc
@@ -310,6 +310,7 @@ OFThreadSpecificData::~OFThreadSpecificData()
   if (theKey) TlsFree(* OFthread_cast(DWORD *, theKey));
 #elif defined(POSIX_INTERFACE)
   delete OFthread_cast(pthread_key_t *, theKey);
+  theKey = NULL;
 #elif defined(SOLARIS_INTERFACE)
   delete OFthread_cast(thread_key_t *, theKey);
 #else
@@ -450,8 +451,12 @@ OFSemaphore::~OFSemaphore()
 #ifdef WINDOWS_INTERFACE
   CloseHandle((HANDLE)theSemaphore);
 #elif defined(POSIX_INTERFACE)
-  if (theSemaphore) sem_destroy(OFthread_cast(sem_t *, theSemaphore));
-  delete OFthread_cast(sem_t *, theSemaphore);
+  if (theSemaphore)
+    {
+    sem_destroy(OFthread_cast(sem_t *, theSemaphore));
+    delete OFthread_cast(sem_t *, theSemaphore);
+    theSemaphore = NULL;
+    }
 #elif defined(SOLARIS_INTERFACE)
   if (theSemaphore) sema_destroy(OFthread_cast(sema_t *, theSemaphore));
   delete OFthread_cast(sema_t *, theSemaphore);
@@ -589,8 +594,12 @@ OFMutex::~OFMutex()
 #ifdef WINDOWS_INTERFACE
   CloseHandle((HANDLE)theMutex);
 #elif defined(POSIX_INTERFACE)
-  if (theMutex) pthread_mutex_destroy(OFthread_cast(pthread_mutex_t *, theMutex));
-  delete OFthread_cast(pthread_mutex_t *, theMutex);
+  if (theMutex)
+    {
+    pthread_mutex_destroy(OFthread_cast(pthread_mutex_t *, theMutex));
+    delete OFthread_cast(pthread_mutex_t *, theMutex);
+    theMutex = NULL;
+    }
 #elif defined(SOLARIS_INTERFACE)
   if (theMutex) mutex_destroy(OFthread_cast(mutex_t *, theMutex));
   delete OFthread_cast(mutex_t *, theMutex);
@@ -741,8 +750,12 @@ OFReadWriteLock::~OFReadWriteLock()
 #if defined(WINDOWS_INTERFACE) || defined(POSIX_INTERFACE_WITHOUT_RWLOCK)
   delete OFthread_cast(OFReadWriteLockHelper *, theLock);
 #elif defined(POSIX_INTERFACE)
-  if (theLock) pthread_rwlock_destroy(OFthread_cast(pthread_rwlock_t *, theLock));
-  delete OFthread_cast(pthread_rwlock_t *, theLock);
+  if (theLock)
+    {
+    pthread_rwlock_destroy(OFthread_cast(pthread_rwlock_t *, theLock));
+    delete OFthread_cast(pthread_rwlock_t *, theLock);
+    theLock = NULL;
+    }
 #elif defined(SOLARIS_INTERFACE)
   if (theLock) rwlock_destroy(OFthread_cast(rwlock_t *, theLock));
   delete OFthread_cast(rwlock_t *, theLock);
-- 
1.7.4.1

[chaircrusher]

If you look at this patch, you may wonder why guarding is necessary against double-frees. It has to do with DCMTK libraries being loaded shared by other shared libraries, and global static object destructors are getting called by different finalize code for each shared object library that depends on DCMTK libraries.

[Uli Schlachter]

If you look at this patch, you may wonder why guarding is necessary against double-frees. It has to do with DCMTK libraries being loaded shared by other shared libraries, and global static object destructors are getting called by different finalize code for each shared object library that depends on DCMTK libraries.

Even when a shared library gets loaded via multiple other shared libraries, this should not cause global destructors to run twice. Something must be wrong on your end.

Anyway, here is the equivalent of your patch, now in our Git repository:
http://git.dcmtk.org/web?p=dcmtk.git;a= ... c904aba3f9

Thanks for the report!