CODEWITHSUNDEEP
cpointerssegmentation-fault
divinedragon
divinedragon

Reputation: 5336

Segmentation fault in C

I have the following C program. It works when I include the following line, otherwise it gives a segmentation fault:

printf("head(%p), last(%p), newnode(%p)\n", head, last, newnode);

Any idea what's the issue here?

Here is my whole program. It's a basic circular queue example.

#include "stdio.h"
#include "malloc.h"

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

typedef struct node NODE;

void display(NODE *);

int main(void) {

    NODE *head, *last, *newnode = NULL;
    int i = 5;

    for ( ; i > 0; i--) {
        newnode = (NODE *) malloc(sizeof(NODE));
        newnode->data = i*10;
        newnode->next = NULL;

        //printf("head(%p), last(%p), newnode(%p)\n", head, last, newnode);


        if (head == NULL) {
            head = newnode;
            last = newnode;
        } else {
           last->next = newnode;
           last = newnode;
        }

        last->next = head;
    }

    display(head);

    return 1;
}

void display(NODE *head) {

    NODE *temp = NULL;
    temp = head;

    printf("Elements --> ");
    do {
            printf("%d ", temp->data);
            temp = temp->next;
    } while (temp != head);

    printf("\n");

}

Upvotes: 0

Views: 281

Answers (3)

pb2q
pb2q

Reputation: 59607

You need to explicitly initialize head and last to NULL also.

In your declaration:

NODE *head, *last, *newnode = NULL;

only newnode is being initialized to NULL. Consequently later in your if/else you're testing/assigning to random memory.

Do something like this:

NODE *head, *last, *newnode;
head = last = newnode = NULL;

You can't assume that pointers are automatically initialized to NULL, even though it may be the case on certain systems. Variables declared as static are an exception. Always initialize variables in C to reasonable values, especially pointers.

When you access garbage memory you're invoking undefined behavior. When you do this you may observe inconsistent and confusing results. In your case, adding the printf seemed to fix the problem. The reason that this affected the behavior of your code is implementation dependent and - once you've introduced undefined behavior - beyond your control.

Upvotes: 7

vanza
vanza

Reputation: 9903

last and head are not being initialized. You can turn on compiler warnings that will help you out:

$ gcc -O1 -Wall w.c -o we
w.c: In function ‘main’:
w.c:30:23: warning: ‘last’ may be used uninitialized in this function [-Wuninitialized]
w.c:26:12: warning: ‘head’ may be used uninitialized in this function [-Wuninitialized]

(Note that gcc needs -O1 for -Wuninitialized to work.)

Upvotes: 3

Daniel Fischer
Daniel Fischer

Reputation: 183873

You're not initialising head and last,

NODE *head, *last, *newnode = NULL;

so they have whatever garbage bits are where they are allocated and if (head == NULL) can fail even though head is not properly set. Initialise them to NULL too.

Upvotes: 2

Related Questions