This bugzilla service is closed. All entries have been migrated to https://gitlab.com/libeigen/eigen
Bug 1530 - potential memory leak in evalSubExprsIfNeeded for non-POD elements
Summary: potential memory leak in evalSubExprsIfNeeded for non-POD elements
Status: CONFIRMED
Alias: None
Product: Eigen
Classification: Unclassified
Component: Tensor (show other bugs)
Version: 3.3 (current stable)
Hardware: All All
: Normal Unknown
Assignee: Nobody
URL:
Whiteboard:
Keywords: test-needed
Depends on:
Blocks:
 
Reported: 2018-03-23 19:55 UTC by Weiming Zhao
Modified: 2019-12-04 17:33 UTC (History)
5 users (show)



Attachments
patch for fix (3.12 KB, patch)
2018-03-28 23:40 UTC, Weiming Zhao
no flags Details | Diff
fix 2 (3.12 KB, patch)
2018-03-30 06:43 UTC, Weiming Zhao
no flags Details | Diff
fix 3 (please ignore fix2) (3.95 KB, patch)
2018-03-30 06:47 UTC, Weiming Zhao
no flags Details | Diff

Description Weiming Zhao 2018-03-23 19:55:22 UTC
in evalSubExprsIfNeeded

    if (NumTraits<CoeffReturnType>::RequireInitialization) {
      for (Index i = 0; i < numValues; ++i) {
        new(m_buffer+i) CoeffReturnType();
      }
    }

Those in-place `new` has no corresponding free. In cleanup(), the m_buffer is simply freed as raw memory.

I have the below patch to address it:

+
+  template<class T>
+  typename std::enable_if<std::is_trivially_destructible<T>::value>::type
+  deepclean(T* m_buffer) { }
+
+  template<class T>
+  typename std::enable_if<!std::is_trivially_destructible<T>::value>::type
+  deepclean(T* m_buffer) {
+    const Index numValues =  internal::array_prod(m_impl.dimensions());
+    for(Index i = 0; i < numValues; ++i)
+      m_buffer[i].~T();
+  }
+
   EIGEN_DEVICE_FUNC EIGEN_STRONG_INLINE void cleanup() {
+    deepclean(m_buffer);
     m_device.deallocate(m_buffer);
     m_buffer = NULL;

Does it look correct?
Comment 1 Christoph Hertzberg 2018-03-27 18:00:19 UTC
This should be replaced by calls to construct_elements_of_array and destruct_elements_of_array from src/Core/util/Memory.h

I don't think there is harm in destructing trivially destructable objects:
https://godbolt.org/g/1GEbAx

So we don't need the enable_if construct (if it was required, it should be replaced by internal::enable_if<NumTraits<T>::RequireInitialization>)

Also, this needs a unit-test (see test/ctorleak.cpp) for an example -- maybe extract some common code into a header.
Comment 2 Christoph Hertzberg 2018-03-27 18:48:23 UTC
Other question: Why is the buffer not deleted by the destructor?
Having to call cleanup() manually looks unsafe to me.
Comment 3 Weiming Zhao 2018-03-28 23:39:35 UTC
yes, we can just use ~T() even for primitive types. please see attached patch
Comment 4 Weiming Zhao 2018-03-28 23:40:11 UTC
Created attachment 838 [details]
patch for fix
Comment 5 Christoph Hertzberg 2018-03-29 13:41:26 UTC
As said in comment 1, please use the existing functions in util/Memory.h
And for the unit-test make sure that it actually exposes the issue (i.e., it should fail without the patch)
Comment 6 Weiming Zhao 2018-03-29 17:37:03 UTC
OK. I will try the existing functions.

Regarding the unit test, how the code itself capture the memory leak?

I checked the existing code of test/ctorleak
the commit log says similar:

"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."
Comment 7 Weiming Zhao 2018-03-30 06:43:34 UTC
Created attachment 840 [details]
fix 2
Comment 8 Weiming Zhao 2018-03-30 06:47:51 UTC
Created attachment 841 [details]
fix 3 (please ignore fix2)
Comment 9 Christoph Hertzberg 2018-03-30 10:54:34 UTC
(In reply to Weiming Zhao from comment #6)
> the commit log says similar:
> 
> "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."

That was for the initial version of the test. The current version counts how often the constructor and destructor got called (of course that alone is no guarantee that everything is 100% correct, but it would catch bugs which existed before the test was introduced)
Comment 10 Nobody 2019-12-04 17:33:00 UTC
-- GitLab Migration Automatic Message --

This bug has been migrated to gitlab.com's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.com/libeigen/eigen/issues/1530.

Note You need to log in before you can comment on or make changes to this bug.