Why is this C linked list program giving 'segmentation fault'?

Question

The first function reads a file that has a bunch of 'char's and puts them in a linked list. It is not working :(.

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

struct list {
    char val;
    struct list* next;
};

typedef struct list element;

int lcreate(char* fname, element* list);
int ldelete(element* list);
int linsert(char a, char b, element* list);
int lremove(char a, element* list);
int lsave(char* fname, element* list);



int lcreate(char* fname, element* list) {
    element* elem = list;
    char c = 0;
    FILE * file = NULL;

    file = fopen(fname, "r");

    while ((c = getc(file)) != EOF)
    {
        if(list == NULL) {
            list = (element*)malloc(sizeof(element));
            if(list == NULL) {
                return 0;
            }
            list->val = c;
        }
        else {

            elem->next=(element*)malloc(sizeof(element));
            elem = elem->next;
            elem-> val = c;
        }
    }
    fclose(file);
    elem->next = NULL;
    return 1;
}



int main(void) {
    int i = 0;


    element * list = NULL;
    lcreate("list.txt", list);

    for(i = 0; i<4; ++i) {
        printf("%c", list->val);
        list = list->next;
    }

    return 0;
}

Fixed problem with 'file' being null.

Jerry Coffin · Accepted Answer · 2010-02-18T23:53:31.667

One obvious problem is right here:

FILE * file = NULL;

fopen(fname, "r");

For the fopen to accomplish much, you need to assign the result from fopen to your FILE *:

file = fopen(fname, "r");

Edit: Since you're working in C, you can't pass the pointer by reference. As an alternative, you can pass a pointer to a pointer:

int lcreate(char *fname, element **list) {

     // ...
     *list = malloc(sizeof(element));
     (*list)->next = null;
     (*list)->val = c;
// ...
}

Basically, all the code inside of lcreate will need to refer to *list instead of just list. Alternatively, you can take a pointer to an existing list as input, and return a pointer to the list, so in main you'd have something like: list = lcreate("list.txt", list);

And don't forget that `elem= list` assigns a value. If you want `elem` updated when `list` is updated you need to change `elem` to `element **elem= &list` in the original code or `element **elem= list` in Jerry's version. — MSN, Feb 19 '10 at 05:42

score 2 · Answer 2 · answered Feb 18 '10 at 23:34

2

file is NULL, and you never assign a file handle to it.

answered Feb 18 '10 at 23:34

Anon.

58,739
8
81
86

LeopardSkinPillBoxHat · Answer 3 · 2010-02-18T23:51:53.040

Yep -- what the others said about the FILE pointer, and passing list by value rather than reference to lcreate(), is true.

You also aren't returning the size of the list from lcreate() -- you should probably return this via the return value or a pointer argument.

You are attempting to iterate through the list 4 times in the main() function, but there may be less than 4 items in the list. Eventually the printf() will cause a segmentation fault if list is NULL.

If you still have issues after making these changes, I would recommend adding tracing to your code to work out at which point the segmentation fault is happening.

Update:

Also please remember to free the memory you have allocated after you traverse the list, otherwise you'll end up with a memory leak (although in practice this won't really be an issue for you as the program is ending, but freeing memory is a good habit to get into).

score 1 · Answer 4 · answered Feb 18 '10 at 23:37

1

In your main function, you are also passing list by value to lcreate. Within the lcreate() function, you are overwriting a local copy of list, not changing the value of list in the main function. Since list is initialized to NULL, you will get a segfault when you call list->val.

answered Feb 18 '10 at 23:37

mob

117,087
18
149
283

Ok so how do I overwrite list in lcreate? Thanks! – ron Feb 18 '10 at 23:40
@ron - Pass in a double pointer to `list`, i.e. `element** list; lcreate("list.txt", list);`. Then in the `lcreate` method, you allocate it like this: `*list = malloc(...)`. This will ensure that the caller will see the changes made by the callee. – LeopardSkinPillBoxHat Feb 18 '10 at 23:50

score 0 · Answer 5 · answered Feb 18 '10 at 23:56

I can see an additional problem as well. In the while statement of lcreate() the true clause of the if statement malloc's some memory and assigns it to list however elem is not updated.

while ((c = getc(file)) != EOF)
{
    if(list == NULL) {
        list = (element*)malloc(sizeof(element));
        if(list == NULL) {
            return 0;
        }
        list->val = c;
    }
    else {

Next time through the while loop list will not be non-null but elem is still null so the assignment of elem->next tries to deference the null pointer and thus the segmentation fault (which, btw, means that you tried to access memory that has not been assigned to your process):-

else {
    elem->next=(element*)malloc(sizeof(element));

As others have pointed out you also don't return list back to main so it will still be NULL when you hit the printf() loop.

Finally, the debugger is your friend when looking at these problems. You'll see exactly which line triggers the seg fault and what the state of the variables were.

score 0 · Answer 6 · answered Feb 19 '10 at 01:00

It would be good to check if the malloc was successful by checking for a non null pinter. Also, you might want to allocate the head/first link outside of the while to avoid the null check for the head every time in the while loop. Of course, these are optimizations, in case your linked list grows really large!

Why is this C linked list program giving 'segmentation fault'?

6 Answers6

Linked