Dangling Pointer- please verify

Question

Could someone verify and tell me if the following code is valid ? I feel lines 160-162 might be wrong.
I have comments to indicate the line number.
the full code is taken from here C++ Binary Search tree

class BinarySearchTree
{
    private:
        struct tree_node
        {
           tree_node* left;
           tree_node* right;
           int data;
        };
        tree_node* root;

    public:
        BinarySearchTree()
        {
           root = NULL;
        }

        bool isEmpty() const { return root==NULL; }
        void print_inorder();
        void inorder(tree_node*);
        void print_preorder();
        void preorder(tree_node*);
        void print_postorder();
        void postorder(tree_node*);
        void insert(int);
        void remove(int);
};


void BinarySearchTree::remove(int d)
{
    //Locate the element
    bool found = false;
    if(isEmpty())
    {
        cout<<" This Tree is empty! "<<endl;
        return;
    }

    tree_node* curr;
    tree_node* parent;
    curr = root;

    while(curr != NULL)
    {
         if(curr->data == d)
         {
            found = true;
            break;
         }
         else
         {
             parent = curr;
             if(d>curr->data) curr = curr->right;
             else curr = curr->left;
         }
    }
    if(!found)
         {
        cout<<" Data not found! "<<endl;
        return;
    }


         // 3 cases :
    // 1. We're removing a leaf node
    // 2. We're removing a node with a single child
    // 3. we're removing a node with 2 children

    // Node with single child
    if((curr->left == NULL && curr->right != NULL)|| (curr->left != NULL
&& curr->right == NULL))
    {
       if(curr->left == NULL && curr->right != NULL)
       {
           if(parent->left == curr)
           {
             parent->left = curr->right;
             delete curr;
           }
           else
           {
             parent->right = curr->right;
             delete curr;
           }
       }
       else // left child present, no right child
       {
          if(parent->left == curr)
           {
             parent->left = curr->left;
             delete curr;
           }
           else
           {
             parent->right = curr->left;
             delete curr;
           }
       }
     return;
    }

         //We're looking at a leaf node
         if( curr->left == NULL && curr->right == NULL)
    {
        if(parent->left == curr) parent->left = NULL;
        else parent->right = NULL;
                 delete curr;
                 return;
    }


    //Node with 2 children
    // replace node with smallest value in right subtree
    if (curr->left != NULL && curr->right != NULL)
    {
        tree_node* chkr;
        chkr = curr->right;
        if((chkr->left == NULL) && (chkr->right == NULL))
        {
            curr = chkr;  // <----------- line 160
            delete chkr;
            curr->right = NULL; // <------------------ line 162
        }
        else // right child has children
        {
            //if the node's right child has a left child
            // Move all the way down left to locate smallest element

            if((curr->right)->left != NULL)
            {
                tree_node* lcurr;
                tree_node* lcurrp;
                lcurrp = curr->right;
                lcurr = (curr->right)->left;
                while(lcurr->left != NULL)
                {
                   lcurrp = lcurr;
                   lcurr = lcurr->left;
                }
        curr->data = lcurr->data;
                delete lcurr;
                lcurrp->left = NULL;
           }
           else
           {
               tree_node* tmp;
               tmp = curr->right;
               curr->data = tmp->data;
           curr->right = tmp->right;
               delete tmp;
           }

        }
         return;
    }

}

curr and chkr point to the same location. By deleting chkr can the same location still be accessed by curr ? Or is this alright because neither of them were actually allocated any memory using the new statement.

There is something really dodgy about the code. I feel that there is a memory leak too. I am a working professional looking to refresh on my C++ basics. Thank you for any help.

Answer 1

I gave a glance at the code around the region you mentioned. I believe you are correct as this is a bug.

void BinarySearchTree::remove(int d)
{

...

    tree_node* curr;
    tree_node* parent;
    curr = root;

...    

    //Node with 2 children
    // replace node with smallest value in right subtree
    if (curr->left != NULL && curr->right != NULL)
    {
        tree_node* chkr;
        chkr = curr->right;
        if((chkr->left == NULL) && (chkr->right == NULL))
        {
            curr = chkr;
            delete chkr;
            curr->right = NULL;
        }

In this code section, curr and chkr are both declared as a pointer to a tree_node instance. While running curr = chkr, pointer value for chkr is wirtten to curr and thus point curr to instance chkr is pointing to. By delete chkr, the instance is destroyed and garbage collected. Thus curr is now pointing to a non-existed object who is out of life. This IS a dangling pointer by definition, if I remember correctly.

If I'm all correct above and the understanding for that particular chunk, the following is the fix for this:

curr->data = chkr->data;

to replace

       curr = chkr;

Regarding memory leak. Sorry, I did not read the whole code. It appears msram's code should be merely for displaying the correct logic. That is a lot more extra information for that purpose though.

Answer 2

Yes, lines 160-162 contain an error as you have described. He is writing to memory that he has released with delete.