delete[] pointer being freed was not allocated

Question

I am using the new operator to create a dynamically allocated array (I am using this one because I want to save on the memory overhead of using a vector). The error occurs in the destructor, saying the pointer being freed was not allocated although obviously it was. The constructors and destructors are as follows:

~Path() {
    printf("Path Destructor\n");
    if(points) {
        delete[] points;
    }
}
Path(const std::vector<PathPoint>& points_) {
    size = points_.size();
    points = new PathPoint[size];
    int i = 0;
    for(const PathPoint& p : points_) {
        points[i++] = p;
    }
    printf("Path created\n");
}

Did you define a copy constructor and or a copy assignment operator ? — quantdev, Oct 10 '14 at 12:13
@Niall It differs, size is from 1-5 usually. It crashes for 2 in my case, but I don't think it matters. There is no default constructor. — Gerard, Oct 10 '14 at 12:13
Think about it : what happens if a `Path` is copy constructed (or copy assigned) and then destructed ? — quantdev, Oct 10 '14 at 12:14
Yes, the copy and assignment would matter here. Read up on the "rule of three", or with C++11 the "rule of zero (or five)" (RAII). — Niall, Oct 10 '14 at 12:14
@Gerard Why not simply use a `std::vector` as your class member `points`? — πάντα ῥεῖ, Oct 10 '14 at 12:15
@πάνταῥεῖ "Memory overhead" its in the question (see my comment). — Borgleader, Oct 10 '14 at 12:16
@πάντα ῥεῖ I thought the overhead of a vector was 24 bytes, atleast that is what I get when I use sizeof(someVector). I thought this was a bit much for a vector that usually only contains 1-5 elements, and I have > 1 million of paths. — Gerard, Oct 10 '14 at 12:17
@Gerard You probably have a misconception what `sizeof(someVector)` actually does. It's the plain size of the `std::vector` class itself, not for the contained elements. — πάντα ῥεῖ, Oct 10 '14 at 12:19
Do you `delete[] points` at any other location in the code? Or do you perhaps copy the `points` pointer and then `delete[]` the copy; maybe by copying the whole `Path` object (as others have hinted), or maybe by returning it through some public function to someone who later `delete`s it. — dlf, Oct 10 '14 at 12:20
@πάνταῥεῖ Yes I know, so that is the overhead of using the vector right? It's some of its internal memory used (I imagine such as a size_t for keeping track of the size etc.) — Gerard, Oct 10 '14 at 12:20
Gerard's right πάνταῥεῖ. For example, VC++2005 `vector` keeps 3 pointers (begin, end, capacity-extends-to), while it sounds like Gerard only need begin and a `uint8_t` size. @Gerard - what's the size of `PathPoint`? — Tony Delroy, Oct 10 '14 at 12:38

score 3 · Accepted Answer · edited May 23 '17 at 12:27

3

You have to apply The Rule of Three :

The C++ standard says that :

The implicitly-defined copy constructor for a non-union class X performs a memberwise copy of its subobjects. [n3126.pdf section 12.8 §16]

The implicitly-defined copy assignment operator for a non-union class X performs memberwise copy assignment of its subobjects. [n3126.pdf section 12.8 §30]

So the implicitly-defined copy constructor and copy assignment operator for your Path class will not call new[] for you.

Define a copy constructor and a copy assignment oerator that perform the required allocation.

Note:

You can also make your type non copyable, declare them without definition :

E.g. :

 Path( const Path& other );      // non construction-copyable
 Path& operator=( const Path& ); // non copyable

(or use boost::noncopyable)

The typical overhead of a std::vector<> is very very low, there are few contexts where it really matters : use it as much as you can to avoid such problems.

edited May 23 '17 at 12:27

Community

1
1

answered Oct 10 '14 at 12:20

quantdev

23,517
5
55
88

1

Either that or *hide* the copy constructor/assignment operator, find out where the code is making copies of your millions of `Path`s, and stop it from doing that. – dlf Oct 10 '14 at 12:23
@dlf : yep, note added – quantdev Oct 10 '14 at 12:26
@dlf, That is good advice, it seems somewhere `paths.push_back(Path(points));` was calling the copy constructor and not allocating the memory like quantdev said. Thank you both. – Gerard Oct 10 '14 at 12:27
@Gerard Yes, that will do it. You can use `paths.emplace_back(points)` to avoid the copy. You may also want to add a move constructor to `Path` to handle other scenarios. – dlf Oct 10 '14 at 12:27
By the way, I have always used vectors before but I think in this case the overhead reduction would be worth it, seeing that sizeof(vector) is 24 bytes, mine would be 2 + sizeof(PathPoint) * size. Or am I missing something?. Also my paths are relatively small <5, but the number of paths is huge so the small overhead is multiplied a great deal. – Gerard Oct 10 '14 at 12:28
@dlf Ah right, well its good to adhere to this rule of three nevertheless :). Thanks – Gerard Oct 10 '14 at 12:29
2

@Gerard Yes, if you're really dealing with millions of the things it's possible that it's worth the extra effort to avoid that otherwise-negligible 24-byte overhead. But if you're really bumping into memory limits, you could do even better; e.g. by putting all the points for all the paths into one giant vector and keeping a separate vector the gives the starting index for each one. – dlf Oct 10 '14 at 12:32

delete[] pointer being freed was not allocated

1 Answers1