?
Solved

will using realloc cause memory leak in this case?

Posted on 2005-04-11
22
Medium Priority
?
552 Views
Last Modified: 2013-12-14
i have some code like the one below and i wonder if it generates memory leaks

while starting in debug mode vs does not report any mem leak, i have a profiler that warns me of a possible memory leak in this situation.

if it does leak, why? and how should i correct the problem?

thanks,
A.

class A{
public:
 int pnVals;
 A()
 {
  pnVals = new int[10];
 }
 A( A& copy )
 {
  pnVals = new int[10];
  memcpy(pnVals, copy.pnVals, 10 );
 }
 ~A()
 {
  delete [] pnVals;
 }
}

void main()
{
 A someA;
 CSimpleArray arrOfA;
 int i = 0;
 while( i < 10 )
 {
   someA.pnVals[0] = i;
   arrOfA.Add( pnVals ); // here realloc gets called
 }

}

class CSimpleArray
{
//...
      BOOL Add(T& t)
      {
            if(m_nSize == m_nAllocSize)
            {
                  T* aT;
                  int nNewAllocSize = (m_nAllocSize == 0) ? 1 : (m_nSize * 2);
                  aT = (T*)realloc(m_aT, nNewAllocSize * sizeof(T));
                  if(aT == NULL)
                        return FALSE;
                  m_nAllocSize = nNewAllocSize;
                  m_aT = aT;
            }
            m_nSize++;
            SetAtIndex(m_nSize - 1, t);
            return TRUE;
      }

//...
}
0
Comment
Question by:Agarici
  • 11
  • 8
  • 2
  • +1
22 Comments
 
LVL 30

Expert Comment

by:Axter
ID: 13753324
Hi Agarici,
> >pnVals = new int[10];
You're trying to assign allocated memory to a non-pointer.
That means this will result in a memory leak.
pnVals should be a pointer.
 int *pnVals;


David Maisonave :-)
Cheers!
0
 
LVL 30

Expert Comment

by:Axter
ID: 13753357
Agarici,
> >memcpy(pnVals, copy.pnVals, 10 );
I recommend against using memcpy in your copy constructor.
Instead, you should iterate through each item and assign the value.
Also, the above memcpy is not coping the entire data, because it's only copying 10 bytes.
An integer is longer then 10 bytes, so it should be something like the following:
memcpy(pnVals, copy.pnVals, 10*sizeof(int) );

David Maisonave :-}
0
 
LVL 11

Author Comment

by:Agarici
ID: 13753373
:)
sorry about that
i did not copy paste the code
and i wrote it wrong in my post

ofcourse the class contains a pointer
class A{
public:
 int *pnVals;
//...
}

you see i even named it pnVals

so considering pnVals is declared as int* is this the case of a mem leak?

A.
0
Concerto's Cloud Advisory Services

Want to avoid the missteps to gaining all the benefits of the cloud? Learn more about the different assessment options from our Cloud Advisory team.

 
LVL 1

Expert Comment

by:VolatileVoid
ID: 13753417
Mixing the *alloc functions with new and delete is generally "bad karma." You CAN do it - and you'll get expected results with built-in primitive types (like int, double, char, float, etc), but unlike new, the *alloc functions will not call your class's constructor.
0
 
LVL 30

Expert Comment

by:Axter
ID: 13753419
Instead of using a pointer to store a dynamic array, I recommend you use std::vector instead.
It's safer, and you're less likely to produce memory leaks.  You destructor doesn't have to do anything to clear up the memory if you use a vector.

#include <vector>


class A{
public:
    std::vector<int> pnVals;
    A()
    {
        pnVals.resize(10);
    }
    A(const A& copy )
    {
        pnVals = copy.pnVals;
    }
    ~A()
    {
        //Destructor doesn't have to do anything
    }
};

By using a vector, that makes less work for you, and makes your code much safer.
0
 
LVL 30

Expert Comment

by:Axter
ID: 13753455
Agarici,
> >arrOfA.Add( pnVals ); // here realloc gets called

Where is pnVals coming from.

Please copy and paste the code, instead of trying to type it in your question.
That way we'll see exactly what you're seeing.
Other we'll be looking in the wrong direction.

David Maisonave :-}
0
 
LVL 30

Expert Comment

by:Axter
ID: 13753490
Agarici,
> > A( A& copy )
> > {
> >  pnVals = new int[10];

Even if pnVals is of the right type, the copy constructor still has a memory leak.
In the copy construct does not first try to delete the contents of pnVals, before assigning it a new value.

That means what ever memory was previously assinged to this pointer, will become a memory leak.


David Maisonave :-}
0
 
LVL 11

Author Comment

by:Agarici
ID: 13753582
ok, so let me re-write (corect) the code.
i can't post my original code, it contains a lot of other stuff in is much dificult to read
for instance in my code the size (10) is not hardcoded but this makes no difference to the question

also, the code is allready written like this, and it works. so i would preffer not to change it unless it is really neccessary.
i got into this question when the profiler app said:
"Memory Leak Due to Free: Calling realloc causes a leak of address 0x2246098 (12) allocated by global_operator_new."



class A{
public:
 int pnVals;
 A()
 {
  pnVals = new int[10];
 }
 A( A& copy )
 {
  pnVals = new int[10];
  memcpy(pnVals, copy.pnVals, 10*sizeof(int) );
 }
 ~A()
 {
  delete [] pnVals;
 }
}

void main()
{
 A someA;
 CSimpleArray<A> arrOfA;
 int i = 0;
 while( i < 10 )
 {
   someA.pnVals[0] = i;
   arrOfA.Add( someA ); // here realloc gets called
 }

}
0
 
LVL 11

Author Comment

by:Agarici
ID: 13753601
why should i try to delete the pnVals if i am now constructing the new object
if it is new, does it not mean pnVals is unallocated?

A.
0
 
LVL 1

Assisted Solution

by:VolatileVoid
VolatileVoid earned 200 total points
ID: 13753690
Instead of using new in class A (if you MUST use realloc), can you try using malloc? And in A's dtor, can you use free instead of delete?

You're deleting alloc'd memory. Never delete alloc'd memory; you delete new'd memory, you free alloc'd memory.
0
 
LVL 30

Expert Comment

by:Axter
ID: 13753713
>>why should i try to delete the pnVals if i am now constructing the new object
>>if it is new, does it not mean pnVals is unallocated?

You're right.  I was think of an assignment operator.

A copy construct would not have this problem, but you still need to fix the memcpy command which is not coping the entire data.
0
 
LVL 30

Expert Comment

by:Axter
ID: 13753750
Agarici,
FYI:
Even in the code you just posted, the variable is still an integer instead of a pointer.

int pnVals;


>>ok, so let me re-write (corect) the code.

Please don't re-write.  We're probably not going to see the problem, with re-written code.

Instead copy and paste the code here.
Then just delete the parts you don't want to post.
That way we're sure that we're seeing the real code, instead of typing errors.
0
 
LVL 16

Expert Comment

by:PaulCaswell
ID: 13753934
Isnt this a leak?

A( A& copy )
 {
  pnVals = new int[10];
  memcpy(pnVals, copy.pnVals, 10*sizeof(int) );
 }

You are orphaning the old pnVals that was built at create time when you create your new one!

You need something like:

A( A& copy )
 {
   memcpy(pnVals, copy.pnVals, 10*sizeof(int) );
 }

Paul
0
 
LVL 11

Author Comment

by:Agarici
ID: 13753971
@VolatileVoid : i did not know that CSimpleArray uses realoc when i wrote the code
CSimpleArray is a atl class and i don't have control over it.
however the code works fine, it does not generate any errors nor memory leaks (i mean the visual studio debugger does not show any mem leak at the end of the execution) but i admit i also see it as a problem (calling delete on something *alloc'd

@Axter: i was not so carefull with the code i posted because i did not thought it is so important. i did not post the question to get the code sintax corrected, but to make a point (that is: in my class i use a pointer that the profiler complains could be overwritten)

well, the code is wrong again, i admit, but i said it perfectly clear that it is a POINTER
and if i copy paste the code now it will result the same code as above except for the stupid 'int *pnVals' and for the main which will be an function called from somewere in the code something like this:

//function
{
//...
 A someA;
 CSimpleArray<A> arrOfA;
 int i = 0;
 while( i < m_nArraySize )
 {
   LoadData( &someA, i );// here i set the values of the class member variables including the values in the int array pointed to by pnVals
   arrOfA.Add( someA ); // here realloc gets called
 }
//...
}




in my oppinion the code should fine,
if it there were an error, i thought it would be because of the pointer pnVals beeing overwriten and hence the data pointed by if beeng lost
but (also imo) there should be no mem leak since the refference to the memory allocated by new in the constructor is not lost when the realloc happens(because the address of pnVals is copied to the new relocated memory so i can delete it in the destructor)

i asked this question because i am not certain that i am correct

so am i wrong?

A.
0
 
LVL 11

Author Comment

by:Agarici
ID: 13753986
@PaulCaswell : as i said earlier ( to axter ) it is a copy constructor so your code will generate an exception the first time executed

A.
0
 
LVL 30

Expert Comment

by:Axter
ID: 13753991
>>You are orphaning the old pnVals that was built at create time when you create your new one!

That's what I first thought too.  But that logic is for an assignment operator.
On an assignement operator you're working on an initialized object, and therefore you would have a memory leak.
However, on a copy constructor you're working on an un-initialized object, and therefore pnVals has not yet been initialized.

To make it less confusing, I would recommend using initialize list.
A( A& copy ):pnVals(new int[10])
 {
  memcpy(pnVals, copy.pnVals, 10*sizeof(int) );
 }

0
 
LVL 30

Expert Comment

by:Axter
ID: 13754034
>>i did not post the question to get the code sintax corrected,
It's hard to find a problem, if you have to filter out syntax errors.

With out having access to the original code, we can only guess as to what the profile is reporting for a memory leak.

Can you at least post the warning that the profile is reporting as a memory leak.

Just copy and paste the entire warning.
0
 
LVL 11

Author Comment

by:Agarici
ID: 13754093
i posted it above, but here it is again

"Memory Leak Due to Free: Calling realloc causes a leak of address 0x2246098 (12) allocated by global_operator_new."


Axter i assure you there is nothing else in the code to change the problem.
and yes it is harder if you have to filter out sintax errors and i'm sorry for those in the code above.

A.
0
 
LVL 30

Expert Comment

by:Axter
ID: 13754159
>>"Memory Leak Due to Free: Calling realloc causes a leak of address 0x2246098 (12) allocated by global_operator_new."

Sorry if I overlooked your previous post.  This makes a big difference in tracking down the problem.

Where are you freeing m_aT?

Can you post that code.

The profiler could be getting confused because the code is assigned the value to a temporary variable before assigning it back to the target variable.  Try using the same variable for return variable as argument variable for realloc.
Example:

               T* aT = m_aT; //Assign to m_aT
               int nNewAllocSize = (m_nAllocSize == 0) ? 1 : (m_nSize * 2);
               aT = (T*)realloc(aT , nNewAllocSize * sizeof(T)); // Us aT instead of m_aT
               if(aT == NULL)
                    return FALSE;
               m_nAllocSize = nNewAllocSize;
               m_aT = aT;
0
 
LVL 11

Author Comment

by:Agarici
ID: 13754183
this is not an option the code in the class CSimpleArray is not of my one
it is part of atl and i can not modiffy it

you will find the entire source in atlbase.h


A.
0
 
LVL 30

Accepted Solution

by:
Axter earned 800 total points
ID: 13754204
If you're profiler is giving you a warning for this part of the code, then more then likely it's a false warning.

Profilers are not perfect, and will often give you false warnings, especially with complex code like templates.
0
 
LVL 11

Author Comment

by:Agarici
ID: 13754234
ok, but it is saying the pointer pnVals will be lost

i'll look more into this tomorrow ( it is getting prety late here)

thank you all for your posts.
A.
0

Featured Post

Industry Leaders: We Want Your Opinion!

We value your feedback.

Take our survey and automatically be enter to win anyone of the following:
Yeti Cooler, Amazon eGift Card, and Movie eGift Card!

Question has a verified solution.

If you are experiencing a similar issue, please ask a related question

C++ Properties One feature missing from standard C++ that you will find in many other Object Oriented Programming languages is something called a Property (http://www.experts-exchange.com/Programming/Languages/CPP/A_3912-Object-Properties-in-C.ht…
Go is an acronym of golang, is a programming language developed Google in 2007. Go is a new language that is mostly in the C family, with significant input from Pascal/Modula/Oberon family. Hence Go arisen as low-level language with fast compilation…
This tutorial covers a step-by-step guide to install VisualVM launcher in eclipse.
The viewer will learn additional member functions of the vector class. Specifically, the capacity and swap member functions will be introduced.
Suggested Courses

864 members asked questions and received personalized solutions in the past 7 days.

Join the community of 500,000 technology professionals and ask your questions.

Join & Ask a Question