[eigen] Memory Leak When Constructor of User-Defined Type Throws Exception

[Moritz Klammler <moritz@xxxxxxxxxxx>]

Hello List,

unfortunately, the Bugzilla server is not functional (I don't know for
how long this might be) so I could not check whether this issue has
already been reported.

Currently, if the constructor of a (user-defined) type throws an
exception during construction of a dense matrix with dynamic size, the
memory for the storage of the (not created) matrix is leaked.  This is
not a documented [1] limitation for user-defined types so I assume that
it is not intended behavior.  (The page also does not mention that
user-defined types must be default-constructible.  Although, that might
go without saying.)

Attached is a unit test that demonstrates the leak if run through
Valgrind (or some other debugger) as in

    $ ./check.sh ctorleak && valgrind --leak-check=full ./test/ctorleak

and a patch that removes it.

If you are concerned about the run-time overhead for the try-catch
constructs, we could easily make the exception safety optional by
introducing (yet another) preprocessor switch.


Regards

Moritz



References:

[1] http://eigen.tuxfamily.org/dox/TopicCustomizingEigen.html#user_defined_scalars
-- 
OpenPGP:

Public Key:   http://openpgp.klammler.eu
Fingerprint:  80C1 EC79 B554 3D84 0A35 A728 7057 B288 CE61 2235

# HG changeset patch
# User Moritz Klammler <moritz@xxxxxxxxxxx>
# Date 1404622693 -7200
#      Sun Jul 06 06:58:13 2014 +0200
# Node ID 28d75bc35611b28c5021c0456d33b03b425a7cde
# Parent  a7a7cf52fb22ecaec44495a784b834c760c53177
Avoid memory leak when constructor of user-defined type throws exception.

The added check `ctorleak.cpp` demonstrates how the leak can be reproduced.
The test appears to pass but it is leaking the storage of the (not created)
matrix.  I don't know how to make this test fail in the existing test suite but
you can run it through Valgrind (or another debugger) to verify the leak.

    $ ./check.sh ctorleak && valgrind --leak-check=full ./test/ctorleak

This patch fixes this leak by adding some try-catch-delete-rethrow blocks to
`Eigen/src/Core/util/Memory.h`.

diff --git a/Eigen/src/Core/util/Memory.h b/Eigen/src/Core/util/Memory.h
--- a/Eigen/src/Core/util/Memory.h
+++ b/Eigen/src/Core/util/Memory.h
@@ -333,35 +333,44 @@ template<> inline void* conditional_alig
 {
   return std::realloc(ptr, new_size);
 }
 
 /*****************************************************************************
 *** Construction/destruction of array elements                             ***
 *****************************************************************************/
 
-/** \internal Constructs the elements of an array.
-  * The \a size parameter tells on how many objects to call the constructor of T.
-  */
-template<typename T> inline T* construct_elements_of_array(T *ptr, size_t size)
-{
-  for (size_t i=0; i < size; ++i) ::new (ptr + i) T;
-  return ptr;
-}
-
 /** \internal Destructs the elements of an array.
   * The \a size parameters tells on how many objects to call the destructor of T.
   */
 template<typename T> inline void destruct_elements_of_array(T *ptr, size_t size)
 {
   // always destruct an array starting from the end.
   if(ptr)
     while(size) ptr[--size].~T();
 }
 
+/** \internal Constructs the elements of an array.
+  * The \a size parameter tells on how many objects to call the constructor of T.
+  */
+template<typename T> inline T* construct_elements_of_array(T *ptr, size_t size)
+{
+  size_t i;
+  try
+    {
+      for (i = 0; i < size; ++i) ::new (ptr + i) T;
+      return ptr;
+    }
+  catch (...)
+    {
+      destruct_elements_of_array(ptr, i);
+      throw;
+    }
+}
+
 /*****************************************************************************
 *** Implementation of aligned new/delete-like functions                    ***
 *****************************************************************************/
 
 template<typename T>
 EIGEN_ALWAYS_INLINE void check_size_for_overflow(size_t size)
 {
   if(size > size_t(-1) / sizeof(T))
@@ -371,24 +380,40 @@ EIGEN_ALWAYS_INLINE void check_size_for_
 /** \internal Allocates \a size objects of type T. The returned pointer is guaranteed to have 16 bytes alignment.
   * On allocation error, the returned pointer is undefined, but a std::bad_alloc is thrown.
   * The default constructor of T is called.
   */
 template<typename T> inline T* aligned_new(size_t size)
 {
   check_size_for_overflow<T>(size);
   T *result = reinterpret_cast<T*>(aligned_malloc(sizeof(T)*size));
-  return construct_elements_of_array(result, size);
+  try
+    {
+      return construct_elements_of_array(result, size);
+    }
+  catch (...)
+    {
+      aligned_free(result);
+      throw;
+    }
 }
 
 template<typename T, bool Align> inline T* conditional_aligned_new(size_t size)
 {
   check_size_for_overflow<T>(size);
   T *result = reinterpret_cast<T*>(conditional_aligned_malloc<Align>(sizeof(T)*size));
-  return construct_elements_of_array(result, size);
+  try
+    {
+      return construct_elements_of_array(result, size);
+    }
+  catch (...)
+    {
+      conditional_aligned_free<Align>(result);
+      throw;
+    }
 }
 
 /** \internal Deletes objects constructed with aligned_new
   * The \a size parameters tells on how many objects to call the destructor of T.
   */
 template<typename T> inline void aligned_delete(T *ptr, size_t size)
 {
   destruct_elements_of_array<T>(ptr, size);
@@ -407,39 +432,69 @@ template<typename T, bool Align> inline 
 template<typename T, bool Align> inline T* conditional_aligned_realloc_new(T* pts, size_t new_size, size_t old_size)
 {
   check_size_for_overflow<T>(new_size);
   check_size_for_overflow<T>(old_size);
   if(new_size < old_size)
     destruct_elements_of_array(pts+new_size, old_size-new_size);
   T *result = reinterpret_cast<T*>(conditional_aligned_realloc<Align>(reinterpret_cast<void*>(pts), sizeof(T)*new_size, sizeof(T)*old_size));
   if(new_size > old_size)
-    construct_elements_of_array(result+old_size, new_size-old_size);
+    {
+      try
+        {
+          construct_elements_of_array(result+old_size, new_size-old_size);
+        }
+      catch (...)
+        {
+          conditional_aligned_free<Align>(result);
+          throw;
+        }
+    }
   return result;
 }
 
 
 template<typename T, bool Align> inline T* conditional_aligned_new_auto(size_t size)
 {
   check_size_for_overflow<T>(size);
   T *result = reinterpret_cast<T*>(conditional_aligned_malloc<Align>(sizeof(T)*size));
   if(NumTraits<T>::RequireInitialization)
-    construct_elements_of_array(result, size);
+    {
+      try
+        {
+          construct_elements_of_array(result, size);
+        }
+      catch (...)
+        {
+          conditional_aligned_free<Align>(result);
+          throw;
+        }
+    }
   return result;
 }
 
 template<typename T, bool Align> inline T* conditional_aligned_realloc_new_auto(T* pts, size_t new_size, size_t old_size)
 {
   check_size_for_overflow<T>(new_size);
   check_size_for_overflow<T>(old_size);
   if(NumTraits<T>::RequireInitialization && (new_size < old_size))
     destruct_elements_of_array(pts+new_size, old_size-new_size);
   T *result = reinterpret_cast<T*>(conditional_aligned_realloc<Align>(reinterpret_cast<void*>(pts), sizeof(T)*new_size, sizeof(T)*old_size));
   if(NumTraits<T>::RequireInitialization && (new_size > old_size))
-    construct_elements_of_array(result+old_size, new_size-old_size);
+    {
+      try
+        {
+          construct_elements_of_array(result+old_size, new_size-old_size);
+        }
+      catch (...)
+        {
+          conditional_aligned_free<Align>(result);
+          throw;
+        }
+    }
   return result;
 }
 
 template<typename T, bool Align> inline void conditional_aligned_delete_auto(T *ptr, size_t size)
 {
   if(NumTraits<T>::RequireInitialization)
     destruct_elements_of_array<T>(ptr, size);
   conditional_aligned_free<Align>(ptr);
diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt
--- a/test/CMakeLists.txt
+++ b/test/CMakeLists.txt
@@ -246,16 +246,18 @@ ei_add_test(rvalue_types)
 ei_add_test(dense_storage)
 
 ei_add_test(simplicial_cholesky)
 ei_add_test(conjugate_gradient)
 ei_add_test(bicgstab)
 ei_add_test(sparselu)
 ei_add_test(sparseqr)
 
+ei_add_test(ctorleak)
+
 # ei_add_test(denseLM)
 
 if(QT4_FOUND)
   ei_add_test(qtvector "" "${QT_QTCORE_LIBRARY}")
 endif(QT4_FOUND)
 
 if(UMFPACK_FOUND)
   ei_add_test(umfpack_support "" "${UMFPACK_ALL_LIBS}")
diff --git a/test/ctorleak.cpp b/test/ctorleak.cpp
new file mode 100644
--- /dev/null
+++ b/test/ctorleak.cpp
@@ -0,0 +1,43 @@
+#include "main.h"
+
+#include <exception>  // std::exception
+
+struct Foo
+{
+  int dummy;
+  Foo() { if (!internal::random(0, 10)) throw Foo::Fail(); }
+  class Fail : public std::exception {};
+};
+
+namespace Eigen
+{
+  template<>
+  struct NumTraits<Foo>
+  {
+    typedef double Real;
+    typedef double NonInteger;
+    typedef double Nested;
+    enum
+      {
+        IsComplex             =  0,
+        IsInteger             =  1,
+        ReadCost              = -1,
+        AddCost               = -1,
+        MulCost               = -1,
+        IsSigned              =  1,
+        RequireInitialization =  1
+      };
+    static inline Real epsilon() { return 1.0; }
+    static inline Real dummy_epsilon() { return 0.0; }
+  };
+}
+
+void test_ctorleak()
+{
+  try
+    {
+      Matrix<Foo, Dynamic, Dynamic> m(14, 92);
+      eigen_assert(false);  // not reached
+    }
+  catch (const Foo::Fail&) { /* ignore */ }
+}

Attachment: pgp_1lmTK2b99.pgp
Description: PGP signature