bookysmell2004

Hi everyone.
I am experiencing a common problem with a native c++ project.
The project consists of a class and a main function file.

Code Snippet

//LString.h - class declaration


#pragma once

class LString {
private:
char *Chars;
void Copy(const char *);
public:
int Length(void);
int IndexOf(char);
int IndexOf(const char *);
int IndexOf(LString);
int LastIndexOf(char);
int LastIndexOf(const char *);
int LastIndexOf(LString);
void Replace(char,char);
void Replace(LString,LString);
void Remove(int Index, int Count);
LString SubString(int Index);
LString SubString(int Index, int Count);
public:
char& operator [] (int Index);
LString& operator = (const LString &);
LString& operator = (const char *);
public:
LString(const LString &);
LString(void);
~LString(void);

};


Code Snippet

//LString.cpp - class implementation


LString::LString(void) {
Chars = new char[1];
*Chars = 0;
}

void LString::Copy(const char *s) {
int slen = 0;
int i = 0;
for (i=0 ; *(s+i)!=0 ; i++);
slen = i;
delete [] Chars;
Chars = new char[slen+1];
for (i=0 ; i<=slen ; i++) {
*(Chars+i) = *(s+i);
}
}

LString::LString(const LString &s) {
Copy(s.Chars);
}

LString::~LString(void) {
delete [] Chars;
}

void LString::Replace(LString s, LString p) {
while (IndexOf(s)>0) {
char *Temp = new char[Length()+s.Length()-p.Length()];
int i = 0 ;
for (i=0 ; i<IndexOf(s) ; i++) {
*(Temp+i) = *(Chars+i);
}
for (i=0 ; i<p.Length() ; i++) {
*(Temp+IndexOf(s)+i) = p[i];
}
for (i=IndexOf(s)+s.Length() ; i<Length() ; i++) {
*(Temp+p.Length()-s.Length()+i) = *(Chars+i);
}
*(Temp+i) = 0;
Copy(Temp);
delete [] Temp;
}
}

int LString::IndexOf(const char *c) {
char *cp = Chars;
char *s1, *s2;
if ( !(*c) ) {
return 0;
}
while (*cp) {
s1 = cp;
s2 = (char *) c;
while ( *s1 && *s2 && !(*s1-*s2) ) {
s1++, s2++;
}
if (!*s2) {
return (int)(cp-Chars);
}
cp++;
}
return -1;
}

int LString::IndexOf(LString s) {
return IndexOf(s.Chars);
}

int LString::Length(void) {
for (int i=0 ; ; i++) {
if (*(Chars+i)==0) {
return i;
}
}
}


LString& LString::operator = (const LString &s) {
Copy(s.Chars);
return *this;
}

LString& LString::operator = (const char *s) {
Copy(s);
return *this;
}


LString will try to dynamically allocate memory according to the length of the char * that has to be copied by deleting the older pointer and allocating a new one (Copy method). The copy constructor and assignement operator has to do almost the same job. I didn't include the implementation of the other methods as they are not triggered by the main function thus their importance is lesser.

The following main function will generate a heap corruption:


Code Snippet
//Main.cpp - main source file


#include "LString.h"

int _tmain(int argc, _TCHAR* argv[])
{
LString MyString;
LString String1; LString String2; String1 = "l"; String2 = "t";
MyString = "oll";
MyString.Replace(String1,String2);
return 0;
}


In fact, I can mask the problem by removing the 2 delete lines (highlighted in source) but probably I will create another problem ... If I remove one of them the problem will still exist. I believe it has something to do with the copy constructor but I can't be sure.
What am I doing wrong Any ideas
Thanks for reading.


Re: Visual C++ General Heap corruption ...

Frank Boyne

bookysmell2004 wrote:
What am I doing wrong

Not to be excessively facetious but one thing you are doing wrong is implementing your own string class. There are several good string classes available for use and you would be much better off using one of them. The CString class for example: http://msdn2.microsoft.com/en-us/library/ms174288(VS.80).aspx

Code Snippet


void LString::Replace(LString s, LString p) {
while (IndexOf(s)>0) {
char *Temp = new char[Length()+s.Length()-p.Length()];
int i = 0 ;
for (i=0 ; i *(Temp+i) = *(Chars+i);
}
for (i=0 ; i *(Temp+IndexOf(s)+i) = p[i];
}
for (i=IndexOf(s)+s.Length() ; i *(Temp+p.Length()-s.Length()+i) = *(Chars+i);
}
*(Temp+i) = 0;
Copy(Temp);
delete [] Temp;
}
}


One problem with the code above is the calculation of the size of the array Temp. The Replace method will modify the current string by substituting the new substring p for the old substring s. Thus the length of the modified string will be the length of the original string reduced by the length of s and increased by the length of p.

The calculation above gets that reversed - it increases by the length of s and reduces by the length of p. If p is longer than s, the calculation will result in a Temp that is too short and replace will overflow off the end of Temp and into the rest of the heap.

The calculation of the length of Temp also forgets to allow for the null character. The function Length () returns the number of characters in the string not including the null termination. In your particular case, the original string is length 3 and the strings s and p are both length 1. Thus, Temp is size 3. But, Temp has to be long enough to hold the modified string (also length 3) plus the null inserted by the statement *(Temp+i) = 0;

Code Snippet

LString::LString(const LString &s) {
Copy(s.Chars);
}

Another problem you have is that this constructor never calls new to instantiate Chars. Thus, when Copy () calls delete to delete the object this.Chars the member Chars isn't pointing at anything (at least nothing created by new).





Re: Visual C++ General Heap corruption ...

bookysmell2004

Well, if you have someone above your head asking you to do this you rather do it ^^
Uhm also this is ment to be as more portable as possible.

I'm impresed how this worked...

Code Snippet

LString::LString(const LString &s) {
Chars = new char[1];
Copy(s.Chars);
}

void LString::Replace(LString s, LString p) {
while (IndexOf(s)>0) {
char *Temp = new char[Length()+p.Length()-s.Length()+1];
int i = 0 ;
for (i=0 ; i<IndexOf(s) ; i++) {
*(Temp+i) = *(Chars+i);
}
for (i=0 ; i<p.Length() ; i++) {
*(Temp+IndexOf(s)+i) = p[i];
}
for (i=IndexOf(s)+s.Length() ; i<Length() ; i++) {
*(Temp+p.Length()-s.Length()+i) = *(Chars+i);
}
*(Temp+i) = 0;
Copy(Temp);
delete [] Temp;
}
}



Those are what I changed and everything seems to be working now.
Thank you very very much Frank Boyne for your time and your answers =)




Re: Visual C++ General Heap corruption ...

Simple Samples

Frank Boyne wrote:
Not to be excessively facetious but one thing you are doing wrong is implementing your own string class.

Usually unreasonable requirements are class assignments. That might not be true here but usually when someone tries to reinvent something such as a string it is because an instructor has made it a requirement.






Re: Visual C++ General Heap corruption ...

Simple Samples

bookysmell2004 wrote:
Well, if you have someone above your head asking you to do this you rather do it ^^
Uhm also this is ment to be as more portable as possible.

By definition of using a class, you are using C++. Nothing would be more portable using C++ than the std :: string class.






Re: Visual C++ General Heap corruption ...

bookysmell2004

I don't know really why and how but that was what I had to do. Besides, it wasn't wasted at all if you consider that I learned some basic concepts of the copy constructor or some common errors that may occur ...

So I can close this thread with a big smilie face

Smile