My destructor is causing my program to crash

[JonnyG]

Below is my code. The include directives and the using std::* declarations have been removed for brevity:

class HasPtr {
    friend void swap(HasPtr&, HasPtr&);
public:
    HasPtr(const std::string &s = string()): ps(new string(s)), i(0) {}
    HasPtr(const string &s, const int &j) : ps(new string(s)), i(j) {}
    HasPtr(const int &j) : ps(new string()), i(j) {}
    HasPtr(const HasPtr &hp) : ps(hp.ps), i(hp.i) {} //copy constructor
   
    //copy-assignment operator
    HasPtr& operator=(HasPtr rhs) {
        swap(*this, rhs);
        return *this;
    }

    bool operator<(const HasPtr &rhs) {
        return this->i < rhs.i;
    }  

    //destructor
    ~HasPtr() {
        delete ps;
    }
       
    std::string *ps;
    int i;
};

inline void swap(HasPtr &hp1, HasPtr &hp2) {
    using std::swap;
    swap(hp1.ps, hp2.ps);
    swap(hp1.i, hp2.i);
}

int main() {
    HasPtr p1(32), p2(44);
    vector<HasPtr> v {p1, p2};
}

Whenever I run this program, it crashes. I ran my debugger and I also ran a memory-checking program on the compiled executable and it looks as if my HasPtr destructor is causing problems. It appears to me that when I initialize my vector, it makes a local copy of p1 and p2 and when the initializing is done, it runs my destructor on these local copies of p1 and p2, resulting in the other p1 and p2 objects (the original ones in the main scope) containing dangling pointers and thus crashing the program.

My destructor must delete the memory pointed to by the object, because otherwise once my object is deleted, the pointer will be deleted but the actual memory itself won't be freed, so I will have a memory leak.

Given this, how can I create a vector of HasPtr objects? Every method for putting HasPtr objects into the vector results in temporaries being created, which seems to be what is causing the problem in the first place.

EDIT: This class is the result of the exercises in my book, and it is required that I use a built-in pointer in this exercise, so I cannot use a smart pointer to replace the built-in pointer.

 

[Filip Larsen]

There is a clear issue in your copy constructor which, when used, will lead to the bug you see.

 

[JonnyG]

There is a clear issue in your copy constructor which, when used, will lead to the bug you see.

I literally just came into this thread to make an update that I figured it out. I changed,

ps(hp.ps)

to

ps(new string(*hp.ps))

 

[jbunniii]

Using smart pointers avoids problems with mismatched new/delete.

#include <memory>
#include <string>
#include <vector>

class HasPtr {
    friend void swap(HasPtr&, HasPtr&);
public:
    HasPtr(const std::string &s = std::string()): ps(std::make_unique<std::string>(s)), i(0) {}
    HasPtr(const std::string &s, const int &j) : ps(std::make_unique<std::string>(s)), i(j) {}
    HasPtr(const int &j) : ps(std::make_unique<std::string>()), i(j) {}
    //copy constructor
    HasPtr(const HasPtr &hp)
    : i(hp.i)
    {
        if (hp.ps != nullptr) {
            ps = std::make_unique<std::string>(*hp.ps);
        }
    }

    //copy-assignment operator and operator< unchanged from raw pointer implementation:
    HasPtr& operator=(HasPtr rhs) {
        swap(*this, rhs);
        return *this;
    }

    bool operator<(const HasPtr &rhs) {
        return this->i < rhs.i;
    }

    //destructor
    ~HasPtr() {
        // nothing to do, unique_ptr destroys the object it points to
    }

    std::unique_ptr<std::string> ps;
    int i;
};

Or just avoid the whole mess by making the member variable a std::string instead of a pointer to a std::string.