Heap Corruption

[ascii]

I've been trying to define my own string class and have barely gotten started but am already getting an obnoxious error :/. I'm getting a "Debug Error: Heap Corruption Detected: After Normal Block(#61) CRT detected that application wrote to memory after end of heap buffer." I've had exceptions like this before, but they're just when I've accidently screwed with a pointer after deleting its memory. Here's my class definition so far:

class CString
{
public:
	CString(const char* string)
	{
		str = new char[strlen(string)];
		strcpy(str, string);
		length = strlen(string);
	}
	CString(const CString& string)
	{
		str = new char[strlen(string.str)];
		strcpy(str, string.str);
		length = strlen(string.str);
	}

	~CString() 
	{ delete[] str; }

	void operator=(const CString string)
	{
		this->str = new char[strlen(string.str)+1];
		strcpy(this->str, string.str);
		return;
	}

private:
	char* str;
	size_t length;
};

[Zhuge]

Your operator=() seems like it could write past the end of this->str if string.str happens to be longer than it can hold.

[ascii]

I changed the function definition to:

void operator=(const CString string)
{
	this->str = new char[strlen(string.str)+1];
	strcpy(this->str, string.str);
	return;
}

But to no avail :o

[firedraco]

Both of your constructors need a +1 after strlen in there too.

[ascii]

<33333333333333

Thank you :)

[D80DSL3A]

A bit premature with that green check mark! Release the memory already allocated to str before allocating a new block of memory to it!!

void operator=(const CString string)
{
        delete [] str;
	this->str = new char[strlen(string.str)+1];
	strcpy(this->str, string.str);
	return;
}

[ascii]

Good catch, thank you :-)

I'm encountering some more troubles overloading the += operator. I haven't even tried the + operator yet. The += operator keeps making the damn program crash! Here's the class definition now:

class CString
{
public:
	//Constructors
	CString(const char* string)
	{
		str = new char[strlen(string)+1];
		strcpy(str, string);
		length = strlen(string);
	}
	CString(const CString& string)
	{
		str = new char[string.length+1];
		strcpy(str, string.str);
		length = string.length;
	}

	~CString() 
	{ delete[] str; }

	void operator=(const CString string);
	CString operator+(const CString string);
	void operator+=(const CString string);
	void print();

private:
	char* str;
	size_t length;
};

void CString::operator=(const CString string)
{
	delete[] str;
	if(this == &string)		// return if the two are the same
		return;
	delete[] str;
	this->str = new char[string.length+1];
	strcpy(this->str, string.str);
	return;
}

CString CString::operator+(const CString string)
{
	char* temp = new char[this->length];
	strcat(temp, string.str);
	CString concatenated(temp);
	delete[] temp;
	return concatenated;
}

void CString::operator+=(const CString string)
{
	strcat(this->str, string.str);
	return;
}

void CString::print()
{
	std::cout << str << std::endl; 
	return;
}

[firedraco]

If string.str isn't big enough to hold itself + this->str (which it never is based on your other operators), this will be writing to invalid memory.

[Zhuge]

Also in your operator=(), are you want to delete the memory at str on line 33? I would think you wouldn't want to do anything if the objects are in fact the same.

In your operator+() you don't ever assign the contents of this->str to temp.

[kfmfe04]

also all your arguments should be const references (const CString& string) if you don't want to be copy-constructing all over the place

[D80DSL3A]

Another point. The = and += operators should return a CString&, not type void.

[ascii]

Thank you for all the advice!

@firedraco, yeah I realized that and am working on it with limited success :-(
@Zhuge, the first thing was a typo, thank you. I haven't really even got started with the operator+ yet, so I have some work to do!
@kfmfe04, I just did that in the beginning to test my copy constructor
@fun2code, very good point!

More heap corruption errors with the += operator! I'm trying to solve these myself, but you guys are all far better programmers than me :o. Sorry if I'm coming off as a help vampire :/ I'll try to fix this when I have a chance next:

class CString
{
public:
	//Constructors
	CString(const char* string)
	{
		str = new char[strlen(string)+1];
		strcpy(str, string);
		length = strlen(string+1);
	}
	CString(const CString& string)
	{
		str = new char[string.length+1];
		strcpy(str, string.str);
		length = string.length+1;
	}

	~CString() 
	{ delete[] str; }

	CString& operator=(const CString string);
	CString operator+(const CString string);
	CString& operator+=(const CString string);
	void print();

private:
	char* str;
	size_t length;
};

CString& CString::operator=(const CString string)
{
	if(this == &string)		// return if the two are the same
		return *this;
	delete[] str;
	this->str = new char[string.length+1];
	strcpy(this->str, string.str);
	this->length = string.length;
	return *this;
}

CString CString::operator+(const CString string)
{
	char* temp = new char[this->length];
	strcat(temp, string.str);
	CString concatenated(temp);
	delete[] temp;
	return concatenated;
}

CString& CString::operator+=(const CString string)
{
	this->length += string.length;
	char* temp = new char[this->length+1];
	temp = this->str;
	strcat(temp, string.str);
	CString newStr(temp);
	delete[] temp;
	return newStr;
}

void CString::print()   
{
	std::cout << str << std::endl; 
	return;
}

[Zhuge]

In your operator+=():

Line 54, you allocate some memory. You then immediately lose the pointer to it (creating a memory leak) on line 55. Why are you even creating "temp"? See next item.

Also, from line 57 and 59, you are returning a reference to a local variable, which is invalid. You should make the changes to this->str in-place (you are supposed to be modifying the variable itself, anyway), instead, and return *this.

[ascii]

The problem is that I can't make the changes to this->str immediately because it is too small to hold both of the strings. That's why I created temp. Temp would be larrge enough to hold both of the strings. So I tried to copy the value of this->str to temp, which I obviously screwed up, leading to that memory leak. Since I'll now be returning *this[/code[, I guess to concatenate [code]string.str with temp, and then make this->str = temp should . Obviously this doesn't work though :/ I'll keep working on it.

[Zhuge]

Ah, I see. Instead of creating another CString on line 57, try delete[]ing this->str, then assigning temp to this->str. Then return *this.

[ascii]

I'm still getting a Heap Corruption Error. It says: "CRT detected that the application wrote to memory after end of heap buffer."

Here's my new function definition for +=

CString& CString::operator+=(const CString string)
{
	this->length += string.length;
	char* temp = new char[this->length+1];
	strcat(temp, this->str);
	strcat(temp, string.str);
	delete[] this->str;
	strcpy(this->str, temp);
	
	delete[] temp;
	return *this;
}

[kfmfe04]

well, Line 8 after doing Line 7 is bad

[LB]

You delete your string on line 7, then try to copy to it!? Remove line 10 and just do str = temp, done.

[ascii]

How do you recommend copying it then? I'm kinda out of ideas here.

[LB]

CString& CString::operator+=(const CString string)
{
	this->length += string.length;
	char* temp = new char[length+1];
	strcat(temp, str);
	strcat(temp, string.str);
	delete[] str;
	//strcpy(str, temp);
	str = temp; //note this
	//delete[] temp;
	return *this;
}

Also, there's not need to use this-> all the time unless you have a parameter or local variable with the same name as your class variable.