Segmentation Fault during linked list in c

Question

I am getting a segfault when I try and print out my linked list. Can anyone explain why? I am aware a segfault means that I am accessing memory I am not supposed to. I am assuming this means I am not setting up my pointers right. Any help would be great. My code...

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


struct node
{
    int val;
    struct node *next;             
}*head;

typedef struct node item;

int main() {

    item *curr, *head;

    head = NULL;

    char word = 'y';
    //int num[10];
    //int i = 0;

    while (word == 'y'){
    printf("Would you like to enter an integer? (y/n) ");
    scanf("%s", &word);

        if(word == 'y'){
            int temp = 0;
            printf("Enter an integer: ");
            scanf("%d", &temp);
            curr = (item *)malloc(sizeof(item));
            curr->val = temp;
            if (head == NULL){
                head = curr;
                head->next = NULL;
            }
            else {
            curr->next  = head;
            head = curr;
            }
        }
    }

    curr = head;

    while(curr != NULL) {
        printf("%d\n", curr->val); //seg fault happens here
        curr = curr->next ;
    }
    return 0;
}

You're right, it compiles and works perfectly in GCC. Must be something with Xcode. Looks like I will finish this out in Linux. Thanks. — user2446973, Jun 03 '13 at 08:15
Sorry, I actually took the *head out. That was from before when I was using a typedef in my struct declaration. — user2446973, Jun 03 '13 at 08:20

score 4 · Answer 1 · edited May 23 '17 at 11:49

This:

scanf("%s", &word);

is a buffer overflow, since %s will read a string, but you only have a single character. This invokes undefined behavior; even if you enter just a single character, scanf() will add 0-termination after that character to make a proper string.

Change the declaration of word:

char word[32];

And scan with an explicit size, to prevent scanf() from writing outside the buffer:

scanf("%30s", word);

Also check the return values of all I/O and memory allocation calls, since they can fail.

Finally, don't cast the return value of malloc(), in C.

score 0 · Answer 2 · answered Jun 03 '13 at 08:02

Regarding the memory leaks, can I suggest you fix them with the following code:

while(curr != NULL) {
    item* temp = curr; // store the current pointer
    printf("%d\n", curr->val);
    curr = curr->next ;
    free(temp); //free the current one now that curr points to the next
}

This frees the already printed head in each iteration of the loop.

The other issues are already addressed by other posters.

score 0 · Answer 3 · answered Jun 03 '13 at 08:02

Initialize the *head pointer as

item *curr=NULL, *head = NULL;

without this, the if will not execute and you would access some random memory for head node. The while loop for printing the linked list may not terminate and keep accessing invalid memory.

if (head == NULL){ 
  ...
}

score 0 · Answer 4 · answered Jun 03 '13 at 19:08

You have been caught out by scanf. First you wish to read a single character and the format for that is %c - %s reads the next non-blank sequence of characters after skipping any leading whitespace. Using %s causes the error, as it overwrites memory.

However if you change the format to %c your code still won't work, and it's scanf again. For most formats scanf will skip leading whitespace, but it does not do this when reading characters. So if you run your code you will see this:

Would you like to enter an integer? (y/n) y
Enter an integer: 10
Would you like to enter an integer? (y/n) 10

The second time around scanf has read the newline after the 10 into word, that is not a y, and then moved on to print out your list - the 10 at the end.

To skip whitespace before a character you add a space into the format string, so the line becomes:

scanf(" %c", &word);

That one change will allow your code to work but you should really do more checking. scanf will return the number of items it successfully found, you should check that to make sure the user really did enter a number etc., etc. As an example here is what happens if the user accidentally enters y twice:

Would you like to enter an integer? (y/n) y
Enter an integer: y
Would you like to enter an integer? (y/n) Enter an integer:

What has happened here is scanf("%d", &temp) failed, returned 0, and stored nothing into temp. However as you did not check the result your program continues and then the second y is consumed by the next scanf(" %c", &word).

Also look at your if (head == NULL) statement - this is not really necessary at all, you can replace the whole if/else with just two lines... that is left as an exercise.

HTH

Segmentation Fault during linked list in c

4 Answers4