Destructor of class with vector of shared_ptr causes error

Question

I have a class Animal which is base class for few different Animals and a class Herd which stores shared_prt to animals in a vector. I'm unfamiliar with smart pointers but I had to use them in my code in orded to handle inheritance. It seemed to work fine but after my code gets to the destructor of "Herd" it throws an error. What's wrong with it?

class Animal {
public:
    Animal(string _sound) :
        sound(_sound) {}
    void give_sound() {
        cout << sound << " ";
    }
    bool operator==(Animal arg) {
        return (typeid(*this).name() == typeid(arg).name());
    }
protected:
    string sound;
};

class Dog : public Animal {
public:
    Dog() : Animal("woof") {}
};

class Cat : public Animal {
public:
    Cat() : Animal("meow") {}
};

class Cow : public Animal {
public:
    Cow() : Animal("moo") {}
};

class Herd {
public:
    Herd() {}
    ~Herd() {
        vec.clear();
    }

    Herd operator+(Animal *arg) {
        shared_ptr<Animal> ptr(arg);
        vec.push_back(ptr);
        return *this;
    }

    void operator+=(Animal *arg) {
        shared_ptr<Animal> ptr(arg);
        vec.push_back(ptr);
    }


    void make_noise() {
        vector<shared_ptr<Animal>>::iterator v = vec.begin();
        while (v != vec.end()) {
            (*v)->give_sound();
            v++;
        }
        cout << endl;
    }

private:
    vector<shared_ptr<Animal>> vec;
};

int main() {
    Herd herd;
    Dog d1, d2;
    Cat c1, c2;
    cout << "sound 1: " << endl;
    herd.make_noise();
    herd += &d1;
    herd += &c1;
    cout << "sound 2: " << endl;
    herd.make_noise();
    herd += &d2;
    herd += &c2;
    cout << "sound 3: " << endl;
    herd.make_noise();
    //herd = herd - &d1;
    //herd = herd - &d2;
    cout << "sound 4: " << endl;
    herd.make_noise();
    return 0;
}

edit: without vec.clear() it also crashes.

Answer 1

Dog d1, d2;
Cat c1, c2;

Those objects have automatic storage duration. They aren't meant to be managed be owning smart pointers.

The use case for smart pointers is heap allocation, for example:

herd += new Dog;

Answer 2

Your problem is passing the address of variables with Automatic storage duration. See: Stack, Static, and Heap in C++

This is what happens in your code:

You create a variable with automatic storage duration:

Dog d1

it will be automatically destroyed after going out of the scope (in your case end of the main function)

Then, you pass it's address to a function that store this address in a SharedPtr:

Herd operator+(Animal *arg) {
    shared_ptr<Animal> ptr(arg);
    vec.push_back(ptr);
    return *this;
}

doing this you tell the shared_ptr that it is responsible for this object deletion. (In simple words destructor of shared pointer will invoke delete Animal)

As a result your object will be freed twice, and this is forbidden.

Instead of using raw pointers you should use:

operator+(shared_ptr<Animal> arg)

And allocate your objects in following way:

std::shared_ptr<Dog> d1 = std::make_shared<Dog>();

Answer 3

What's wrong with it?

In this code you are trying to create shared_ptr with stack allocated object. This leads to double deletion of this object, the first one happens when stack object goes out of scope. The second one happens in shared_ptr destructor in delete operator. The second one is invalid and program crashes.

Herd operator+(Animal *arg) {
    shared_ptr<Animal> ptr(arg);
    vec.push_back(ptr);
    return *this;
}

void operator+=(Animal *arg) {
    shared_ptr<Animal> ptr(arg);
    vec.push_back(ptr);
}

Answer 4

There are two obvious problems I can see.

The first, as others have mentioned, is that shared_ptr assumes the object it manages was dynamically created (with operator new) so releases it using operator delete (unless a custom deleter is provided when constructing the shared_ptr, which your code doesn't do. Applying operator delete to an object with auto storage duration causes undefined behaviour.

The second problem - which you will eventually encounter after fixing the first - is that class Animal does not have a virtual destructor. Even if the object is created using operator new, operator delete will cause undefined behaviour for objects of actual type derived from Animal (i.e. if the actual objects are of type Cat, Dog, etc).