Little memory leak in file_base_crtp #63

Open
freekvw opened this Issue Mar 1, 2013 · 2 comments

Comments

Projects
None yet
3 participants
@freekvw
Collaborator

freekvw commented Mar 1, 2013

In file_base_crtp.inl, line 33, fileAccessor = new default_file_accessor() isn't deleted, I guess because we'd need to remember if it was user-specified or not.
Not a big thing, just showed up in valgrind.

@Mortal

This comment has been minimized.

Show comment Hide comment
@Mortal

Mortal Mar 4, 2013

Collaborator

I tried the following patch, but it gives me all kinds of double free/corruption errors from libc. Any ideas?

diff --git a/tpie/file_base_crtp.h b/tpie/file_base_crtp.h
index 9274d87..16f9356 100644
--- a/tpie/file_base_crtp.h
+++ b/tpie/file_base_crtp.h
@@ -296,6 +296,7 @@ protected:
        bool m_canWrite;
        bool m_open;
        memory_size_type m_itemSize;
+       tpie::auto_ptr<file_accessor::file_accessor> m_ownedFileAccessor;
        file_accessor::file_accessor * m_fileAccessor;
        tpie::auto_ptr<temp_file> m_ownedTempFile;
        temp_file * m_tempFile;
diff --git a/tpie/file_base_crtp.inl b/tpie/file_base_crtp.inl
index 570c52d..8a3cdd3 100644
--- a/tpie/file_base_crtp.inl
+++ b/tpie/file_base_crtp.inl
@@ -29,8 +29,10 @@ file_base_crtp<child_t>::file_base_crtp(
        m_size = 0;
        m_itemSize = itemSize;
        m_open = false;
-       if (fileAccessor == 0)
-               fileAccessor = new default_file_accessor();
+       if (fileAccessor == 0) {
+               fileAccessor = tpie_new<default_file_accessor>();
+               m_ownedFileAccessor.reset(fileAccessor);
+       }
        m_fileAccessor = fileAccessor;

        m_blockSize = block_size(blockFactor);
Collaborator

Mortal commented Mar 4, 2013

I tried the following patch, but it gives me all kinds of double free/corruption errors from libc. Any ideas?

diff --git a/tpie/file_base_crtp.h b/tpie/file_base_crtp.h
index 9274d87..16f9356 100644
--- a/tpie/file_base_crtp.h
+++ b/tpie/file_base_crtp.h
@@ -296,6 +296,7 @@ protected:
        bool m_canWrite;
        bool m_open;
        memory_size_type m_itemSize;
+       tpie::auto_ptr<file_accessor::file_accessor> m_ownedFileAccessor;
        file_accessor::file_accessor * m_fileAccessor;
        tpie::auto_ptr<temp_file> m_ownedTempFile;
        temp_file * m_tempFile;
diff --git a/tpie/file_base_crtp.inl b/tpie/file_base_crtp.inl
index 570c52d..8a3cdd3 100644
--- a/tpie/file_base_crtp.inl
+++ b/tpie/file_base_crtp.inl
@@ -29,8 +29,10 @@ file_base_crtp<child_t>::file_base_crtp(
        m_size = 0;
        m_itemSize = itemSize;
        m_open = false;
-       if (fileAccessor == 0)
-               fileAccessor = new default_file_accessor();
+       if (fileAccessor == 0) {
+               fileAccessor = tpie_new<default_file_accessor>();
+               m_ownedFileAccessor.reset(fileAccessor);
+       }
        m_fileAccessor = fileAccessor;

        m_blockSize = block_size(blockFactor);
@svendcsvendsen

This comment has been minimized.

Show comment Hide comment
@svendcsvendsen

svendcsvendsen Mar 10, 2015

Collaborator

Currently the file_base class deletes the object pointed to. Removing this statement and also adding a swap statement in file_stream_base::swap seems to fix the errors. This, however, probably introduces more memory leaks. I'm honestly not quite sure how to tackle this problem.

diff --git a/tpie/file_base.cpp b/tpie/file_base.cpp
index aecbd2b..4359668 100644
--- a/tpie/file_base.cpp
+++ b/tpie/file_base.cpp
@@ -122,7 +122,6 @@ void file_base::close() {
 file_base::~file_base() {
    assert(m_free.empty());
    assert(m_used.empty());
-   delete m_fileAccessor;
 }

 /*************************> file_base::stream <*******************************/
diff --git a/tpie/file_base_crtp.h b/tpie/file_base_crtp.h
index 599a9d3..03f7f35 100644
--- a/tpie/file_base_crtp.h
+++ b/tpie/file_base_crtp.h
@@ -303,6 +303,7 @@ protected:
    bool m_canWrite;
    bool m_open;
    memory_size_type m_itemSize;
+   tpie::auto_ptr<file_accessor::file_accessor> m_ownedFileAccessor;
    file_accessor::file_accessor * m_fileAccessor;
    tpie::auto_ptr<temp_file> m_ownedTempFile;
    temp_file * m_tempFile;
diff --git a/tpie/file_base_crtp.inl b/tpie/file_base_crtp.inl
index a131b20..57df8ee 100644
--- a/tpie/file_base_crtp.inl
+++ b/tpie/file_base_crtp.inl
@@ -31,8 +31,10 @@ file_base_crtp<child_t>::file_base_crtp(
    m_canRead = false;
    m_canWrite = false;
    m_open = false;
-   if (fileAccessor == 0)
-       fileAccessor = new default_file_accessor();
+   if (fileAccessor == 0) {
+       fileAccessor = tpie_new<default_file_accessor>();
+       m_ownedFileAccessor.reset(fileAccessor);
+   }
    m_fileAccessor = fileAccessor;

    m_blockSize = block_size(blockFactor);
diff --git a/tpie/file_stream_base.h b/tpie/file_stream_base.h
index bab0f92..677d1dd 100644
--- a/tpie/file_stream_base.h
+++ b/tpie/file_stream_base.h
@@ -99,6 +99,7 @@ protected:
        swap(m_canWrite,        other.m_canWrite);
        swap(m_itemSize,        other.m_itemSize);
        swap(m_open,            other.m_open);
+       swap(m_ownedFileAccessor, other.m_ownedFileAccessor);
        swap(m_fileAccessor,    other.m_fileAccessor);
        swap(m_block.size,      other.m_block.size);
        swap(m_block.number,    other.m_block.number);
Collaborator

svendcsvendsen commented Mar 10, 2015

Currently the file_base class deletes the object pointed to. Removing this statement and also adding a swap statement in file_stream_base::swap seems to fix the errors. This, however, probably introduces more memory leaks. I'm honestly not quite sure how to tackle this problem.

diff --git a/tpie/file_base.cpp b/tpie/file_base.cpp
index aecbd2b..4359668 100644
--- a/tpie/file_base.cpp
+++ b/tpie/file_base.cpp
@@ -122,7 +122,6 @@ void file_base::close() {
 file_base::~file_base() {
    assert(m_free.empty());
    assert(m_used.empty());
-   delete m_fileAccessor;
 }

 /*************************> file_base::stream <*******************************/
diff --git a/tpie/file_base_crtp.h b/tpie/file_base_crtp.h
index 599a9d3..03f7f35 100644
--- a/tpie/file_base_crtp.h
+++ b/tpie/file_base_crtp.h
@@ -303,6 +303,7 @@ protected:
    bool m_canWrite;
    bool m_open;
    memory_size_type m_itemSize;
+   tpie::auto_ptr<file_accessor::file_accessor> m_ownedFileAccessor;
    file_accessor::file_accessor * m_fileAccessor;
    tpie::auto_ptr<temp_file> m_ownedTempFile;
    temp_file * m_tempFile;
diff --git a/tpie/file_base_crtp.inl b/tpie/file_base_crtp.inl
index a131b20..57df8ee 100644
--- a/tpie/file_base_crtp.inl
+++ b/tpie/file_base_crtp.inl
@@ -31,8 +31,10 @@ file_base_crtp<child_t>::file_base_crtp(
    m_canRead = false;
    m_canWrite = false;
    m_open = false;
-   if (fileAccessor == 0)
-       fileAccessor = new default_file_accessor();
+   if (fileAccessor == 0) {
+       fileAccessor = tpie_new<default_file_accessor>();
+       m_ownedFileAccessor.reset(fileAccessor);
+   }
    m_fileAccessor = fileAccessor;

    m_blockSize = block_size(blockFactor);
diff --git a/tpie/file_stream_base.h b/tpie/file_stream_base.h
index bab0f92..677d1dd 100644
--- a/tpie/file_stream_base.h
+++ b/tpie/file_stream_base.h
@@ -99,6 +99,7 @@ protected:
        swap(m_canWrite,        other.m_canWrite);
        swap(m_itemSize,        other.m_itemSize);
        swap(m_open,            other.m_open);
+       swap(m_ownedFileAccessor, other.m_ownedFileAccessor);
        swap(m_fileAccessor,    other.m_fileAccessor);
        swap(m_block.size,      other.m_block.size);
        swap(m_block.number,    other.m_block.number);
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment