Memory leak

[bcthund]

I have been using valgrind to find memory leaks recently and I can't seem to figure out why this binary file loader is causing a memory leak. It's not a serious leak but I would like to fix it:

Valgrind Error:

 ==20878== 32,256 bytes in 7 blocks are definitely lost in loss record 407 of 412
 ==20878==    at 0x402BE68: malloc (in /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so)
 ==20878==    by 0x8083469: readFile(std::string, MemBlock&) (functions.h:271)
 ==20878==    by 0x8068FD5: O2D::load(GameVars&) (O2D.h:73)
 ==20878==    by 0x80829E0: main (TheDestroyer.cpp:451)

readFile Function with apparent memory leak:

inline bool readFile(std::string someFile, MemBlock &memBlock) {
 std::ifstream myFile(someFile.c_str(), std::ifstream::in | std::ifstream::binary);
 if (myFile.is_open()) {
  myFile.seekg(0, std::ifstream::end);
  memBlock.size = myFile.tellg();
  memBlock.buffer = (char*) malloc(memBlock.size*sizeof(char*));
  myFile.seekg(0, std::ifstream::beg);
  myFile.read(memBlock.buffer, memBlock.size);
  myFile.close();

  return true;
 }
 else {
  return false;
  printf("Failed to open file.\n");
 }
}

memBlock is a struct defined elsewhere and passed by reference:

struct MemBlock {
 char * buffer;
 int size;
};

MemBlock *memBlock = (MemBlock*) malloc(sizeof(MemBlock*));
readFile("myFile.bin", *memBlock);

Any suggestions as to what might be causing a leak with this would be helpful, I have been looking all over the Internet trying to figure out how to stop these leaks and have been unable to eliminate them completely. Thanks.

[MiiNiPaa]

when you do MemBlock *memBlock = (MemBlock*) malloc(sizeof(MemBlock*));
buffer contains random data, essentually a random pointer.
On line 6 of readFile you assigning another value to buffer. Maybe Valgrind thiks you losing data it contained before?

And why are you manually allocating your memory?

[MikeyBoy]

Where are you freeing the memory you allocate on line 6 of readFile?

Where are you freeing the memory you allocate in line 6 of your final code snippet?

[MrHutch]

Are you writing this in C or C++?

Most of the code looks like good ol' C, but you're using the bool data type, which C doesn't support, leading me to believe it's compiled as C++.

Reason I ask is because I'm wondering why you'd use malloc and free over new and delete in a C++ application.

[bcthund]

The program is written in C++, I was using new before with the same results and I figured that by using malloc instead of new I was ensuring that I was using the heap and not the stack.

I don't have a free or delete because I was under the impression that because memBlock is declared within a class function that it was automatically destroyed when the function ended.

After adjusting the program to use new and delete I am still getting similar problems, here are the valgrind examples:

==23809== Invalid write of size 4
==23809==    at 0x808346D: readFile(std::string, MemBlock&) (functions.h:270)
==23809==    by 0x805C976: GameSys::load(GameVars&) (System.h:41)
==23809==    by 0x80827CA: main (TheDestroyer.cpp:429)
==23809==  Address 0x76e95f4 is 0 bytes after a block of size 4 alloc'd
==23809==    at 0x402BE68: malloc (in /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so)
==23809==    by 0x805C8DF: GameSys::load(GameVars&) (System.h:35)
==23809==    by 0x80827CA: main (TheDestroyer.cpp:429)
==23809== 
==23809== Invalid read of size 4
==23809==    at 0x8083476: readFile(std::string, MemBlock&) (functions.h:274)
==23809==    by 0x805C976: GameSys::load(GameVars&) (System.h:41)
==23809==    by 0x80827CA: main (TheDestroyer.cpp:429)
==23809==  Address 0x76e95f4 is 0 bytes after a block of size 4 alloc'd
==23809==    at 0x402BE68: malloc (in /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so)
==23809==    by 0x805C8DF: GameSys::load(GameVars&) (System.h:35)
==23809==    by 0x80827CA: main (TheDestroyer.cpp:429)
...
...
...
9,216 bytes in 8 blocks are definitely lost in loss record 398 of 409
==23809==    at 0x402B454: operator new[](unsigned int) (in /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so)
==23809==    by 0x8083480: readFile(std::string, MemBlock&) (functions.h:274)
==23809==    by 0x8068FFF: O2D::load(GameVars&) (O2D.h:73)
==23809==    by 0x80829FA: main (TheDestroyer.cpp:451)

Line 270 is memBlock.size = myFile.tellg(); which is giving and invalid write of size 4.
Line 274 is memBlock.buffer = new char[memBlock.size]; which is giving an invalid read of size 4.

What I changed was:
MemBlock *memBlock = (MemBlock*) malloc(sizeof(MemBlock*));
is now:
MemBlock *memBlock = new MemBlock;

and:
memBlock.buffer = (char*) malloc(memBlock.size*sizeof(char*));
is now:
memBlock.buffer = new char[memBlock.size];

And at the end of the function that declares memBlock I added a delete memBlock; but it doesn't seem to have any impact on the results.

[moorecm]

Use the initializer list on MemBlock to clear buffer and see if that helps.

MemBlock() : size( 0 ), buffer( 0 )    // buffer can be safely deleted
{
}

It's been mentioned already: the problem is that you are just overwriting a pointer unconditionally. (And make sure you use delete[] if you used new[]).

if( memBlock.buffer ) {                // this makes it safe, if you were already using a buffer
    delete[] memBlock.buffer;
}
memBlock.buffer = new char[SOMESIZE];  // make sure you account for a null, if necessary 

Regardless, the best way to solve the problem is to quit reinventing the wheel. How about a std::string or std::vector?

[MrHutch]

bcthund wrote:

I don't have a free or delete because I was under the impression that because memBlock is declared within a class function that it was automatically destroyed when the function ended.

No, that's not true. You still need to explicitly delete.

Be sure to follow moorecm's advice and safely delete before overwriting.

And to delete the data when the class is destroyed, write out the destructor.

SomeClass::~SomeClass()
{
   if( memBlock.buffer )
   {
      delete[] memBlock.buffer;
      memBlock.buffer = nullptr;
   }
}

[bcthund]

Thanks for all the help, after a little bit of tweaking of a couple loops and putting the proper declarations in all my files I have eliminated all of these errors.

The problem was that malloc was not creating memBlock in the proper size in some areas so using MemBlock *memBlock = new MemBlock; fixed some of the problem.

I also added delete memBlock to the end of the function and added the delete[] memBlock.buffer; along with the test before redeclaring the buffer for new data. The other problem was that I had one loop that was using less than or equal to for it's condition when it should have just been equal to:

for (int d=0; d<=memBlock->size; d+=gameVars.recordSize.o3d) {
became:
for (int d=0; d<memBlock->size; d+=gameVars.recordSize.o3d) {

After that the rest of the errors that I was about to ask about went away.