Avoid double free when deleting a vector of pointers

Question

I have tried the following, but getting segmentation fault and "double free detected in tcache2" during runtime:

class A {
... 
};

class B {
    public:
        B(...) {...}
        ~B() {
        for(int i=0;i<As.size();i++) {
            if(As[i] != nullptr) {
                delete As[i];
                As[i] = nullptr;
                }
    
            }
        }
        std::vector<A*> As;
};

int main()
{   
    A* a1 = new A(...);
    A* a2 = new A(...);
    
    B* b1 = new B(...);
    b* b2 = new B(...);
    
    b1.As.push_back(a1);
    b1.As.push_back(a2);
    
    b2.As.push_back(a1);

    delete b1;
    delete b2;
    return 0;
}

Is there any way I could assure that I delete these pointers only once?

`delete As[i]; As[i] = nullptr;` will only change the value of `As[i]` for that particular `B` object. Any other `B` objects that included the previous value of `As[i]` will still have that, but when they try to `delete` it it'll be double deletion. Suggestion: Change `As` to a `std::vector>` and then you don't need to write a `B` destructor at all. — Nathan Pierson, May 21 '22 at 14:36
Another obvious way would be to ensure no `A *` is ever stored in more than one object of type `B` at a time (since, when each `B` is destructed, any `A*`s stored by both will be `delete`d by both). One way of handling this is for `B` not to have any member functions that accept an `A*`. Instead, have `B` provide a non-static function that accepts necessary data to construct an `A` (e.g. values passed to `A`s constructor) so that function creates the object, stores it in the current `B`, and doesn't share with other `B`s. — Peter, May 21 '22 at 14:49
You have stumbled across the concept of [ownership](https://stackoverflow.com/questions/49024982/what-is-ownership-of-resources-or-pointers). Ultimately a resource, in this case a dynamic allocation, can have only one owner, and both `b1` and `b2` have claimed ownership of the allocation pointed at by `a1`. — user4581301, May 21 '22 at 14:50
Future bug: If `B` is going to directly own resources it must also ensure that the resource is copied or it's ownership transferred correctly. See the [Rules of Three and Five](https://en.cppreference.com/w/cpp/language/rule_of_three) for the usual solution to this aspect of ownership. — user4581301, May 21 '22 at 14:53
You could use smart pointers here for automatically managing the pointer so you don't have to free manually. — codingwith3dv, May 21 '22 at 17:07
What's the point of setting `As[i]` to `nullptr` given that you're in the destructor and the entire `As` vector is about to be destroyed anyway? — David Schwartz, May 21 '22 at 23:43

score 1 · Answer 1 · answered May 21 '22 at 22:33

Okay, you're storing the same pointer in two arrays. So yes, you're deleting it twice. This is why the use of raw pointers has become discouraged in modern C++. Use smart pointers instead.

std::unique_ptr<A> a1 = std::make_shared<A1>();

After that, you can use them just like any other pointer, except the type is different and you do NOT have to delete them. They reference count. Just set the value to nullptr if you want.

a1 = nullptr;  // Will free if this is the last copy.

Avoid double free when deleting a vector of pointers

1 Answers1