STL vector push_back() memory double free

Question

Possible Duplicate:
What is The Rule of Three?

I have the a problem of the double freeing of memory in the following program.

The debugger shows that the issue is in the push_back() function.

Class A:

class A {
    public:
        A(int x);
        int x;
};

A::A(int x) {
    this->x = x;
}

Class B:

class B {
    public:
        B(int x);
        ~B();
        A* a;
};

B::B(int x) {
    this->a = new A(x);
}

B::~B() {
    delete a;
}

Main function:

int main() {
    vector<B> vec;

    for(int i = 0; i < 10; i++) {
        vec.push_back(B(i)); <------------ Issue is here
    }

    cout << "adding complete" << endl;

    for(int i = 0; i < 10; i++) {
        cout << "x = " << (vec[i].a)->x << endl;
    }

    return 0;
}

What is wrong in this code?

EDIT: Error double free or memory corruption

http://stackoverflow.com/questions/4172722/what-is-the-rule-of-three — Benjamin Lindley, Nov 12 '12 at 16:12
See http://stackoverflow.com/questions/4172722/what-is-the-rule-of-three — hmjd, Nov 12 '12 at 16:12

score 6 · Answer 1 · answered Nov 12 '12 at 16:12

You forgot to define a copy constructor and copy assignment operator, so your wrapped object is being deleted by some B.... then again when some copy of B goes out of scope.

In this case it's the B(i) temporary on the line you've identified, as well as an implementation-defined number of copies within the vector.

Abide by the rule of three.

score 4 · Answer 2 · answered Nov 12 '12 at 16:12

4

The problem in your code is due to the fact that "plain" C/C++ pointers have no concept of ownership. When a pointer gets copied, both copies^* "think" that they own the data, leading to double-deletion.

In recognition of this fact, the designers of the C++ standard library introduced a unique_ptr<T> class that helps you address problems like that.

^* One copy of the pointer is in the instance of B passed to push_back; the other copy of the pointer is in the instance entered into the vector.

answered Nov 12 '12 at 16:12

Sergey Kalinichenko

714,442
84
1,110
1,523

2

In C++11, as well as fixing the class `B` using `unique_ptr`, you can write `vec.emplace_back(i)` instead of `vec.push_back(B(i))`. But that's an aside, even with that change you do still need to fix `B`, one way or another. – Steve Jessop Nov 12 '12 at 16:55

score 2 · Accepted Answer · answered Nov 12 '12 at 19:30

Heed the Rule of Three

Everyone else has already harped on this so I won't dive further.

To address the usage you are apparently trying to accomplish (and conform to the Rule of Three in the process of elimination), try the following. While everyone is absolutely correct about proper management of ownership of dynamic members, your specific sample can easily be made to avoid their use entirely.

Class A

class A {
    public:
        A(int x);
        int x;
};

A::A(int x) 
   : x(x)
{
}

Class B

class B {
    public:
        B(int x);
        A a;
};

B::B(int x) 
    : a(x) 
{
}

Main Program

int main() {
    vector<B> vec;

    for(int i = 0; i < 10; i++) {
        vec.push_back(B(i));
    }

    cout << "adding complete" << endl;

    for(int i = 0; i < 10; i++) {
        cout << "x = " << vec[i].a.x << endl;
    }

    return 0;
}

Bottom Line Don't use dynamic allocation unless you have a good reason to, and it is lifetime-guarded by contained variables such as smart pointers or classes that vigorously practice the Rule of Three.

STL vector push_back() memory double free

3 Answers3

Heed the Rule of Three