getting segmentation fault in searching an element in binary search tree in c++

Question

node ** BST :: searchElement(node **tree, int item)
{
    if( ((*tree)->data == item) || ( (*tree) == NULL) )
        return tree;
    else if( item < (*tree)->data)
        return searchElement( &(*tree)->left, item);
    else
       return searchElement( &(*tree)->right, item);
}

int main(){
    BST obj;
    int choice;
    int height=0,total=0,n,item;
    node **tmp;
    system("cls");

    while(1){
        //clrscr();
        cout<<"*****BINARY SEARCH TREE OPERATIONS*****\n\n";
        cout<<"1) Create Tree\n";
        cout<<"2) Traversal\n";
        cout<<"3)  Insert Node\n";
        cout<<"4)  Search Node\n";
        cout<<"5 Find Smallest Node\n";
        cout<<"6) Find Largest Node\n";
        cout<<"7) Exit\n";
        cout<<"Enter your choice : ";
        cin>>choice;
        switch(choice){
            case 1 : //Create Tree
                cout<<"\n\n--Creating Tree--";
                cout<<"\nHow many nodes u want to enter : ";
                cin>>n;
                for(int i=0;i<n;i++){
                    cout<<"Enter value : ";
                    cin>>item;
                    obj.createTree(&obj.tree,item);
                }
                break;

            case 2 : //All Traversals
                cout<<"\n\nInorder Traversal : ";
                obj.inOrder(obj.tree);

                cout<<"\n\nPre-order Traversal : ";
                obj.preOrder(obj.tree);

                cout<<"\n\nPost-order Traversal : ";
                obj.postOrder(obj.tree);
                getch();
                break;

            case 3 : //Inserting a node in a tree
                cout<<"\n\n--Inserting Node in a tree--\n";
                cout<<"Enter value : ";
                cin>>item;
                obj.createTree(&obj.tree,item);
                cout<<"\nItem is inserted\n";
                getch();
                break;

            case 4 : //Search element
                cout<<"\n\n--Search Element--\n";
                cout<<"Enter item to searched : ";
                cin>>item;
                &(*tmp) = obj.searchElement(&obj.tree,item);
                if( (*tmp) == NULL)
                cout<<"\nSearch Element was not Found";
                else
                    cout<<"\nSearch Element was Found";
                getch();
                break;
            case 5 : //Find Smallest Node
                cout<<"\n\nSmallest Node is :  ";
                obj.findSmallestNode(obj.tree);
                getch();
                break;

            case 6 : //Find Largest Node
                cout<<"\n\nLargest Node is :  ";
                obj.findLargestNode(obj.tree);
                getch();
                break;



            case 7: exit(1);
        }//end of switch
    }
}

In the above program, only case 4 is not working correctly when I try to find the particular element in tree. I have included search element member function on the top of the main program. When I was debugging program, I was getting segmentation fault in search element member function especially in if condition. I really don't know what I need to do to come out of this problem. Can anyone please help me to find out why segmentation fault is happening inside search element member function. Let me know if you have any questions regarding this program.

Answer 1

if( ((*tree)->data == item) || ( (*tree) == NULL) )

Should be

if ( ( (*tree) == NULL) || ((*tree)->data == item) )

If *tree actually is null you're dereferencing a null pointer in the first check. Swapping them around will ensure that *tree isn't NULL when you check (*tree)->data - due to Short Circuit Evaluation

Additionally, &(*tmp) should be written as just tmp

Answer 2

You're dereferencing an uninitialized pointer (tmp). You should either allocate memory for it or just skip it's usage (I really can't figure out why you need a temporary NODE** here.)

Answer 3

Here are a couple critiques:
Since you are only searching for a node, you don't need pointers-to-pointers. The only time you need pointers to pointers are when you actually need to modify the parameter. Also, since you are using C++, instead of passing a pp, you should pass a reference: node * &tree. This makes it so you can work with the tree variable without having to dereference it, since the compiler will take care of that for you.

In your if statements, you aren't checking if your left or right pointers are null pointers. I'm not sure if you have sentinel nodes for this, but I'm assuming you don't. With that, I would change your method to this:

node * BST :: searchElement(node *tree, int item)
{
    if(tree->data == item)
        return tree;
    //short circuit if statements
    else if( (tree->left != NULL) && (item < tree->data) )
        return searchElement( tree->left, item );
    else if( (tree->right != NULL) && (item > tree->data) ) //>= for duplicates
        return searchElement( tree->right, item );

    return NULL; //if it isn't found
}

Answer 4

Yes. Indeed as Erik already posted you MUST write

if ( ( (*tree) == NULL) || ((*tree)->data == item) )

instead of

if( ((*tree)->data == item) || ( (*tree) == NULL) )

Because if item is not already in a tree your code will DEFINITELY lead to segfault when trying to dereference NULL pointer.

There is also another (not so obvious) problem - absolutely unnecessary recursion. If you are not doing careful balancing when insert or remove tree nodes you will have at most linear tree height and thus at most linear recursion depth that may easily lead to stack overflow. So you should transform searchElement function to

node** BST::searchElement( node** tree, int item )
{
    while(  ( (*tree) != NULL)  &&  ( (*tree)->data != item )  )
    {
        if( item < (*tree)->data )
        {
            tree = &(*tree)->left;
        }
        else
        {
            tree = &(*tree)->right;
        }
    }

    return tree;
}