emplace_back and push_back give 'double free or corruption (fasttop)' error although copy and move constructor are defined

Question

I am just tapping into C++ and probably missing something obvious here. I have a class that dynamically allocates an array and I would like to put its objects into a vector. Since the array has to be freed in the destructor and it is called by vector.push_back() I read that I have to define a copy- as well as move-constructor to avoid memory corruption errors. However, the example below still gives me a double free or corruption (fasttop) error.

#include <iostream>
#include <vector>
using namespace std;

class bla {
  public:
    int* arr;

    bla() {
        arr = new int[10];
        for (size_t i = 0; i < 10; i++) {
            arr[i] = 42;
        }
    }

    bla(bla&& b) : arr(b.arr) {
        cout << "move" << endl;
    }

    bla(const bla& b) : arr(b.arr) {
        cout << "copy" << endl;
    }

    ~bla() {
        cout << "delete" << endl;
        delete[] arr;
    }
};

int main() {
    vector<bla> blas;
    blas.reserve(5000);

    blas.push_back(bla());          // same result with emplace_back
    blas.push_back(bla());          // same result with emplace_back
    blas.push_back(bla());          // same result with emplace_back

    return 0;
}

Could anybody explain what I'm doing wrong and maybe also provide an example of how to add objects with dynamic memory to vectors (I know that I can use pointers, but from what I read it should also work with the objects themselves).

I run g++ (Ubuntu 5.4.0-6ubuntu1~16.04.12) 5.4.0 20160609 with -Wall -std=c++11 -lm

UPDATE 1: Running the same code with the same flags on g++ 7.4.0 does not result in the double free error. Does anybody have an idea why?

UPDATE 2: For posterity, since apparently there are not that many minimal examples implementing the rule of 5 for dynamic arrays floating around. This is how you should do it (please correct me, if I'm wrong):

#include <iostream>
#include <vector>
using namespace std;

class bla {
public:
int* arr;

bla()
    : arr(nullptr)
{
    arr = new int[10];
    for (size_t i = 0; i < 10; i++) {
        arr[i] = 42;
    }
}

~bla() {
    cout << "delete" << endl;
    delete[] arr;
}

// deep copy constructor
bla(const bla& other) {
    cout << "copy" << endl;
    arr = new int[10];
    for (size_t i = 0; i < 10; i++) {
        arr[i] = other.arr[i];
    }
}

// deep copy assignment
bla& operator = (const bla& other) {
    cout << "copy assignment" << endl;
    if (&other != this) {
        delete[] arr;
        arr = new int[10];

        for (size_t i = 0; i < 10; i++) {
            arr[i] = other.arr[i];
        }
    }
    return *this;
}

// move constructor
bla(bla&& other)
    : arr(other.arr) {
    cout << "move" << endl;
    other.arr = nullptr;
}

// move assignment
bla& operator = (bla&& other){
    cout << "move assignment" << endl;
    if(&other != this) {
        delete[] arr;
        arr = other.arr;
        other.arr = nullptr;
    }
    return *this;
}
};

int main() {
    vector<bla> blas;

    cout << "=========\nstart pushs\n=========" << endl;

    blas.push_back(bla());
    blas.push_back(bla());
    blas.push_back(bla());

    cout << endl << "=========\nend pushs\n=========" << endl << endl;;

    cout << "for each:" << endl;
    for (bla b : blas) {
        cout << b.arr[0] << endl;
    }

    cout << endl;
    cout << "native for:" << endl;
    for (size_t i = 0; i < 3; i++) {
        cout << blas[i].arr[0] << endl;
    }

    return 0;
}

Answer 1

I read that I have to define a copy- as well as move-constructor to avoid memory corruption errors.

That is true, but they also need to do the right thing. Not just any copy/move constructor will do the job; and both the copy constructor and the move constructor here have the same flaw. Using the copy constructor as an example:

bla(const bla& b) : arr(b.arr) {

So, if you work out what happens here with a piece of paper and pencil, after copy construction takes place both the new object and this original object, b, will have the same arr pointer. Because that's exactly what this code does.

So, when both objects get destroyed, their destructor will attempt to delete[] the same exact pointer. Both b and the same object have the same arr. There's your double-free.

Same problem occurs with the move constructor.

Also, the shown class does not have an explicit assignment or a move-assignment operator. So, assigning one of these objects to another one will: 1) end up leaking the overwritten object's arr, 2) end up with two objects having the same arr, and the same problem, when they get eventually get destroyed.

In the case of a copy constructor you don't have any option other than making a copy of the copied-from arr, and its values. In the case of a move constructor you do have the option of just setting b.arr to nullptr. The destructor will then quietly ignore delete[]ion of a nullptr; although it's often considered good practice to explicit check for a nullptr, before delete[]ing it.

You will also need to implement a proper operator= overload (just a regular assignment with an optional move-assignment), in order to fix the same problem.