CODEWITHSUNDEEP
cdebuggingsegmentation-fault
DanSchneiderNA
DanSchneiderNA

Reputation: 378

Linked List making segmentation fault

Below I have made a simple Linked List in C. The code is currently producing a segmentation fault which I find odd because I was copying an example from our current book. The only thing I did to the code was put the code into the method "addToList". I'm aware the segmentation fault is coming from the method addToList but I do not know where I made a mistake.

#include <stdio.h>
#include <stdlib.h>
typedef struct node {
  int val;
  struct node *next;
} Node;

void addToList(Node *, int);
void printList(Node *);

void main() {
  int x;
  Node *head = malloc(sizeof(Node));
  for (x = 1; x < 4); x++) {
    printf("Enter an integer: ");
    x = scanf("%d");
    addToList(head, x);
  }
  printList(head);
}

void addToList(Node *head, int val) {
  Node *current = head;

  while (current->next != NULL) {
    current = current->next;
  }

  current->next = malloc(sizeof(Node));
  current->next->val = val;
  current->next->next = NULL;
}

void printList(Node *head) {
  Node *current = head;

  while (current != NULL) {
    printf("%d->", current->val);
    current = current->next;
  }
  printf("\n");
}

Any help with telling me what is wrong or where I'm making the mistake would be greatly appreciated.

Upvotes: 0

Views: 76

Answers (1)

Pablo
Pablo

Reputation: 13570

Look carefully at your code:

int main(void) {
  int x;
  Node *head = malloc(sizeof(Node));
  for (x = 1; x < 4); x++) {
      ...
    addToList(head, x);
  }
  ...
}

You are not initializing the memory, so head->val and head->next are not initialized. Because of that

while (current->next != NULL) {
    current = current->next;
}

will loop an undefined amount of times. The first current->next is most probably not NULL, so current = current->next get executed. At that point current is pointing to nowhere, hence the undefined behaviour which in your case leads to a segfault.

You have to initialized the memory like this:

Node *head = malloc(sizeof *head);
if(head == NULL)
    // error handling

head->next = NULL;

But you could also use calloc, which also sets the memory to 0, thus you don't have to initialize the values (in this case):

Node *head = calloc(1, sizeof *head);
if(head == NULL)
    // error handling

You should always check for the return value of malloc/calloc/realloc.

Also note that the signature of the main function can be one of these:

  • int main(void);
  • int main(int argc, char **argv);
  • int main(int argc, char *argv[]);

edit

Another error I've noticed right now:

x = scanf("%d");

That's not how scanf works. You have to pass a pointer, scanf saves the scanned value through the passed pointer. scanf returns the number of matched values on success, in this case, success would be 1:

int num;
int ret = scanf("%d", &num);
if(ret != 1)
{
    fprintf(stderr, "Could not read value from the user\n");
    continue; // to contiune looping

    // you could also do a break; and stop the looping, or
    // exit(1), etc.
}
    // error with scanf

Also don't use the same variable x for the loop iteration and user input, otherwise you are messing with the loop.

edit

User user3629249 wrote in the comment

good information, however the result will be the first entry in the linked list will contain garbage. Better to declare head via: Node *head = NULL; and the function addToList() check for NULL and proceed accordingly.

That's right, the head element doesn't save any number in this way.

Option 1: double pointer

Here addToList receives a double pointer. The initialization of head occurs when *head points to NULL. The function allocates memory for it, initializes the memory, saves the value and returns. In the concurrent calls of addToList *head won't be NULL, so addToList looks for the end of the list.

I've made small changes in the way you do malloc and realloc. Also I added an implementation of freeList which should be used to free the memory:

void addToList(Node **head, int val) {
    if(head == NULL)
    {
        fprintf(stderr, "head cannot be NULL\n");
        return;
    }


    if(*head == NULL)
    {
        *head = calloc(1, sizeof **head);
        head[0]->val = val;
        head[0]->next = NULL;
        return;
    }
    Node *current = *head;

    while (current->next != NULL) {
        current = current->next;
    }

    current->next = malloc(sizeof *current->next);
    if(current->next == NULL)
        return;
    current->next->val = val;
    current->next->next = NULL;
}

int main(void)
{
    int x;
    Node *head = NULL;
    for (x = 1; x < 4; x++)
    {
        int val;
        printf("Enter an integer: ");
        if(scanf("%d", &val) != 1)
        {
            fprintf(stderr, "Could not read from user. Skipping entry\n");
            continue;
        }

        addToList(&head, val);
    }

    printList(head);

    freeList(head);
    return 0;
}

void freeList(Node *head)
{
    if(head == NULL)
        return;

    Node *current = head;
    Node *next;

    while(next = current->next)
    {
        free(current);
        current = next;
    }

    free(current); // the last one

    free(head);
}

Option 2: addToList returns a pointer to the head

Here addToList takes a pointer to the head. If it's NULL, it allocates memory and initializes like in the shown above. If head is not NULL, the functions looks for the last element and the returns the head. On error the function returns NULL.

Node *addToList(Node *head, int val) {

    if(head == NULL)
    {
        head = calloc(1, sizeof **head);
        head->val = val;
        head->next = NULL;
        return head;
    }
    Node *current = *head;

    while (current->next != NULL) {
        current = current->next;
    }

    current->next = malloc(sizeof *current->next);
    if(current->next == NULL)
        return NULL;
    current->next->val = val;
    current->next->next = NULL;

    return head;
}

int main(void)
{
    int x;
    Node *head = NULL, *tmp;
    for (x = 1; x < 4; x++)
    {
        int val;
        printf("Enter an integer: ");
        if(scanf("%d", &val) != 1)
        {
            fprintf(stderr, "Could not read from user. Skipping entry\n");
            continue;
        }

        tmp = addToList(head, val);
        if(tmp == NULL)
        {
            fprintf(stderr, "Not enough memory\n");
            freeList(head);
            return 1;
        }

        head = tmp;

    }

    printList(head);

    freeList(head);
    return 0;
}

Upvotes: 3

Related Questions