Circular linked list crashes when displayed

Question

I'm trying to make a circular linked list. When I try to display the list after creating it, the program keeps on crashing. Here is my code:

#include <stdio.h>
#include <stdlib.h>

typedef struct node {
    int data;
    struct node * next;
} node;

node * createList(int);
void display(node * head);

int main() {
    struct node * head;

    head = createList(5);
    display(head);

}

node * createList(int n) {

    int i = 0,data = 0;
    struct node * head = NULL;
    struct node * temp = NULL;
    struct node * p = NULL;

    for (i = 0; i < n; i++) {
        temp = (node*)malloc(sizeof(node));
        temp->data = data++;
        temp->next = head;

        if (head == NULL) {
            head = temp;
        } else {
            p = head;
            while (p->next != NULL) {
                p = p->next;
            }
            p->next = temp;
        }
    }
    return head;
}

void display(node * head) {
    struct node * temp = head->next;
    while (temp != head) {
        printf("%d-> \t",temp->data);
        temp = temp->next;
    }
    printf("\n");
}

What am I doing wrong?

Answer 1

1. You have set every temp's next to head in temp->next = head; but did it too early (the first is just NULL). Then you tested p->next against NULL in while (p->next != NULL) { but you should have tested against head. Alternatively, you can continue to test against NULL but then you need to initialize temp->next to NULL and assign head to temp->next only after the for loop.

2. Your display code started from the second link.

Here is a fixed code using the first option in 1. above:

    for (i = 0; i < n; i++) {
        temp = (node*)malloc(sizeof(node));
        temp->data = data++;

        if (head == NULL) {
            head = temp;
        } else {
            p = head;
            while (p->next != head) {
                p = p->next;
            }
            p->next = temp;
        }
        temp->next = head;
    }

Here is a fixed code using the alternative option in 1. above. You still need to initialize temp->next to NULL since malloc() does not initialize.

    for (i = 0; i < n; i++) {
        temp = (node*)malloc(sizeof(node));
        temp->data = data++;
        temp->next = NULL;

        if (head == NULL) {
            head = temp;
        } else {
            p = head;
            while (p->next != NULL) {
                p = p->next;
            }
            p->next = temp;
        }
    }
    if (temp != NULL) {
        temp->next = head;
    }

But in reality, there is no need to "walk" from the head on every creation. You can simply keep the previous and link it to the next:

    for (i = 0; i < n; i++) {
        temp = (node*)malloc(sizeof(node));
        temp->data = data++;

        if (head == NULL) {
            head = p = temp;
        } else {
            p = p->next = temp;
        }
    }
    if (temp != NULL) {
        temp->next = head;
    }

Here is a fix for the display():

void display(node * head) {
    struct node * temp = head;
    if (temp != NULL) {
        do {
            printf("%d-> \t",temp->data);
            temp = temp->next;
        } while (temp != head);
    }
    printf("\n");
}