Where is the Memory Leak ?

[Niner710]

Hi,

I keep getting an error in Visual Studio C++. I am trying to create a copy constructor but keep getting this error.

"Windows has triggered a breakpoint in test.exe.
This may be due to a corruption of the heap, and indicates a bug in test.exe or any of the DLLs it has loaded."

After going thru the debugger I found the problem on a delete [] that I do.

Here is a simplified version of my code. I have a couple classes in my header file(I know this is bad design but for now I just want to understand what is going wrong).

Test.h //header file
class StringValues
{
public:
StringValues();
~StringValues();
char *Name;
char *Message;
}

class Tblk
{
public:
Tblk();
~Tblk();
private:
StringValues myStringValues;
}

Here is my cpp file.

Tblk::Tblk()
{
//constructor
}

Tblk::~Tblk
{
//destructor
}

Tblk::Tblk(const Tblk& other)
{
strncpy(myStringValues.Name, other.myStringValues.Name, 100);
strncpy(myStringValues.Message, other.myStringValues.Message, 100);
}

StringValues::StringValues()
{
	Name = new char[MAXCOMMENTSTRING];
	Message = new char[MAXCOMMENTSTRING];
}

StringValues::~StringValues()
{
delete [] Name;  ===Error here
delete [] Message;
}

In my main file I have.....

void main()
{
   Tblk A;
    Tblk B = A;  
}

When I run the code thru the debugger object B gets destroyed and then myStringValues destructor is called. This is where I get an error. When I get to "delete [] Name" I get the error message that there may be a corruption of the heap. This seems like it should work. I allocated dynamic memory on the heap in the StringValues constructor and delete it in the destructor. Can anyone tell me why this doesn't work????

[Ancient Dragon]

since this is c++ why use pointers? Just make name and message both std::string and that will probably solve the memory leek problemn.

[stilllearning]

I think what is happening is this

Tblk A;
 Tblk B = A;

This is using a default copy constructor since you haven't defined one, and both your objects A and B are pointing to the same set of strings.

So when you leave main, both A and B will be destroyed, but once you destroy A and delete the pointers, when B's destructor is called, you have no valid pointer values to destroy. Hope that makes sense.

And as Ancient Dragon said you should use strings, and declare a copy constructor explicitly in order to avoid such situations.

[Niner710]

I actually created a copy constructor, so I wouldn't have that exact problem that you mentioned. I dont know if the issue is with the copy constructor that I used. I probably should just try this by using the string class, but am just curious for further knowledge why my implementation won't work. This was the copy constructor that I had.

Tblk::Tblk(const Tblk& other)
{
strncpy(myStringValues.Name, other.myStringValues.Name, 100);
strncpy(myStringValues.Message, other.myStringValues.Message, 100);
}

[Ancient Dragon]

your implementation doesn't work because it doesn't allocate memory for the two strings. Here is one way to implement it

const int MAXCOMMENTSTRING = 255;
class StringValues
{
public:
    StringValues()
    {
        Initialize();
    }
    StringValues(StringValues& s)
    {
        Initialize();
        strcpy(Name,s.Name);
        strcpy(Message,s.Message);
    }
    ~StringValues()
    {
        delete[] Name;
        delete[] Message;
    }
    char *Name;
    char *Message;
private:
    void Initialize()
    {
        Name = new char[MAXCOMMENTSTRING];
        Message = new char[MAXCOMMENTSTRING];
        memset(Name,0,MAXCOMMENTSTRING);  // optional
        memset(Message,0,MAXCOMMENTSTRING); // optional
    }
};

[skatamatic]

When you leverage off of the = operator without an overloaded ooperator=() implemented, the compiler uses a predefined method of copying the data by doing a direct member to member copy. Since you are using pointers as members, the addresses of the pointers are being copied, rather than firing the copy constructor that you have written. The easiest way to fix this would be to implement an overloaded = operator like this:

//add this to the public part of the header
Tblk & operator=(Tblk const &);

//implement it in the .cpp like this:
Tblk & Tblk::operator=(Tblk const & rhs)
{
    strncpy(myStringValues.Name, rhs.myStringValues.Name, 100);
    strncpy(myStringValues.Message, rhs.myStringValues.Message, 100);
    return *this;
}