views:

105

answers:

4

I've got the following bit of code, which I've narrowed down to be causing a memory leak (that is, in Task Manager, the Private Working Set of memory increases with the same repeated input string). I understand the concepts of heaps and stacks for memory, as well as the general rules for avoiding memory leaks, but something somewhere is still going wrong:

while(!quit){
   char* thebuffer = new char[210]; 
   //checked the function, it isn't creating the leak
   int size = FuncToObtainInputTextFromApp(thebuffer); //stored in thebuffer
   string bufferstring = thebuffer;
   int startlog = bufferstring.find("$");
   int endlog = bufferstring.find("&");
   string str_text="";
   str_text = bufferstring.substr(startlog,endlog-startlog+1);
   String^ str_text_m = gcnew String(str_text_m.c_str());
   //some work done
   delete str_text_m;
   delete [] thebuffer; 
}

The only thing I can think of is it might be the creation of 'string str_text' since it never goes out of scope since it just reloops in the while? If so, how would I resolve that? Defining it outside the while loop wouldn't solve it since it'd also remain in scope then too. Any help would be greatly appreciated.

+1  A: 

I know its recommended to set the freed variable to NULL after deleting it just to prevent any invalid memory reference. May help, may not.

delete [] thebuffer; 
thebuffer = NULL;     // Clear a to prevent using invalid memory reference
Darxval
+3  A: 

You should use scope-bound resource management (also known as RAII), it's good practice in any case. Never allocate memory manually, keep it in an automatically allocated class that will clean up the resource for you in the destructor.

You code might read:

while(!quit)
{
    // completely safe, no leaks possible
    std::vector<char> thebuffer(210);
    int size = FuncToObtainInputTextFromApp(&thebuffer[0]);

    // you never used size, this should be better
    string bufferstring(thebuffer, size);

    // find does not return an int, but a size_t
    std::size_t startlog = bufferstring.find("$");
    std::size_t endlog = bufferstring.find("&");

    // why was this split across two lines?
    // there's also no checks to ensure the above find
    // calls worked, be careful
    string str_text = bufferstring.substr(startlog, endlog - startlog + 1);

    // why copy the string into a String? why not construct 
    // this directly?
    String^ str_text_m = gcnew String(str_text_m.c_str());

    // ...

    // don't really need to do that, I think,
    // it's garbage collected for a reason
    // delete str_text_m; 
}

The point is, you won't get memory leaks if you're ensured your resources are freed by themselves. Maybe the garbage collector is causing your leak detector to mis-fire.

On a side note, your code seems to have lots of unnecessary copying, you might want to rethink how many times you copy the string around. (For example, find "$" and "&" while it's in the vector, and just copy from there into str_text, no need for an intermediate copy.)

GMan
My only side point would be the unchecked buffer usage - but other than that.
Lee
@Lee: What do you mean?
GMan
Well it's a problem in the original code as well - if the amount of data put into the buffer by FuncToObtainInputTextFromApp() is > 210 then it'll overrun. However, in his case it'll be on the heap, so less likely to be in an executable area. Whereas the buffer above would be on the stack instead. Or am I wrong?
Lee
@Lee: No, you're right. I just assume 210 is some maximum enforced by the function, so it's safe. Not our problem, though. :)
GMan
+2  A: 

Are you #using std, so that str_text's type is std::string? Maybe you meant to write -

String^ str_text_m = gcnew String(str_text.c_str());

(and not gcnew String(str_text_m.c_str()) ) ?

Most importantly, allocating a String (or any object) with gcnew is declaring that you will not be delete'ing it explicitly - you leave it up to the garbage collector. Not sure what happens if you do delete it (technically it's not even a pointer. Definitely does not reference anything on the CRT heap, where new/delete have power).

You can probably safely comment str_text_m's deletion. You can expect gradual memory increase (where the gcnew's accumulate) and sudden decreases (where the garbage collection kicks in) in some intervals.

Even better, you can probably reuse str_text_m, along the lines of -

    String^ str_text_m = gcnew String();
    while(!quit){
       ...
       str_text_m = String(str_text.c_str());
       ...
    }
Ofek Shilon
A: 

There is a tool called DevPartner which can catch all memory leaks at runtime. If you have the pdb for your application this will give you the line numbers in your application where all memory leak has been observed.

This is best used for really big applications.

ckv