Segmentation fault when removing item from linked list

Question

I'm trying to create program where you enter '+word' and it adds the word and when you enter '-word' it takes the word out of the linked list.

Inserting the word works fine for me but removing it causes a segmentation fault. I'm not sure where the fault lies. Also, is there a way where you can get a hint of where the segmentation fault is?

void
remove_from_list(struct linked_list *list, char *data)
{
    struct node *current_node = list->head;
    struct node *previous_node = NULL;

    while (current_node != NULL) {
        if (current_node->data == data) {
            break;
        }
        previous_node = current_node;
        current_node = current_node->next;
    }
    if (previous_node == NULL) {
        list->head = list->head->next;
    } else {
        previous_node->next = current_node->next;
    }
    free(current_node);
    if (list->tail == current_node)
        list->tail = previous_node;
}

int
main(void)
{
    struct linked_list list = { .head = NULL, .tail = NULL };
    char word[50];

    do {
        printf("Enter string: ");
        fgets(word, 50, stdin);
        if (word[0] == '+')
            add_to_list(&list, word);
        else if (word[0] == '-')
            remove_from_list(&list, word);
    } while (word[0] != '\n');

    print_list_rec(&list);
    free_list(&list);
    return 0;
}

You should use an actual string comparison (like strncmp) to find whether a node holds your string. All that this: current_node->data == data does is compare the pointers. — turbulencetoo
– turbulencetoo, Commented Jun 25, 2014 at 19:31
Because strings are tricky in C, if only from a memory management perspective, perhaps try your same code but with integers as data and see if you get the same issues. — turbulencetoo
– turbulencetoo, Commented Jun 25, 2014 at 19:33
regarding this line: free_list(&list);, you cannot free a struct on the stack. However, you could loop through the linked list, free'ing each node in the list (other than the first one since it is on the stack. — user3629249
– user3629249, Commented Jun 25, 2014 at 19:55
The function remove_from_list() does not properly handle an empty list, rather it tries 'list->head = list->head->next;' which is taking the address of an offset past address 0 (null). — user3629249
– user3629249, Commented Jun 25, 2014 at 19:57
regarding this line: struct linked_list list = { .head = NULL, .tail = NULL }; does not clear the .next field nor the .data field. A much better line is: struct linked_list list = { 0 }; — user3629249
– user3629249, Commented Jun 25, 2014 at 20:00

Andrew_CS · Accepted Answer · 2014-06-25 19:55:54Z

The main reason you're getting a seg fault is because you don't handle the case of not having the data in the list when attempting to remove.

if (previous_node == NULL) { 
    list->head = list->head->next;
} else { // ------------------------- If at the end of the list you go in here
    previous_node->next = current_node->next;
}

current_node is Null so current_node->next seg faults.

The reason you go to the end of your list is because you aren't comparing your data correctly for strings. Use strcmp() like @this suggested to correctly compare. But you should handle the case of not having the data in your list.

You could add a check in-between your while loop and first if statement, this would handle an empty list and data not in the list -

if(current_node == NULL) // Empty list or wasn't found
    return;

Another note:

You free current_node before checking to see if it was the tail. Reverse this order.

if (list->tail == current_node)
    list->tail = previous_node;
free(current_node);

this · Accepted Answer · 2014-06-25 19:33:29Z

1

You loop to the end of you linked list and then proceed to dereference a NULL pointer here

} else {
    previous_node->next = current_node->next;
}

This happens because your comparison doesn't actually compare the data;

 if (current_node->data == data) {

and you never get a true result out of that if statement.

Use strcmp() if you want to compare strings.

answered Jun 25, 2014 at 19:33

this

5,3031 gold badge25 silver badges53 bronze badges

Comments

Shlomi Agiv · Accepted Answer · 2014-06-25 19:40:26Z

0

on top of what the others said, if the list is empty, this code will cause a segmentation:

if (previous_node == NULL) {
    list->head = list->head->next;
}

answered Jun 25, 2014 at 19:40

Shlomi Agiv

1,1988 silver badges18 bronze badges

Comments

cbelwal · Accepted Answer · 2014-06-25 19:42:40Z

0

Without the code for the insertion function it is difficult to say what it wrong, since the steps for removal lool ok. However, there could be something wrong in insertion due to which this is not working.

But there is a problem in your code, wherein if you are trying to remove a node which does not exist, you will still end up removing the last node. You need to set a flag befre the while loop break and then only remove the node if flag is true.

answered Jun 25, 2014 at 19:42

cbelwal

3692 silver badges7 bronze badges

1 Comment

Andrew_CS Over a year ago

Note: The last node wouldn't be removed if the data wasn't in the list - it would seg fault when attempting to access current_node->next. Also a flag isn't needed, just need to check if current_node is Null

Vlad from Moscow · Accepted Answer · 2014-06-25 20:09:48Z

0

The function should look the following way

void
remove_from_list( struct linked_list *list, char *data )
{
    struct node *current_node = list->head;
    struct node *previous_node = NULL;

    while ( current_node != NULL && strcmp( current_node->data, data ) != 0 ) 
    {
        previous_node = current_node;
        current_node = current_node->next;
    }

    if ( current_node != NULL ) 
    {
        if ( previous_node != NULL ) 
            previous_node->next = current_node->next;
        else
            head = head->next; 

        if ( list->tail == current_node )
            list->tail = previous_node;

        free( current_node->data );
        free( current_node );
    }
}

Also I would store strings without leading + or -. In this case the if statement in main would look as

    if ( word[0] == '+' )
        add_to_list( &list, word + 1 );
    else if ( word[0] == '-' )
        remove_from_list( &list, word + 1 );

Otherwise you will never find a string that was added with plus sign that to remove it from the list.

edited Jun 25, 2014 at 20:09

answered Jun 25, 2014 at 19:49

Vlad from Moscow

313k27 gold badges204 silver badges358 bronze badges

2 Comments

Andrew_CS Over a year ago

Should be using strcmp() to compare the strings.

Vlad from Moscow Over a year ago

@Andrew_CS Oh, yes.:)

Collectives™ on Stack Overflow

Segmentation fault when removing item from linked list

5 Answers 5

Comments

Comments

Comments

1 Comment

2 Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

5 Answers 5

Comments

Comments

Comments

1 Comment

2 Comments

Your Answer

Sign up or log in

Post as a guest

Related