Have you looked into tools for detecting memory leaks, such as Valgrind? (Windows substitutes)
Running these over your code will show you how much memory you're leaking and quite possibly where.
This isn't a complete program, so we can't pass it through for you. Some advice:
- Break up routines and have separate test executables so you can ensure bits of your code are free of memory leaks. This is kind of like test driven development - you write a test for a function to ensure it works and validates input correctly, and you test it for memory leaks too.
- Develop in a library like fashion. This ought to be second nature as it optimises code reuse (don't repeat yourself unless repeating yourself is advantageous) and helps with the above.
Edit: I'll expand on that. To give you an idea of when a memory leak happens, consider this:
#include <iostream>
using namespace std;
int main(int argc, char** argv)
{
int* arr = new int(5);
// delete arr; // <-- uncomment this to fix memalloc bug.
return 0;
}
This program very simply allocates 4 bytes on the heap, then exits without tidying up that memory.
Valgrind would tell you:
HEAP SUMMARY:
==23008== in use at exit: 4 bytes in 1 blocks
==23008== total heap usage: 1 allocs, 0 frees, 4 bytes allocated
==23008==
==23008== LEAK SUMMARY:
==23008== definitely lost: 4 bytes in 1 blocks
==23008== indirectly lost: 0 bytes in 0 blocks
==23008== possibly lost: 0 bytes in 0 blocks
==23008== still reachable: 0 bytes in 0 blocks
==23008== suppressed: 0 bytes in 0 blocks
Why? Because you haven't deleted the memory you allocated with new. This is, roughly speaking, as complicated as it gets.
I say complicated as it gets because programs become big codebases rather quickly and memory leaks suddenly get more complicated. Who allocated the memory? Who freed it? Was memory allocated as part of a library? What happens when you start using arrays? You might cause indirect memory loss by freeing the outside level of the array but not the inner. See indirect memory loss.
This quickly becomes very complicated, especially when functions start allocating memory and you start using them elsewhere, and you start relying on programmatically determined memory sizes, such as lengths of strings based on user input. The maxlen+1 error is the common mistake and can make huge leaks in larger 2D arrays of data.
Edit 2 based on your new question:
Firstly, if you're using C++, I strongly suggest forgetting about malloc. Use new and delete because they understand polymorphism and objects, whereas malloc doesn't. Your constructors might not get called correctly. And if you need strings, use the string type. If you need arrays, vector will usually do.
Secondly, you are only freeing the memory if the read operation works correctly. Why? You've still allocated the memory, therefore you still need to free it, regardless of whether the read operation worked. You should also check that the memory was indeed allocated, otherwise you'll get a segmentation fault caused by accessing memory you're not supposed to access.
Edit three:
Ok, based on your comments, consider this code, which is what I think you're proposing:
char* buffer = (char*)malloc(4096*sizeof(char));
if ( ReadToSerialPort(buffer, ...) > 0 ) // you are proposing to free in this func call
{
// do stuff A
}
// do stuff B
// free(buffer); // <-- you should free buffer here
What happens if you free inside the function call? Well, that's actually sort of ok, apart from the fact there's still a variable in the scope of this code that you might accidentally use. If you do, you'll be accessing memory you've explicitly released, which will probably cause your program to crash.
I assume at some stage you want to use this buffer either at part A or B. In which case you need that memory still to be allocated.
As for only freeing inside the if statement, that's a guaranteed memory leak if the read function doesn't work, because that memory never gets freed.
Here's how you should do it:
//
// I maintain you should use a string here, or
// if you insist on char*, use new.
// and delete.
//
char* buffer = (char*)malloc(4096*sizeof(char));
if ( buffer == NULL )
{
// os refused the alloc.
cout << "Out of memory\n" << endl;
return 1;
}
if ( ReadToSerialPort(buffer, ...) > 0 ) // this call frees as well?
{
// do stuff A using buffer
}
// do stuff B using buffer
free(buffer); // <-- you should free buffer here
Edit 4: Just for clarification:
Do not mix malloc/free and new/delete. If you malloc, free that memory, if you new, delete that memory. If you're interfacing with the C code, unless it requires an array of something like int, you can use the string type and there's no need for dynamic allocation. Unless you're passing to a C function you have no control over, you should probably consider vector above an array. Only if you are passing to a C function you have no control over should you need to enter the world of C's malloc and free methods.
