Memory Leaks Out of Control 2

[Kalisto]

Hi, In my code at present I have 500 memory leaks, which is not good (for my code, or my self esteem!)

I am doing a lot of work with one particular class, which is allocating memory, so I believe that this is where the issues are starting.

One of my class member variables is declared as

private:
char* pData;

in my destructor I have the line free ((char*) pData);

WHen this line is in my code it throws up the error Damage at Normal Block (#345). If I comment out the line then the code compiles normally, but I have this memory loss.

The data was allocated (in another function) using malloc or realloc, so I know that free() is the function to use, but what am I doing wrong.

MS memory leak detection is basic, is there a free 3rd party tool I can use to at least track down these leaks to a line / file in my code that is causing them ?

Regards,

K

 

[PerFnurt]

>Hi, In my code at present I have 500 memory leaks, which is not good

Well observed.

Your code is screaming for being unit tested. Ie you should take the class into another, very simple, program (a simple console application would do) that just calls it as your application calls it (this should be done for all classes IMHO, but lets start at where the problems are). That way you can test your code and see in what situations it bugs out. It is also easier to test it as you run just the code youre interested in at the moment, ignoreing the rest of the application.

There are quite clever 3rd party tools for analyzing, for example BoundsChecker.

http://www.compuware.com/products/devpartner/bounds.htm

Btw, whats wrong with new and delete? It is C++, right?


/Per

_{if (typos) cout << &quot;My fingers are faster than my brain. Sorry for the typos.&quot;;}

 

[Kalisto]

It is C++, the pData is a member of the class, and so I can't use new in the .h file when I first set up the private data members.

Bounds Checker looks nice, but I can't get it on 14 day trial, and will be hard pressed to buy more software this year (they got the budget wrong)

K

 

[PerFnurt]

>It is C++, the pData is a member of the class, and so I can't use new in the .h file when I first set up the private data members.

Not sure what you mean, but of course you can (and you should) initialize your members in the class' constructor (for example by using new on members that are pointers).

>will be hard pressed to buy more software

It is a matter of priority. If quality ensurance of your products is considered important, there will be money for it.

>they got the budget wrong

Of course they did.

/Per

_{if (typos) cout << &quot;My fingers are faster than my brain. Sorry for the typos.&quot;;}

 

[Kalisto]

;-) I remember what you do now, I take it you have the same problems with Budget holders where you are!

Thanks, I'll play around with it, I may be being stupid, and now I have had some time away from it all may become clear.

Bp.

 

[Kalisto]

Oh, I didn't explain my problem with teh .h earlier.

in my .h file,

Class foo
{
public:
bar()
private:
char* pData;
}

So I cannot now do

foo:foo()
{
char* pData = new char;
}
as instead it creates a local pData using new.

 

[PerFnurt]

// Two ways to new it.
foo:foo()
{
  pData = new char;
}
foo:foo():pData(new char)
{
}

/Per

_{if (typos) cout << &quot;My fingers are faster than my brain. Sorry for the typos.&quot;;}

 

[Cagliostro]

I guess you should use:
free ((void*) pData);

Ion Filipski

ICQ: 95034075
AIM: IonFilipski
filipski@excite.com

 

[Kalisto]

OK, thanks to both of you, but I still have the error when I try to release the memory, no matter which way I do it.

It needs more investigation I think, along with some looking at the MS reasons for the error messages thrown up.

K

 

[Cagliostro]

I think, you create a copy of variable somewhere, for example as a parameter in the function or as an assignment/initialization, and pointer is copied as is. In this case, the destructor try to free twice or more the same block of memory. Create a copy constructor and an assignment operator, so what each class would create a copy of memory block, not the copy of address of the block.

Ion Filipski

ICQ: 95034075
AIM: IonFilipski
filipski@excite.com

 

[Kalisto]

OK, I'll try that. I don't think that I am creating a copy anywhere, but after the chopping about I have done the last few hours anything is possible.

Thanks

K

 

[PerFnurt]

>I don't think that I am creating a copy anywhere

Easy way to find out: Make the copy constructor and assignment operator private. You'll get a compiple error in those places that tries to copy.

/Per

_{if (typos) cout << &quot;My fingers are faster than my brain. Sorry for the typos.&quot;;}

 

[Cagliostro]

a copy is created automatically in the stack when you pass your object as a parameter to some function. If you do not want a copy to be created, respectively the destructor on it not to be executed, pass a reference instead or a pointer. By the way, if you do not alloc memory under a pointer initialize it always to 0 (NULL). So, if destructor executes on an noninitialized pointer, it will not throw any errors.

Ion Filipski

ICQ: 95034075
AIM: IonFilipski
filipski@excite.com

 

[Kalisto]

I created copy and assignment functions, and put a breakpoint in each.
Neither of them is called.

I now have in my code a simple function that creates a single instance of the tag, initialises it then deletes it.

As soon as it tries to delete it it generates the error.

Anymore ideas ?

 

[Cagliostro]

could you put the code there?

Ion Filipski

ICQ: 95034075
AIM: IonFilipski
filipski@excite.com

 

[Kalisto]

I'll put bits of it (too much all in all)
[part of the .h]

//Length Attributes (Lenght is Implicitly 16 or 32 bit
__int16 length;
__int32 wlength;
byte* data;

[where the data is initialised]

[main program]

DTag* myTag = new DTag;
myTag->operator >>(fs);

delete myTag;

[.cpp file]

DTag::~DTag
{
//delete[] data; //Error with this version of delete too.
delete data; //Point Error Occurs
}

DTag:perator >>(std::istream* iSt)
{

//read in other information
...
__int32 len = 0;

//Get Length no matter whether it is 16 or 32 bit
if (length > 0)
len = length;
else
len = wlength;

//Size array
data = new byte(len); //This is done here as know length, not in c'tor


fs->read((char*)data,len);
}


I Hope that this all makes some kind of sense.

K

 

[PerFnurt]

Youre not mixing malloc<->delete or new<->free are you?



/Per

_{if (typos) cout << &quot;My fingers are faster than my brain. Sorry for the typos.&quot;;}

 

[Kalisto]

all the mallocs are commented out (I double checked this, and reallocs !)

and I don't use free anywhere anymore.

K

 

[PerFnurt]

Two holy rules of C++:

For every new you should have a delete.
For every new [ ] you should have a delete [].
----------

data = new byte(len); 
[code]
Is not an array allocation. Should read (I guess)
[code]
data = new byte[len]; 
[code]

And consequently should you delete with
[code]
delete[] data;
data = 0;

This doens't make any sense (unless you just put it here for illustration). Only new if you have to.:

DTag* myTag = new DTag;
myTag->operator >>(fs); 

delete myTag;
....
// More sense
DTag d;
d >> (fs);
[/delete]



/Per
[sub]
if (typos) cout << &quot;My fingers are faster than my brain. Sorry for the typos.&quot;;
[/sub]

 

[Kalisto]

OK, Thanks to the pair of you. There were numerous issues, and some were clouding others.
With your help I have fixed that, and removed nearly 1/2 the errors by fixing the one class.

Now all I need to do is to track down the rest of them,

Thanks again both.

K