C++ - pointer being freed was not allocated

Question

I am trying to store objects that have pointers members in a std::vector. As far as I understand, when push_back is called, a temporary copy of the passed object is made and sent to the vector internal memory, and then it gets destroyed. Therefore, i wrote the copy constructor as shown below:

class MeltPoint
{
public:
    MeltPoint();
    MeltPoint(b2Vec2* point);
    MeltPoint(b2Vec2* point, Segment* segment, bool intersection);
    MeltPoint(MeltPoint const& copy);
    MeltPoint& operator= (const MeltPoint& m);
    ~MeltPoint();
private:
    b2Vec2* point;
    Segment* segment;
    bool intersection;
};

MeltPoint::MeltPoint()
{
    CCLog("MeltPoint DEFAULT CONSTRUCTOR");
}

MeltPoint::MeltPoint(b2Vec2* point)
{
    CCLog("MeltPoint CONSTRUCTOR");
    this->point = new b2Vec2();
    *(this->point) = *point;
    this->segment = new Segment();
    this->intersection = false;
}

MeltPoint::MeltPoint(b2Vec2* point, Segment* segment, bool intersection)
{
    this->point = point;
    this->segment = segment;
    this->intersection = intersection;
}

MeltPoint::MeltPoint(MeltPoint const& copy)
{
    CCLog("MeltPoint COPY");
    point = new b2Vec2();
    *point = *copy.point;

    segment = new Segment();
    *segment= *copy.segment;
}

MeltPoint& MeltPoint::operator= (const MeltPoint& m)
{
CCLog("MeltPoint ASSIGNMENT");
    *point = *m.point;
    *segment = *m.segment;
    return *this;
}

MeltPoint::~MeltPoint()
{
    CCLog("MeltPoint DESTRUCTOR");
    delete this->point;
    delete this->segment;
}

b2Vec2 (Box2D framework) is a struct that simply holds 2D coordinates

Segment is a custom class:

class Segment
{
public:
    Segment();
    Segment(b2Vec2* firstPoint, b2Vec2* secondPoint);
    ~Segment();

private:
    b2Vec2* firstPoint;
    b2Vec2* secondPoint;
};

Segment::Segment()
{
    CCLog("Segment DEFAULT CONSTRUCTOR");
    this->firstPoint = new b2Vec2(0, 0);
    this->secondPoint = new b2Vec2(0, 0);
}

Segment::Segment(b2Vec2* firstPoint, b2Vec2* secondPoint)
{
    CCLog("Segment CONSTRUCTOR");
    this->firstPoint = firstPoint;
    this->secondPoint = secondPoint;
}

Segment::~Segment()
{
    CCLog("Segment DESTRUCTOR");
    delete firstPoint;
    delete secondPoint;
}

In some function I am populating the vector:

void someFunction()
{
    vector<MeltPoint> randomVertices;
    randomVertices.push_back(MeltPoint(new b2Vec2(190, 170))); //10
    randomVertices.push_back(MeltPoint(new b2Vec2(70, 110))); //9
}

And the final output:

MeltPoint CONSTRUCTOR
Segment DEFAULT CONSTRUCTOR
MeltPoint COPY
Segment DEFAULT CONSTRUCTOR
MeltPoint DESTRUCTOR
Segment DESTRUCTOR
MeltPoint CONSTRUCTOR
Segment DEFAULT CONSTRUCTOR
MeltPoint COPY
Segment DEFAULT CONSTRUCTOR
MeltPoint COPY
Segment DEFAULT CONSTRUCTOR
MeltPoint DESTRUCTOR
Segment DESTRUCTOR
MeltPoint DESTRUCTOR
Segment DESTRUCTOR
test(1074,0xac7d9a28) malloc: *** error for object 0x844fd90: pointer being freed was not allocated
*** set a breakpoint in malloc_error_break to debug
test(1074,0xac7d9a28) malloc: *** error for object 0x844fda0: pointer being freed was not allocated
*** set a breakpoint in malloc_error_break to debug

The errors are raised in the Segment destructor, but I allocated the two pointer members with a new in the constructor. Can you help me please?

Why are you even using `new`? I see no need for that here. The code would work just fine (and be a lot simpler) if you just used values instead of pointers. You wouldn't even need to implement any copy constructor, copy assignment, nor destructor. — R. Martinho Fernandes, Jan 22 '13 at 11:04
Maybe not related to your problem here but the default ctor leaves point and segment uninitailised. This will crash when the dtor is called. — simonc, Jan 22 '13 at 11:06
The diagnostics doesn't match the code. Nowehere in the code does it say "Segment BASE CONSTRUCTOR". This just wastes our time. Take the time and effort to be sure that the code and the output match. — David Heffernan, Jan 22 '13 at 11:07
Way to many pointers. As @R.MartinhoFernandes said, store values. Your life will be much easier. — Pete Becker, Jan 22 '13 at 11:16

score 4 · Accepted Answer · edited May 23 '17 at 12:04

Segment violates the rule of three. It lacks user-defined copy constructor and copy assignment operator. Whenever you make a copy of one, you end up with a double deletion.

One fix could be following the rule of three and writing the copy constructor and copy assigment operator. But I won't recommend that. I will recommend following the rule of zero instead. There is no need for custom destructors or custom copy constructors anywhere. Just give up on the idea of using dynamic memory allocation for naught.

class MeltPoint
{
public:
    MeltPoint();
    MeltPoint(b2Vec2 const& point);
    MeltPoint(b2Vec2 const& point, Segment const& segment, bool intersection);

private:
    b2Vec2 point;
    Segment segment;
    bool intersection;
};

MeltPoint::MeltPoint(b2Vec2 const& point)
: point(point), segment(), intersection(false) {}

MeltPoint::MeltPoint(b2Vec2 const& point, Segment const& segment, bool intersection)
: point(point), segment(segment), intersection(intersection) {}

class Segment
{
public:
    Segment();
    Segment(b2Vec2 const& firstPoint, b2Vec2 const& secondPoint);

private:
    b2Vec2 firstPoint;
    b2Vec2 secondPoint;
};

Segment::Segment()
: firstPoint(0, 0), secondPoint(secondPoint) {}

Segment(b2Vec2 const& firstPoint, b2Vec2 const& secondPoint)
: firstPoint(firstPoint), secondPoint(secondPoint) {}

void someFunction()
{
    vector<MeltPoint> randomVertices;
    randomVertices.push_back(MeltPoint(b2Vec2(190, 170))); //10
    randomVertices.push_back(MeltPoint(b2Vec2(70, 110))); //9
}

Thank you for your help. In fact i don't really know when i need to use dynamic allocation, thus I think i overuse it. I'll watch some documentation about this ;) — olma, Jan 22 '13 at 11:29
If I just use the vector to save primitive types, i.e. int, class pointers, there will be no risk of violation of Rule of three, right ? — deepsky, Jan 28 '16 at 09:15

score 0 · Answer 2 · edited Jan 22 '13 at 13:48

Yes, I do agree that the missing copy constructor & assignment operator were the root cause of the problem. “The rule of zero” does solves the problem.

We may want to construct objects on heap(specially if the class like segment is a heavy object in terms of memory layout). In that case, using smart pointer would be a good idea. This will also take care of memory de-allocation. This also satisfies "The rule of zero”

The above example is solved using smart pointers:

void CCLog(const char* const X)
{
    std::cout << X << endl;
}

struct b2Vec2 {};

class Segment
{
public:
    Segment();
    Segment(b2Vec2* firstPoint, b2Vec2* secondPoint);
    ~Segment();

private:
    std::shared_ptr<b2Vec2> firstPoint;
    std::shared_ptr<b2Vec2> secondPoint;
};

class MeltPoint
{
public:
    MeltPoint();
    MeltPoint(b2Vec2* point);
    MeltPoint(b2Vec2* point, Segment* segment, bool intersection);
    MeltPoint(MeltPoint const& copy);
    MeltPoint& operator= (const MeltPoint& m);
    ~MeltPoint();
private:
    std::shared_ptr<b2Vec2> point;
    std::shared_ptr<Segment> segment;
    bool intersection;
};

MeltPoint::MeltPoint()
{
    CCLog("MeltPoint DEFAULT CONSTRUCTOR");
}

MeltPoint::MeltPoint(b2Vec2* point)
{
    CCLog("MeltPoint CONSTRUCTOR");
    this->point = std::make_shared<b2Vec2>();
    this->point.reset(point);

    this->segment = std::make_shared<Segment>();
    this->intersection = false;
}

MeltPoint::MeltPoint(b2Vec2* point, Segment* segment, bool intersection)
{
    this->point = std::make_shared<b2Vec2>();
    this->point.reset(point);

    this->segment = std::make_shared<Segment>();
    this->segment.reset(segment);
    this->intersection = intersection;
}

MeltPoint::MeltPoint(MeltPoint const& copy)
{
    CCLog("MeltPoint COPY");
    this->point = copy.point;
    this->segment = copy.segment;
    this->intersection = copy.intersection;

}

MeltPoint& MeltPoint::operator= (const MeltPoint& m)
{
    CCLog("MeltPoint ASSIGNMENT");
    point = m.point;
    segment = m.segment;
    return *this;
}

MeltPoint::~MeltPoint()
{
    CCLog("MeltPoint DESTRUCTOR");
}



Segment::Segment()
{
    CCLog("Segment DEFAULT CONSTRUCTOR");
    this->firstPoint = std::make_shared<b2Vec2>();
    this->secondPoint = std::make_shared<b2Vec2>();
}

Segment::Segment(b2Vec2* firstPoint, b2Vec2* secondPoint)
{
    CCLog("Segment CONSTRUCTOR");
    this->firstPoint = std::make_shared<b2Vec2>();
    this->firstPoint.reset(firstPoint);

    this->secondPoint = std::make_shared<b2Vec2>();
    this->secondPoint.reset(secondPoint);
}

Segment::~Segment()
{
    CCLog("Segment DESTRUCTOR");
}

int _tmain(int argc, _TCHAR* argv[])
{
    vector<MeltPoint> randomVertices;
    randomVertices.push_back(MeltPoint(new b2Vec2())); //10
    randomVertices.push_back(MeltPoint(new b2Vec2())); //9
    return 0;
}

C++ - pointer being freed was not allocated

2 Answers2