Keep getting a seg fault on this code

Question

I am trying to implement a linked list in C. There is a struct list that has void pointers to the first and last position of the list. A struct node that has a void pointer to data and a struct node pointer to next. For some reason when I try and access the struct lists front pointer it seg faults. Here is the code. Any help will be appreciated.

(The main method initializes the list that is passed into the function as null)

int main()
{
   struct list *linked_list;
   linked_list = NULL;

   int *ptr;
   int x = 5;
   ptr = &x;

    linked_list = list_add(linked_list,ptr);

 }

struct list {
    void *front;
    void *back;
};

struct node{
    void *data;
    struct node *next;
    struct node *prev;
};

struct list *list_add(struct list *li, void *d){
    struct node *new_node;

    new_node = (struct node *)malloc(sizeof(struct node));
    if(new_node == NULL){
        printf("Malloc failed");
        exit(1);
    }


    new_node->data = d;
    new_node->next = NULL;
    struct node *cur;
    for(cur = li->front; cur != NULL; cur = cur->next){
        if(cur->next == NULL){
            new_node->prev = cur;
        }

    }
    li->front = new_node;
    return li;
}

this really isn't the correct implementation of linked lists — dbarnes, Nov 18 '13 at 21:10
Based on where you indicate the problems are, your `li` is invalid. Without seeing how you're calling `list_add()`, it's impossible to know for certain what you're doing to cause the crash. — mah, Nov 18 '13 at 21:11
***[Don't cast the return value of `malloc()`!!!](http://stackoverflow.com/questions/605845/do-i-cast-the-result-of-malloc/605858#605858)*** — , Nov 18 '13 at 21:15

mah · Accepted Answer · 2013-11-18T21:27:25.167

1

struct list *linked_list;
linked_list = NULL;

...

linked_list = list_add(linked_list,ptr);

You're passing a NULL pointer (linked_list) into list_add(). Then in list_add() you dereference that pointer, leading to the crash.

Inside list_add(), consider something like this at the top:

struct list *list_add(struct list *li, void *d){
struct node *new_node;

if (!li) {
    li = malloc(sizeof(*li));
    li->front = li->back = NULL;
}

new_node = malloc(sizeof(struct node));
...

There may be more you need to do, but this will get you past the first hurdle. Note also that you could do a similar initialization in main() before ever calling list_add().

edited Nov 18 '13 at 21:27

answered Nov 18 '13 at 21:15

mah

39,056
9
76
93

How would I fix it? the list is initially empty. if I do li.front = new_node after I make it, I get an error. – user2858976 Nov 18 '13 at 21:19
I have a suggested method of fixing it in my answer, a check for NULL and a few lines to run in that condition to create a valid list structure. You cannot use `li.front` in this code because `li` is a pointer, not a structure. You must `li->front = new_node;`. – mah Nov 18 '13 at 21:21
Thank you very much. I got it working now. Out of curiosity, why do we malloc li if its a pointer? – user2858976 Nov 18 '13 at 21:31
pointers must point to _something_ in order to be useful… when you declare a pointer you have not made space for whatever it must point to (the struct list, in this case). malloc() is one method of allocating that space. In your case however, another viable solution might be to declare (in main) `struct list linked_list;` (NOT a pointer), then call `list_add(&linked_list,…)` -- _except_ that your list_add is returning a pointer that you seem to want. Though you don't otherwise modify the pointer in list_add, so it looks like it would work with minor changes. – mah Nov 18 '13 at 21:33

score 0 · Answer 2 · answered Nov 18 '13 at 21:15

well first off I don't think that this is the correct way to do linked lists, there is no need for the list struct

int main()
{
   struct list *linked_list;
   linked_list = NULL;

   int *ptr;
   int x = 5;
   ptr = &x;

    linked_list = list_add(linked_list,ptr);

 }

struct list {
    void *front;
    void *back;
};

struct node{
    void *data;
    struct node *next;
    struct node *prev;
};

struct list *list_add(struct list *li, void *d){
    //you need to check if li is null if so initialize it
    if(li ==null){
      li = (struct list*) malloc(sizeof(struct list));
    }
    struct node *new_node;

    new_node = (struct node *)malloc(sizeof(struct node));
    if(new_node == NULL){
        printf("Malloc failed");
        exit(1);
    }


    new_node->data = d;
    new_node->next = NULL;
    struct node *cur;
    for(cur = li->front; cur != NULL; cur = cur->next){
        if(cur->next == NULL){
            new_node->prev = cur;
        }

    }
    li->front = new_node;
    return li;
}

but instead of checking for null in the function just have list_add be a void and pass in an initialized list

Keep getting a seg fault on this code

2 Answers2