operator+ overload returning object causing memory leaks, C++

Question

The problem I think is with returning an object when i overload the + operator. I tried returning a reference to the object, but doing so does not fix the memory leak. I can comment out the two statements:

dObj = dObj + dObj2;

and

cObj = cObj + cObj2;

to free the program of memory leaks. Somehow, the problem is with returning an object after overloading the + operator.

    #include <iostream>
    #include <vld.h>

    using namespace std;

    class Animal
    {
    public :
        Animal() {};
        virtual void eat()  = 0 {};
        virtual void walk() = 0 {};
    };

    class Dog : public Animal
    {
    public :
        Dog(const char * name, const char * gender, int age);
        Dog() : name(NULL), gender(NULL), age(0) {};

        virtual ~Dog();
        Dog operator+(const Dog &dObj);

    private :
        char * name;
        char * gender;
        int age;
    };

    class MyClass
    {
    public :
        MyClass() : action(NULL) {};
        void setInstance(Animal &newInstance);
        void doSomething();

    private :
        Animal * action;
    };


    Dog::Dog(const char * name, const char * gender, int age) :  // allocating here, for data passed in ctor
            name(new char[strlen(name)+1]), gender(new char[strlen(gender)+1]), age(age)
    {
        if (name)
        {
            size_t length = strlen(name) +1;
            strcpy_s(this->name, length, name);
        }
        else name = NULL;
        if (gender)
        {
            size_t length = strlen(gender) +1;
            strcpy_s(this->gender, length, gender);
        }
        else gender = NULL;
        if (age)
        {
            this->age = age;
        }
    }
    Dog::~Dog()
    {
        delete name;
        delete gender;
        age = 0;
    }

    Dog Dog::operator+(const Dog &dObj)
    {
        Dog d;
        d.age = age + dObj.age;
        return d;
    }

    void MyClass::setInstance(Animal &newInstance)
    {
        action = &newInstance;
    }
    void MyClass::doSomething()
    {
        action->walk();
        action->eat();  
    }
    int main()
    {
        MyClass mObj;

        Dog dObj("Scruffy", "Male", 4); // passing data into ctor
        Dog dObj2("Scooby", "Male", 6);

        mObj.setInstance(dObj); // set the instance specific to the object.
        mObj.doSomething();  // something happens based on which object is passed in

        dObj = dObj + dObj2; // invoke the operator+ 
        return 0;
    }

Answer 1

If you are going to do your own memory management (which you shouldn't; use std::string!), you need to make sure that your class has the following user-defined functions:

• a destructor
• a copy constructor
• an assignment operator

(In addition, you'll usually have a user-defined constructor, too)

You have a user-defined destructor (thought you need to use the array delete[], not the scalar delete), but you do not have a user-defined copy constructor or assignment operator, so any time you copy an object or assign an object, it ends up doing a memberwise copy. Two objects then have the same pointers, and when they both get destroyed, the pointers get deleted twice--a big no-no.

Answer 2

you need to declare copy constructor since you are returning object in overloaded operator +, the compiler automatically generates one for you if you dont explicitly define it, but compiler are stupid enough to not do deep copy on pointers

to summarize your mistake in the code posted:

1.) No Copy-Constructor/Assignment-Operator defined (deallocation exception/ memory leak here)
Since you are dealing with pointers, the compiler generated functions only perform shallow copy.
It is you job to make sure such behavior is intended, otherwise redefine it yourself into :

Dog::Dog(const Dog& ref) :
_name( strdup(ref._name) ), 
_gender( strdup(ref._gender) ), 
_age( ref._age )
{
}

Dog& Dog::operator=(const Dog &dObj)
{
    if (this != &dObj)
    {
        free (_name);
        free (_gender);
        _name = strdup( dObj._name );
        _gender = strdup( dObj._gender );
        _age = dObj._age;
    }
    return *this;
}

2.) Poor handling on pointer passed in (Memory leak here)
You performed allocation before verifying null state on input parameters.
It is smart move to additionally extra allocate 1 char of memory but you do not deallocate them after finding input parameters are null. A simple fix similar to copy-constructor above will be :

Dog::Dog(const char * name, const char * gender, int age) :
_name( strdup(name) ), 
_gender( strdup(gender) ), 
_age( age )
{
}

3.) Improper pairing of allocator/deallocator (Potential memory leak here)
Array allocation with new[] should match with array deallocation delete[], otherwise destructor for array element will not be handled correctly.
However, to be consistent with sample code posted above using strdup (which internally make use of malloc), your destructor should be as below :

Dog::~Dog()
{
    free (_name);
    free (_gender);
    _age = 0;
}

Answer 3

Use std::string and all your problems go away. Afterall you tagged the question as C++ so you should use C++ standard libraries. There's really absolutely no reason why you should not use std::string.

Of course you don't need to use char* allocations because this question isn't homework as it's not tagged as homework. Right?

Answer 4

Another tangential bug: after calling the + operator and the assignment operator, dObj.name and dObj.gender are now null, because the Dog d that you declare as the return value in operator+ was created using the default constructor, and you didn't reassign name or gender before returning the new object.

Answer 5

I think it could be cause:

delete[] name;
delete[] gender;

Answer 6

You're allocating the name & gender with new char[], so you need to deallocate it with delete [] name;. Whenever you allocate an array, you must delete the array.

As the comments noted, save yourself the trouble and use std::string! :)

Answer 7

The problem is in

Dog::Dog(const char * name, const char * gender, int age) :  // allocating here, for data passed in ctor
            name(new char[strlen(name)+1]), gender(new char[strlen(gender)+1]), age(age)

and

Dog::~Dog()
{
    delete name;
    delete gender;
    age = 0;
}

Your new[] needs to be matched with a delete[], not a delete.

Answer 8

Everyone has given the reason for the leak

There is one thing i would like to point out

Dog::~Dog()
{
    delete name;
    delete gender;
    age = 0;
}

There is no need to do age=0, this is useless since the object ceases to exist after the destructor.

Answer 9

You need a copy constructor that deletes the memory on the old object before copying over the new object. The problem is that:

dObj = dObj + dObj2;

The result of the + is overwriting dObj's old char*'s with NULLs (from the returned object in your operator+) without deallocating those char*'s first.

Answer 10

Though it doesn't affect your problem (since you're not dynamically allocating any Dogs) if you're since you're declaring the virtual ~Dog destructor in your in the Dog class, you probably also need to declare a virutual ~Animal destructor in the Animal class, even if it does nothing.

That way, if you:

Animal* a=new Dog();
delete a;

then Dog's destructor will be called.

Answer 11

This isn't the cause of your memory leak, but your Animal class needs to have a virtual destructor. Otherwise, if you were to write:

Animal *dog = new Dog("Spot", "Male", 5);
delete dog;

then you would be leaking memory even after fixing any other problems, because without a virtual destructor in your Animal class, the Dog destructor will never get called to free the memory allocated in the Dog constructor.

This didn't happen in your case because you created your Dogs as automatic variables on the stack, which is good.