0
votes

Segmentation faults when trying to delete a node with 2 children

So i've recently started studying algorithms and have been working a bit on binary search trees, basing my C++ code on the pseudo code taken from the book Introduction to Algorithms. I'm however having large issues with it's delete function for it's binary search tree. The code for the delete function is:

void bstDelete(BSTtree &bst, int key)
{
    treeNode *z = findNode(bst, key);
    if (z != nullptr)
    {
        treeNode *y = z;
        treeNode *x;
        if (z->left == bst.NIL)
        {
            x = z->right;
            transplant(bst, z, z->right);
        }
        else if (z->right == bst.NIL)
        {
            x = z->left;
            transplant(bst, z, z->left);
        }
        else
        {
            y = treeMin(z->right);
            x = y->right;
            if (y->parent == z)
            {
                x->parent = y;
            }
            else
            {
                transplant(bst, y, y->right);
                y->right = z->right;
                y->right->parent = y;
            }
            transplant(bst, z, y);
            y->left = z->left;
            y->left->parent = y;  
        }
    }
}

Transplant function:

void transplant(BSTtree &bst, treeNode *u, treeNode *v)
{
    if (u->parent == bst.NIL)
    {
        v->right = u->right;
        v->left = u->left;

        v->right->parent = v;
        v->left->parent = v;
        bst.root = v;
    }
    else if (u == u->parent->left)
    {
        u->parent->left = v;
    }
    else
    {
        u->parent->right = v;
    }
    v->parent = u->parent;
}

I'm not expecting someone to debug my code, I've just sat pretty much for multiple days trying to figure it out, looking at other people's versions of the delete function trying to understand why mine doesn't work. There's also the issue of why the book's own delete function doesn't do it's job, or if it's just me who's at fault. I would really appreciate if someone could take a look at this. I've been looking around quite a lot and still haven't found anyone having issues with this.

Edit
Added other parts related to the code

treeNode *findNode(RBtree bst, int key)
{
    treeNode *z = nullptr;
    treeNode *x = bst.root;
    treeNode *y = bst.NIL;
    while (x != nullptr && x != bst.NIL)
    {
        y = x;
        if (key < x->value)
        {
            x = x->left;
        }
        else
        {
            x = x->right;
        }

        if (y->value == key)
        {
            break;
        }
    }
    if (y->value == key)
    {
        z = y;
    }
    return z;
}
treeNode *treeMin(treeNode *root)
{
    treeNode *treeNode = root;
    while (treeNode->left != nullptr)
    {
        treeNode = treeNode->left;
    }
    return treeNode;
}
struct treeNode
{
    treeNode *left;
    treeNode *right;
    treeNode *parent;
    std::string color;
    int value;
    treeNode() : left(nullptr), right(nullptr), parent(nullptr) {}
    treeNode(int paramValue) : left(nullptr), right(nullptr), parent(nullptr), value(paramValue) {}
    treeNode(int paramValue, treeNode *parent) : value(paramValue), parent(parent) {}
};
struct BSTtree
{
    treeNode *root;
    treeNode *NIL;
    BSTtree()
    {
        NIL = new treeNode();
        root = nullptr;
    }
};

Reason i have with NIL is so that i can easier make this into an RB tree later on. The smallest scale needed to cause a crash is a 3 node tree, where you have a root, and it has a right and left child. That way you push the delete function into case three, where it has to find a successor and transplant it. It's in this part of the delete-function which it crashes and gives me a segmentation-fault.

Edit 2
Code which causes a crash:

int main()
{
    BSTtree bst;
    bstInsert(bst, 5);
    bstInsert(bst, 7);
    bstInsert(bst, 2);


    bstDelete(bst, 5);
}

Insert function

void bstInsert(BSTtree &bst, int key)
{
    treeNode *z = new treeNode(key);
    treeNode *y = bst.NIL;
    treeNode *x = bst.root;
    while (x != nullptr && x != bst.NIL)
    {
        y = x;
        if (z->value < x->value)
        {
            x = x->left;
        }
        else
        {
            x = x->right;
        }
    }
    z->parent = y;
    if (y == bst.NIL)
    {
        bst.root = z;
        z->parent = bst.NIL;
    }
    else if (z->value < y->value)
    {
        y->left = z;
    }
    else
    {
        y->right = z;
    }
    z->left = bst.NIL;
    z->right = bst.NIL;
}

The nullptr is not a problem here as it's resolved in the insert function, as you can see if it's root, root is given bst.NIL as it's parent. I agree it's error prone to have this construction, it's only there to be able to be used for red and black trees which uses a specific NIL node instead of nullptr.

1
What's the smallest scenario where you see the crash? Can you repro it with a 1-node BST? Also let's see the definition of BSTtree and findNode() (and anything else that is referred to in the code you've shown so far).j_random_hacker
@j_random_hacker i've updated the post with more code and instructions on how to cause a segmentation fault with the code given. You need at least a 3-node BST to cause the segmentation fault, where you have the root and it's 2 children, then you simply have to try and delete the root and it should segfault. Hope this information comes to aid. Anyways, thank you for your help :)Dingus
Thanks -- what about treeNode?j_random_hacker
When you call transplant(bst, z, z->right);, if z is your tree root, here : v->right = u->right; v->right is v itself? And v->right->parent = v; does the same thing?HangrY
Also struct BSTtree seems to have something that looks like a constructor, but it's named RBtree.j_random_hacker

1 Answers

0
votes

I've resolved the issues, firstly the treeMin() function is faulty. It should take in an additional parameter which is BSTtree bst and instead of while (treeNode->left != nullptr) it should be while (treeNode->left != bst.NIL), outside of this one of the constructors in treeNode is faulty, this one being treeNode(int paramValue, treeNode *parent) : value(paramValue), parent(parent) {}, it should instead be changed into:

treeNode(int paramValue, treeNode *paramParent) {
  value = paramValue;
  parent = paramParent;
}