Two pointers pointing to same memory location and deleting one of those

Question

I have got two different classes in two different threads accessing a pointer to another class. In first thread, this pointer is deleted based on some user input and secodn thread still tries to access that memory location and application crashes at that point. All the data structres that are shared between two threads are protected by required locks and there are no race conditions. I have omitted details related to locks etc here.

Thread1:

class UD
{
public:
char* GetName(); //returns name of UD entry

private:
    //some data
char* name;

}


struct UDentry 
{
        class UD*       mpUd;
        //and other required member variables
};

class ABC
{
    --usual functions

    UD* getUD(const char* name);
    private:
        std::vector<struct UDentry> mUDs;
};

UD* ABC::getUD(const char* name)
{
    if((idx = GetIndex(name)) >= 0)
    {
        return(mUDs[idx].mpUd);
    }
    else
    {
        // o/w create UD and return it
    //............... Create UD, say localUd
    //and add it to mUDs
        if((idx = GetIndex(pFontName)) >= 0)
        {
            return(mUDs[idx].mpUd);
        }       
    }
}

//There is some code to ensure that only one UD with a given name is added to list. I can't change the data structure now, even though i know its not the right data strcuture.

int ABC::GetIndex(const char * pName) const
{
    for(int i = 0; i < mUDs.size(); ++i)
    {
        if((stricmp(mUDs[i].mpUd->GetName(), pName) == 0))
            return i;
    }
    return(-1);
}

//In another thread, there is a list, called DrawList. //It contains pointer to UD and uses it and draws something on the screen based on info in UD

Thread2:

class Item
{
public: 
    Item(char* name, //other args);
private:
    UD* mpUd;
}

Item::Item(char* name, //other args):
mpUd(getUD(name))
{

}

//Stores draw commands
class DrawList
{
public:
    DrawList();
    ~DrawList();

    void AddItem(Item *pItem);

    //method to draw text on screen based on info in UD 

    void Draw();

private:
    DrawList *      mpHead;                     //!< Pointer to the start of the list of blocks containing rendering commands (or NULL if empty)
    DrawList *      mpTail;                     //!< Pointer to the last addtion to the list of block of rendering commands (for easy & fast apends)

    //and some other class specific methods and variables

};


class Render
{
private:
//contains a ptr to DrawList class, another boost::shared_ptr to Drawlist and a mutex to access list
//This class sits in a tight loop, 

}

void DrawList::Draw()
{
        //takes each item in turn and displays some text on-screen based on info in UD

}

---

All this is legacy code, so I don't have the option of chaging ptrs to smart ptrs etc, vast codebase

I added some new functionality to this code. Now, in 1st thread, where UD is created, based on some user action, we might have to delete UD

//new methods
bool ABC::deleteUD()
{
    if((idx = GetIndex(pFontName)) >= 0)
    {
        delete mUDs[idx].mpUd;
        mUDs[idx].mpUd = 0;
        return true;
    }

    return false;
}

//Now, in second thread this UD has already been added in Drawlist many times and Some of the draw nodes still have a ptr pointing to that UD. //All this deleting, adding and rendering on UD is done with a mutex protection, so there are no race conditions.

I had assumed, if I deleted UD in class ABC and made it Null, then the pointer (Item::mpUd), will also become Null. But that pointer is still pointing to that address and memory at that address space has either been re-used or it contains junk value (which is surprising as , I thought I made it Null). SO, when in Draw method, I try to access it, the program crashes.

In Draw, before rendering, I am checking, if(Item::mpUd) -- //This is never 0 { mpUd->GetName() //etc, access its methods. Here I get a crash }

I have looked at this answer to a similar question: C++ deleting a pointer when there are 2 pointers pointing to the same memory locations

So, I know what I have done is wrong. Any suggestions, how I can correctly implement this behavior, I stress, I can't change to shared ptrs

Answer 1

Without a garbage collector, it's up to the programmer to manage object lifetimes. There are patterns to make that job easier, and smart pointers make it easier to implement some of those patterns.

You currently have two objects, we'll call them A and B, and B is responsible for delete-ing something in A. Some of A is valid for all of A's lifetime, but some of A is limited by B's lifetime. Today that's considered a bad idea, and it's almost impossible to get it right. There's a widespread belief that letting B know about A's internals is always a bad idea (Law of Demeter), and while I think that's a good rule of thumb, you don't have to redesign everything to follow it.

The best answer for the problem you're having right now is to simply stop having one object delete something internal to another object. Leave each object in charge of managing the lifetime of its own internals. From the code you posted, there is no reason for B to call delete simply because it no longer wants to use the pointer. You could instead have B NULL out its copy of the pointer, while leaving the original object available for other things (e.g., C) to use. As you've learned, NULL-ing one pointer doesn't affect other pointers to the same thing:

int i = 5;
int* ip = &i;
int* ip2 = &i;
int* ip3 = ip;
int* ip4 = ip2;
ip = NULL; // has no effect on ip2, ip3 or ip4

Answer 2

What shared_ptr gives you is reference counting. So you create a shared_ptr, then copy it to create another one and access your memory through these. When you delete either shared_ptr, the destructor does not free the memory. It only does this when the last shared_ptr is destroyed.

shared_ptrs do not help you if you (somehow) manage to get two independant shared_ptrs pointing at the same memory - but this never happens as the memory in a shared_ptr is always allocated by the shared_ptr, you have strong ownership of the pointer's memory (and you should not think of it in terms of normal C pointer, think of it more as the contents of the shared_ptr object, just like any other object contains data).

So, you can implement this same behaviour externally. Every time you initialise a pointer to the shared memory area, you increment a reference counter. Every time you 'free' it you instead decrement the reference counter. When the reference counter is decremented to 0, then,and only then, do you free the memory.