Memory Leak?

[bcthund]

I have an application I have been working on for some time, and just started cleaning up the memory management when I realised my application was taking up nearly 700mb of memory. The app and all it's resources combined are only 95mb. Through a lot of work I eventually tracked down the culprit to specifically the line "myFile->read(memBlock->buffer, memBlock->size);" in the function below. If I comment out this line it reduces my memory consumption by nearly 500mb:

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

inline MemBlock readFile(std::string someFile) {
  MemBlock * memBlock;
  memBlock = new MemBlock;

  std::ifstream * myFile = new std::ifstream;
  myFile->open(someFile.c_str(), std::ios::in|std::ios::binary|std::ios::ate);
  if (myFile->is_open()) {
    memBlock->size=myFile->tellg();
    memBlock->buffer = new char[memBlock->size];
    myFile->seekg(0, std::ios::beg);
    myFile->read(memBlock->buffer, memBlock->size);
    myFile->close();
    delete myFile;
    return *memBlock;
  }
  else {
    printf("Failed to open file.\n");
    return *memBlock;
  }
}

The file that it reads is a 183618 Byte (179.3 KB) file, read all at once. I tried using the code directly and made sure to delete both the memBlock and the myFile. The memBlock->size is correct after reading the file, and the data from the file is correct as well. Any thoughts or suggestions would be greatly appreciated.

[jsmith]

memBlock is never being deleted. You create a new one on line 8 but there is no corresponding delete. Line 19 returns a copy of the allocated MemBlock.

[bcthund]

Deleting memBlock causes a segfault, memBlock should be deleted upon destruction of the function as I understand it. Also, I can take the function and put it directly into my code not as a function and delete the memBlock when i'm finished using it and still have the same problem. I have also set up the function to pass memBlock by reference into the function and still no difference.

[firedraco]

You are also new'ing an ifstream...for some reason I can't fathom...

Anyway, the reason it segfaults it because you have a design flaw. You either need to return a pointer to the new'd memory and delete it later (when you are done), or use a container to hold it so it gets delete'd when the function ends (like a vector).

[bcthund]

This is how the function used to be when I first had this problem arise, it did not set ifstream with new. The memBlock was declared in an outside function and passed by reference, then deleted when finished with:

Outside function:

MemBlock *memBlock = new MemBlock;
readFile(*loadFile, *memBlock);
  //Run memBlock based routines
delete memBlock;

Old readFile Function

inline void readFile(std::string someFile, MemBlock &memBlock) {
  std::fstream myFile(someFile.c_str(), std::ios::in|std::ios::binary|std::ios::ate);
  if (myFile.is_open()) {
    memBlock.size=myFile.tellg();
    memBlock.buffer = new char[memBlock.size];
    myFile.seekg(0, std::ios::beg);
    myFile.read(memBlock.buffer, memBlock.size);
    myFile.close();
  }
  else {
    printf("Failed to open file.\n");
  }
}

Declaring memBlock with new or without doesn't make a difference. A memory map always shows around 505 MB on the heap regardless of either method. By declaring something with new it shouldn't end up on the heap correct?

[bcthund]

It appears I have fixed this although I still am not sure exactly why it was doing this. I changed the function so that it stores the data into some temporary local variables, I then do a memcpy into the reference memBlock. The application now runs around 170MB instead of 505MB.

inline void readFile(std::string someFile, MemBlock &memBlock) {
	int size;
	std::fstream myFile(someFile.c_str(), std::ios::in|std::ios::binary|std::ios::ate);
	if (myFile.is_open()) {
		size = myFile.tellg();
		char buffer[size];
		myFile.seekg(0, std::ios::beg);
		myFile.read(buffer, size);
		myFile.close();

		memBlock.buffer = new char[size];
		memcpy(memBlock.buffer, buffer, size);
		memBlock.size = size;
	}
	else {
		printf("Failed to open file.\n");
	}
}

[kbw]

That shouldn't compile as size isn't constant.

Don't move the file position around to check the size, use stat() to get the size or fstat() if you have a file handle.

How does memBlock know how big its buffer is?

Pass in the filename by const ref rather than by value.

[jsmith]

@bcthund:

This coding "pattern" is always a memory leak:

SomeType func()
{
    SomeType* t = new SomeType;
    // stuff
    return *t;
}

It returns a copy of what t points to. Then the function destroys t, which is a pointer, which does not implictly "call delete" on the pointer; it simply deallocates the pointer from the stack.

[ne555]

Don't move the file position around to check the size, use stat()

Why? It is not standard, it gives much information and I need an extra object.

[exiledAussie]

This coding "pattern" is always a memory leak:

 SomeType func()
{
    SomeType* t = new SomeType;
    // stuff
    return *t;
}

But return the pointer itself (and delete it later) or wrap your pointer in a std::unique_ptr (or auto_ptr) and you will be fine.

[bcthund]

This coding "pattern" is always a memory leak:

SomeType func()
{
    SomeType* t = new SomeType;
    // stuff
    return *t;
}

I thought that might be the case when using this method in a function, but it still doesn't explain why it behave the same way when passing memBlock into the function by reference. However, in the working code above if I used memBlock.buffer = buffer then I always got garbage. Is this because memBlock.buffer turns out to only be a pointer to buffer and gets destroyed at the end of the function?

[jsmith]

The function no longer leaks memory. It is up to the caller to delete the internal buffer in MemBlock.

[jsmith]

The function no longer leaks memory. It is up to the caller to delete the internal buffer in MemBlock.