Solved

Memory leak

Posted on 2000-04-07
6
335 Views
Last Modified: 2013-12-14
Hello

I have made a string class(sample code below) that for some reason is leaking memory.
Why do I get runtime errors when I call 'delete []' or 'delete' in the destructor(when one of them is uncommented :-)?
If I don't delete the pointer to the 'stringData', it will leak memory. I need someone to help me out with this. I want to delete the pointer in the destructor!

/NiL

/////////////////////////
//Declaration
class CMyString
{
public:
      CMyString();
      virtual ~CMyString();

      char * data();
      char data(int iPos);
      bool set(const char * ccStr);
      bool set(CMyString myString);
      bool clear();
      bool append(const char * ccStr);
      bool append(const char ccChar);
      bool prepend(const char * ccStr);
      bool prepend(const char ccChar);
      bool remove(int start, int slut = -1);
      //int last(const char * ccStr);
      int last(const char ccChar);
      //int first(const char * ccStr);
      int first(const char ccChar);
      bool contains(const char * ccStr, int ignoreCase = 0);
      int length();
      bool iter(const char * delims = "\x0A\x0D ");
      bool iter2(const char * delims = "\x0D\x0A");
      char * iterKey();
      bool iterReset();
      bool compareTo(const char * aString);
      bool trim(int how = 0, const char * ts = " ");//0 = trailing, 1 = leading, 2 = both
      bool isEmpty();
      CMyString & operator=(const CMyString &s);
      bool toUpper();

private:
      char * stringData;
      int iterPos;
      int iterStart;
      int iterEnd;
};

/////////////////////////////
//Implementation
CMyString::CMyString()
{
      stringData = new char[1];
      stringData[0] = '\0';
      iterPos = -1;
      iterStart = -1;
      iterEnd = -1;
}

CMyString::~CMyString()
{
      //stringData[0] = '\0';
      //delete stringData;
      try
      {
      //      delete [] stringData;
      //      delete stringData;
      }
      catch(...)
      {
      }
}

char * CMyString::data()
{
      char * temp = new char[strlen(stringData)+1];
      strcpy(temp, stringData);

      return temp;
}

char CMyString::data(int iPos)
{
      char * temp = new char[1];
      temp[0] = '\0';
      int j = -1;
      try
      {
            j = strlen(stringData);
      }
      catch(...)
      {
            return temp[0];
      }
      if((iPos < 0) || (iPos >= j) || (j < 0))
            return temp[0];
      temp[0] = stringData[iPos];

      return temp[0];
}

bool CMyString::set(const char *ccStr)
{
      try
      {
            int i = strlen(ccStr);
            delete [] stringData;
            stringData = new char[i + 1];
            strcpy(stringData, ccStr);
            stringData[i] = '\0';
      }
      catch(...)
      {
            return false;
      }
      return true;
}

bool CMyString::set(CMyString myString)
{
      try
      {
            int i = myString.length();
            delete [] stringData;
            stringData = new char[i + 1];
            strcpy(stringData, myString.data());
            stringData[i] = '\0';
      }
      catch(...)
      {
            return false;
      }
      return true;
}

bool CMyString::clear()
{
      try
      {
            delete [] stringData;
            stringData = new char[1];
            stringData[0] = '\0';
      }
      catch(...)
      {
            return false;
      }
      return true;
}

bool CMyString::append(const char * ccStr)
{
      try
      {
            int i = strlen(ccStr);
            int j = strlen(stringData);
            char * tc = new char[j + i + 1];
            sprintf(tc,"%s%s",stringData,ccStr);
            delete [] stringData;
            stringData = new char[j + i + 1];
            strcpy(stringData, tc);
            stringData[j + i] = '\0';
      }
      catch(...)
      {
            return false;
      }
      return true;
}

bool CMyString::append(const char ccChar)
{
      try
      {
            int j = strlen(stringData);
//            int j = sizeof(stringData)/sizeof(char);
            char * tc = new char[j + 1 + 1];
            sprintf(tc,"%s%c",stringData,ccChar);
            delete [] stringData;
            stringData = new char[j + 1 + 1];
            strcpy(stringData, tc);
            stringData[j + 1] = '\0';
      }
      catch(...)
      {
            return false;
      }
      return true;
}

bool CMyString::prepend(const char * ccStr)
{
      try
      {
            int i = strlen(ccStr);
            int j = strlen(stringData);
            char * tc = new char[j + i + 1];
            sprintf(tc,"%s%s",ccStr,stringData);
            delete [] stringData;
            stringData = new char[j + i + 1];
            strcpy(stringData, tc);
            stringData[j + i] = '\0';
      }
      catch(...)
      {
            return false;
      }
      return true;
}

bool CMyString::prepend(const char ccChar)
{
      try
      {
            int j = strlen(stringData);
//            int j = sizeof(stringData)/sizeof(char);
            char * tc = new char[j + 1 + 1];
            sprintf(tc,"%c%s",ccChar,stringData);
            delete [] stringData;
            stringData = new char[j + 1 + 1];
            strcpy(stringData, tc);
            stringData[j + 1] = '\0';
      }
      catch(...)
      {
            return false;
      }
      return true;
}

bool CMyString::remove(int start, int slut)
{
      if((start < 0) || ((slut < start) && (slut != -1)))
            return false;

      try
      {
            int j = strlen(stringData);

            if((j <= 0) || (j <= start))
                  return false;
            char * tc = new char[j + 1];
            for(int xi = 0; xi < (j + 1); xi++)
                  tc[xi] = '\0';
            int i = 0;
            for(i = 0; i < start; i++)
            {
                  tc[i] = stringData[i];
            }
            int ax = slut+1;
            if(slut == -1)
                  ax = j;
            for(int ii = ax; ii < j; ii++,i++)
            {
                  tc[i] = stringData[ii];
                  //i++;
            }
            int k = strlen(tc);
            delete [] stringData;
            stringData = new char[k + 1];
            strcpy(stringData, tc);
            stringData[k] = '\0';
      }
      catch(...)
      {
            return false;
      }
      return true;
}
0
Comment
Question by:Nusse
6 Comments
 
LVL 7

Expert Comment

by:KangaRoo
ID: 2692681
Not sure if my last comment arrived, here it is again:

There is a leak in CMyString::data(int) and CMyString::data() allocates memory which the caller of that function must delete (that is unusual).

The destructor should indeed delete the allocated memory with delete [] stringData but you should make sure that it is either valid or the NULL pointer. To ensure this it is common practice to assign NULL to the pointer, ie:

   delete [] stringData;
   stringData = 0;

It does appear that stringData gets to point to a newly allocated memory everywhere it is deleted, but with a little 'catch'
 
   try
   {
        delete [] stringData;
        stringData = new char[some_size + 1];
   }
   catch(...)
   {
        return false;
   }

Although this piece appears to handle exceptions thrown my new (bad_alloc), it does not! When new throws, stringData will still point to the old memory, which is deleted...
But, since shortage of memory is rare these days and bad_alloc will rarely be  thrown, there is probably still another error.
0
 
LVL 30

Expert Comment

by:Zoppo
ID: 2692756
Hi Nusse,

I agree with KangaRoo, the 'data' function shouldn't allocate memory which then has to be deleted from calling functions. That's definitly bad style. Let the 'char * CMyString::data()' function simply return the 'stringData' pointer, that's enough that calling function can work (i.e. create a copy) with the string. In 'char CMyString::data(int iPos)' you do not need to allocate memory at all, just use something like this:

char CMyString::data(int iPos)
{
 int j = -1;
 try
 {
  j = strlen(stringData);
 }
 catch(...)
 {
 return 0;
 }

 if((iPos < 0) || (iPos >= j) || (j < 0))
  return 0;

 return stringData[iPos];
}


function 'append', 'prepend' and 'remove' will also cause memory leaks coz there two buffers are create with 'new'. In 'append' and 'prepend' you can simply call a 'stringData = tc;' after deleting the 'stringData', this will avoid the memory leaks and even spare code/time, coz the 'strcpy' isn't needed for this.

The remove function could be implemented similar if you calculate the length ot the resulting string before allocating 'tc' with this calculated length and the even simply call 'stringData = tc;' after deleting the 'stringData'.

so, this will solve all problems I found in your code. Of course you have to call 'delete [] stringData' in dtor.

hope that helps,

ZOPPO
0
 

Author Comment

by:Nusse
ID: 2693484
Thanks for the quick reply to this Q. I will implement the changes ASAP(this weekend).


Btw, do You know where I can buy some time? I need like a week or two right now.

0
What Should I Do With This Threat Intelligence?

Are you wondering if you actually need threat intelligence? The answer is yes. We explain the basics for creating useful threat intelligence.

 
LVL 1

Expert Comment

by:ntdragon
ID: 2697431
did you fixed the problem???

here some thing i would change
1)you should use delete[]stringData
2)move the Implementation of the C'Tor and the D'Tor into the class
3)if you use string.h why don't you use strcat

i would do it like that:

bool CMyString::append(const char * ccStr)
{
try
{
int i = strlen(ccStr);
int j = strlen(stringData);
char * tc = new char[j + i + 1];
strcpy(tc,stringData);
strcat(tc,ccStr);
tc[j+i]='\0';
delete [] stringData;
stringData = new char[j + i + 1];
strcpy(stringData, tc);
stringData[j + i] = '\0';
}
catch(...)
{
return false;
}
return true;
}


bool CMyString::prepend(const char * ccStr)
{
try
{
int i = strlen(ccStr);
int j = strlen(stringData);
char * tc = new char[j + i + 1];
strcpy(tc,ccStr);
strcat(tc,stringData);
tc[i+j]='\0';
delete [] stringData;
stringData = new char[j + i + 1];
strcpy(stringData, tc);
stringData[j + i] = '\0';
}
catch(...)
{
return false;
}
return true;
}


4)problem with '\0'

bool CMyString::prepend(const char ccChar)
{
try
{
int j = strlen(stringData);
// int j = sizeof(stringData)/sizeof(char);
char * tc = new char[j + 1 + 1];
sprintf(tc,"%c%s",ccChar,stringData);
delete [] stringData;
stringData = new char[j + 1 + 1];
strcpy(stringData, tc);
stringData[j + 1] = '\0';
}
catch(...)
{
return false;
}
return true;
}

bool CMyString::remove(int start, int slut)
{
if((start < 0) || ((slut < start) && (slut != -1)))
return false;

try
{
int j = strlen(stringData);

if((j <= 0) || (j <= start))
return false;
char * tc = new char[j + 1];
for(int xi = 0; xi < (j + 1); xi++)
tc[xi] = '\0';
int i = 0;
for(i = 0; i < start; i++)
{
tc[i] = stringData[i];
}
int ax = slut+1;
if(slut == -1)
ax = j;
for(int ii = ax; ii < j; ii++,i++)
{
tc[i] = stringData[ii];
//i++;
}
int k = strlen(tc);
delete [] stringData;
stringData = new char[k + 1];
strcpy(stringData, tc);
stringData[k] = '\0';
}
catch(...)
{
return false;
}
return true;
}


about the loop
for(int ii = ax; ii < j; ii++,i++)
{
tc[i] = stringData[ii];
//i++;
}
j is the length of stringData
then in your loop you go till ii<j
so you don't get till the '\0' in stringData
so tc won't have '\0' at the end
so when you are making:

int k = strlen(tc);
delete [] stringData;
stringData = new char[k + 1];
strcpy(stringData, tc);
stringData[k] = '\0';

you're accesing not allocated memory
0
 

Author Comment

by:Nusse
ID: 2700206
Processing last input... *beep*
0
 
LVL 2

Accepted Solution

by:
abesoft earned 100 total points
ID: 2716415
It looks like your problem stems from the lack of a copy constructor in your class.  A copy c'tor is called whenever an object of your class is copied (as a value parameter, for example.)  By default, the compiler will generate a copy c'tor for you, but the copied object will contain a copy of the data pointer.  This is bad.  You now have two objects pointing at the same data.

To confirm that this is your problem, tack this into your class and compile:

private:
    CMyString( const CMyString &copy):
        stringData( 0)
    {
        stringData = new char[ lstrlen( copy.stringData)];
    }

If you get new errors after you add this, then you know that your code was using the default copy c'tor.  

You can fix the problem by making that copy c'tor public.

Hope this helps.
0

Featured Post

Highfive + Dolby Voice = No More Audio Complaints!

Poor audio quality is one of the top reasons people don’t use video conferencing. Get the crispest, clearest audio powered by Dolby Voice in every meeting. Highfive and Dolby Voice deliver the best video conferencing and audio experience for every meeting and every room.

Join & Write a Comment

Article by: SunnyDark
This article's goal is to present you with an easy to use XML wrapper for C++ and also present some interesting techniques that you might use with MS C++. The reason I built this class is to ease the pain of using XML files with C++, since there is…
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…
The viewer will learn how to use NetBeans IDE 8.0 for Windows to connect to a MySQL database. Open Services Panel: Create a new connection using New Connection Wizard: Create a test database called eetutorial: Create a new test tabel called ee…
The viewer will learn how to use and create new code templates in NetBeans IDE 8.0 for Windows.

758 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

Need Help in Real-Time?

Connect with top rated Experts

19 Experts available now in Live!

Get 1:1 Help Now