1

I am teaching myself c++ templates. I wrote the following code, and I am getting a weird error about a pointer being freed that wasn't allocated. I am guessing that something in my class template constructor isn't actually calling new on the int when I ask for an <int> type of this class. The code is being compiled and run automatically by CodeRunner for mac which I set up to use the clang++ compiler for c++ files.

#include <vector>
#include <iostream>

template <typename T>
class HeapVal
{
    public:
        HeapVal(T val) {ptr = new T(val);}
        ~HeapVal() {delete ptr;}
        T get() {return *ptr;}
    private:
        T* ptr;
};

int main(){
  std::vector< ::HeapVal<int> > vec;
  for(int i = 0; i < 1000; ++i){
    ::HeapVal<int> h(i);
    vec.push_back(h);
  }
  for(int i = 0; i < 1000; ++i){
    std::cout << vec[i].get() << std::endl;
  }
  return( 0 );
}

That code results in the following error either during compilation or execution (looks to me like a run-time kind of error).

Untitled(30214) malloc: *** error for object 0x7f82f24007c0: pointer being freed was not allocated
*** set a breakpoint in malloc_error_break to debug
Run Command: line 1: 30214 Abort trap: 6           ./"$2" "${@:3}"
Cœur
  • 37,241
  • 25
  • 195
  • 267
John St. John
  • 1,542
  • 1
  • 13
  • 22
  • 7
    This might explain your problem well: http://stackoverflow.com/questions/4172722/what-is-the-rule-of-three – chris Mar 21 '12 at 22:32

3 Answers3

6

You're missing a copy constructor and assignment operator for HeapVal. The implication of that is that you'll ultimately end up trying to delete the same memory more than once, because you have multiple pointers to it - that's what causes the crash.

Try something like this:

template <typename T>
class HeapVal
{
public:
    explicit HeapVal(T val)
    :   ptr(new T(val))
    {}

    ~HeapVal()
    {
        delete ptr;
    }

    HeapVal(const HeapVal& rhs)
    :   ptr(new T(*rhs.ptr))
    {}

    HeapVal& operator=(const HeapVal& rhs)
    {
        HeapVal(rhs).swap(*this);
        return *this;
    }

    T get() const
    {
        return *ptr;
    }

private:
    T* ptr;

    void swap(HeapVal& rhs)
    {
        std::swap(ptr, rhs.ptr);
    }
};
Stuart Golodetz
  • 20,238
  • 4
  • 51
  • 80
  • 2
    Oooh yea, the default copy constructor would just make one pointer equal to the other... I guess I didn't realize that copies would be made when I pushed something into the std::vector. – John St. John Mar 21 '12 at 23:09
  • @JohnSt.John: When you pass an object to `std::vector.push_back`, the `std::vector` will copy that object into it's buffer. If it needs to resize teh buffer, then it makes copies of all items from the old buffer to the new bigger buffer. – Mooing Duck Jan 18 '18 at 02:12
2

Anytime a HeapVal is copied (or assigned, for that matter) you'll end up with two HeapVal objects, both containing pointers to the same place. When the first of those is destroyed, it'll delete the pointed-to object. When the second is destroyed, it'll try to delete the same object again -- leading to undefined behavior.

In your case, you were fortunate enough that the run-time library detected and reported the problem.

Googling for "C++ law of the big three" or something similar should yield quite a bit of discussion on the subject.

Jerry Coffin
  • 476,176
  • 80
  • 629
  • 1,111
2

The problem is when u push the HeapVal object you end up doing a shallow copy of the ptr so this is prone to double delete.

You need to override the default copy constructor and assignment operator or you could use smart pointers instead of raw ptr . Boost::shared_ptr maybe an overkill in your case but its something good to learn.

keety
  • 17,231
  • 4
  • 51
  • 56