C++, 'coupling' of member pointers in multiple objects copied from the same original object

Question

#include <iostream>
#include <vector>
#include <cstdlib>
#include <cassert>

struct s_A {
    bool bin;
    s_A(): bin(0) {}
};

class c_A {
public:
    s_A * p_struct;

    c_A(): p_struct(NULL) {p_struct = new s_A [16];}

    void Reset()
    {
        delete [] p_struct;
        p_struct = new s_A [16];
    }
};

int main () 
{   
    srand(1);
    int x = 30;
    std::vector <c_A> objects;
    objects.assign(x, c_A());
    std::vector <c_A> objects_copy;

    for(int q=0; q < x; q++)
    {
        objects_copy.push_back(objects[ rand() % x ]);
        objects_copy[q].Reset();
    }

    for(int q=0; q < 16; q++)
        for(int w=0; w < x; w++)
        {
            // Assertion should not fail, but it does
            assert(!objects_copy[w].p_struct[q].bin);
            objects_copy[w].p_struct[q].bin = true;
        }
}

Somehow the pointers in the different copied objects end up pointing to the same memory, and the assertion eventually fails. This behavior does not happen if run on the un-copied vector.I thought that c_A.Reset() should free up the pointers (via delete[]) to point to a new array, but I'm obviously missing something.

Answer 1

The particular source of your problem are these lines here:

objects_copy.push_back(objects[ rand() % x ]);
objects_copy[q].Reset();

The issue is that when you try pushing copies of the objects into objects_copy, you end up making a shallow copy of the objects in the objects vector. This means that the objects in the two vectors will end up having pointers that are copies of one another. Consequently, when you invoke Reset on the elements of the objects_copy vector, you deallocate memory that is still being pointed at by by elements of the objects array.

The problem is that your c_A class violates the rule of three. Because your class encapsulates a resource, it needs to have a destructor, copy constructor, and copy assignment operator. If you define these three functions, then when you try copying the objects into the objects_copy vector, you will have the ability to manage the underlying resource, perhaps by duplication or by reference-counting. For details on how to write these functions, check out this description of how to write these functions.

EDIT: Here's a more detailed description of what's going on:

The issue is that when you add an object to a vector, you aren't actually storing that object in the vector. Rather, you're storing a copy of that object. Thus when you write objects_copy.push_back(objects[ rand() % x ]);, you are not storing the same object in both vectors. Instead, you're creating a copy of one of the objects from objects and storing it in objects_copy. Since your c_A type does not have copy functions defined, this ends up making a shallow-copy of the object, which creates a copy of the pointer. This means that if you think about the original object from the objects list and its corresponding copy in objects_copy, they will each have a copy of the same p_struct pointer. When you call Reset on the object in the objects_copy vector, you free the memory pointed at by its pointer. However, you did not update the pointer of the original object stored in objects, and so that pointer now refers to garbage memory. Trying to use that pointer leads to undefined behavior, hence the crash.

Adding copy functions will fix this by allowing you to control how the copy is being made. If you define a copy function for c_A that causes the copy to point to a new copy of the object pointed at by the original, then this problem won't occur because each object will have its own separate pointer. Alternatively, if you use reference counting, then you can avoid the problem by not deleting the resource if you know that some other object points at it.

Hope this helps!