Writing a copy constructor for a class gives an unexpected crash while deleting object in the destructor

Question

Writing a copy constructor for a class gives an unexpected crash while deleting object in the destructor. See following codes:

class ordinaryClass  // Sample class is used in Foo class
{
public:
    ordinaryClass(){}
};

class Foo
{
public:
    Foo():_ordinarayClass(0)
    {
        _ordinarayClass = new ordinaryClass();
    }

    ~Foo()
    {
        delete _ordinarayClass; // the program crashes here when copy constructor is called.
    }

    Foo(const Foo &obj) // copy constructor
    {
        *_ordinarayClass = *obj._ordinarayClass;
    }

    Foo& operator=(const Foo& other) // copy assignment
    {
        *_ordinarayClass = *other._ordinarayClass;
        return *this;
    }

    ordinaryClass *_ordinarayClass;
};

In the main, If I write these codes:

Foo container2;
Foo container;
container = container2;

The program runs normally and exits normally. But if I write it in the following way:

Foo container2;
Foo container = container2;

The program crashes while exites. I found that the program crashes at the destructor of Foo class. Have I made any mistakes in the copy constructor? Thanks a lot.

Answer 1

Your needs objects. Don't just copy pointers. I'll save the "you should be using smart pointers for this" speech, since any time spent on this site will leave you with that tattooed on your eyelids in short-order (and you should, btw use smart pointers =P)

class Foo
{
public:
    Foo() : _ordinaryClass(new ordinaryClass())
    {
    }

    virtual ~Foo()
    {
        delete _ordinaryClass;
    }

    Foo(const Foo &obj) : _ordinaryClass(new ordinaryClass(*obj._ordinaryClass))
    {
    }

    // copy/swap idiom for assignment. the value-parameter is
    //  intentional. think about what it does (hint: copy-ctor)
    Foo& operator=(Foo other)
    {
        this->swap(other);
        return *this;
    }

private:
    ordinaryClass *_ordinaryClass;

    // they get our cruft, we get theirs.
    void swap(Foo& other)
    {
        std::swap(_ordinaryClass, other._ordinaryClass);
    }
};

I would seriously reconsider whether that member should be dynamic in the first place, btw.