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.

templatetypedef · Accepted Answer · 2011-06-01T23:08:30.467

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!

@template: Actually, now that I think about it, I don't quite understand what causes the error. how do two elements of objects_copy end up pointing at the same spot, and what does it have to do with elements of the objects vector pointing at deallocated memory? — Matt Munson, Jun 01 '11 at 22:59
@Matt Munson- I just updated my post to be a bit more technical. Let me know if this helps or if there's something I can try to clarify in more detail. — templatetypedef, Jun 01 '11 at 23:09
@template: Ok, so then the objects in objects_copy are just references to the same memory as the elements of objects? And then changes to that memory effectively changes all of the references to it? — Matt Munson, Jun 01 '11 at 23:30
@Matt Munson- Actually, quite the opposite. The objects in `objects_copy` are completely separate from the objects in `objects`, but the pointers in those objects are pointing at the same resources pointed at by the objects in `objects`. When you're changing the objects in `objects_copy`, you delete the resource pointed at by both those objects and the objects in `objects`, hence the crash. On top of that, because you're populating `objects_copy` with a random sample from `objects`, if you insert the same object twice, you'll end up deleting the same pointer twice for the same reason. — templatetypedef, Jun 01 '11 at 23:50
@template: Did you compile and run this? on my computer it crashes because the assertion fails. But when I take out the line that changes the bools to true, and then compile, its fine. — Matt Munson, Jun 02 '11 at 00:10
The problem is that this is not guaranteed to work at all. You may be pointing at memory that has been deleted and thus is no longer valid. The assertion could be failing because the memory has been reused or overwritten by the memory allocator. This is mostly because (on a second reading) if you add multiple copies of the same element from `objects` to `objects_copy`, you could delete the same memory twice, which could destroy internal heap consistency and cause the memory allocator to fail. — templatetypedef, Jun 02 '11 at 00:16

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

1 Answers1