Deleting node in linked list - segmentation fault

Question

Problem requires to delete node from linked list given head pointer of list and the position of node in list to be deleted. More details of question can be found at: https://practice.geeksforgeeks.org/problems/delete-a-node-in-single-linked-list/1

Code returns segmentation fault but not exactly sure where I went wrong. My code is as follows:

Node* deleteNode(Node *head,int x)
{
    //Your code here
    struct Node* temp = head;

    if(x==0){
        //change head
        head = temp->next;
        free(temp);
    }
    //find previous node of node to be deleted
    for(int i=0; temp!=NULL && i<x-1; ++i){
        //traverse
        temp = temp->next;
    }
    //now temp should be pointing to previous node
    //store pointer to next of node to be deleted
    struct Node* next = temp->next->next;
    free(temp->next);
    temp->next= next;
}

Answer 1

Your code contains many places where a null pointer could be dereferenced. For example here:

struct Node* temp = head;

if(x==0){
    //change head
    head = temp->next;
    free(temp);
}

if head was already a null pointer. Furthermore, you seem to be assuming that this changes the head variable in the caller, which is not the case. You would have to pass a double-pointer to achieve this. Lastly, it looks like your function should return after the free(temp);.

Then, looking at this part:

for(int i=0; temp!=NULL && i<x-1; ++i){
    //traverse
    temp = temp->next;
}
//now temp should be pointing to previous node
//store pointer to next of node to be deleted

No, it not necessarily points to a node, temp could also contain a null pointer, which is the second break condition of your for-loop. You have to check for that and bail out if this is the case.

Answer 2

You should check to see if head == nullptr in the very beginning and also before you execute the last 3 lines of code if temp == nullptr because you can exit the for loop because the node the user is trying to remove is beyond the size.

You should also consider returning void from your function.

You can do something like this:

void deleteNode(Node *head,int x)
{
    // this way you avoid dereferencing nullptr with temp->next later on
    if(head == nullptr) return;
    //Your code here
    struct Node* temp = head;

    if(x==0){
        //change head
        head = temp->next;
        free(temp);
    }
    //find previous node of node to be deleted
    for(int i=0; temp!=NULL && i<x-1; ++i){
        //traverse
        temp = temp->next;
    }
    //now temp should be pointing to previous node
    //store pointer to next of node to be deleted

    // you need to check to see if temp is nullptr here or if the node
    // after temp is nullptr because either of those can cause you to 
    // get a segmentation fault because of dereferencing a nullptr
    if(temp == nullptr || temp->next == nullptr) return;
    struct Node* next = temp->next->next;
    free(temp->next);
    temp->next= next;
}