Trying to remove last element from linked list and save it into a variable passed as parameter giving segfault

Question

I have a linked list in which I am trying to remove the last element that is a char* and set the parameter I pass into that function to the char*. However, this either gives a segfault or prints out gibberish. Why is this happening?

Here is the linked list:

  struct list_node {
          char* name;
          struct list_node* next;
  };
  typedef struct list_node node;

And here are its functions (I'm pretty sure the other functions work but not the remove_last):

  int remove_last(node* head, char* ret) {
          while(head) {
                  if (head->next->next == NULL) {
                          printf("here\n");
                          ret = strdup(head->next->name);
                          printf("%s\n", ret);
                          printf("there\n");
                          //free(head->next);
                          //head->next = NULL;
                          return 0;
                  }
                  head = head->next;
          }
          return -1;
  }

  void add_last(char* name, node* current) {
          node* new_node;
          new_node = (node*)malloc(sizeof(node));
          while(current) {
                  if (current->next == NULL) {
                          current->next = new_node;
                          new_node->name = name;
                          return;
                  }
                  current = current->next;
          }
  }

  node* add_first(char* name,  node* head) {
          node* new_node;
          new_node = (node*)malloc(sizeof(node));
          new_node->name = name;
          new_node->next = head;
          head = new_node;
          return head;
  }

Inside the function, when I printf ret, I get what I am supposed to get, but in my main when I try to print out the ret variable I pass into parameters:

char *temp = NULL;
remove_last(head, temp);
printf("%s\n", temp);

I get a segfault. I originally thought maybe it was because I was setting the nodes and freeing them, but I also used strdup (which I think copies it to a new location or smth like that?). I think it might also have something to do with how I am setting temp to null and then I am not setting assigning ret = name correctly in the function? Any advice?

Answer 1

For starters the function add_last is incorrect and can invoke undefined behavior.

Firstly it does not check whether the pointer current (I suppose it can be a pointer to the head node) is equal to NULL. In this case there is a memory leak because the newly created node is not appended to the list.

Secondly the function does not set the data member next of the newly created node to NULL

At least the function can be declared and defined the following way if to use your approach to implementing the function add_first

node * add_last(char* name, node *head ) {
    node* new_node; 
    new_node = (node*)malloc(sizeof(node));
    new_node->name = name;
    new_node->next = NULL;

    if ( head == NULL )
    {
        head = new_node;
    }
    else
    {  
        current = head;

        while ( current->next ) current = current->next;

        current->next = new_node;
    }

    return head;
}

As for the function remove_last then the function parameter ret is a local variable of the function that will not be alive after exiting the function. The function parameter must have the type char **. That is the corresponding argument must be passed by reference to the function.

And in any case the function is incorrect because it ignores a case when the list contains only one node. You need to pass the pointer to the head node also by reference.

The function can look the following way.

int remove_last( node **head, char **ret )
{
    int result = *head == NULL ? -1 : 0;

    if ( result == 0 )
    {
        while( ( *head )->next != NULL ) head = &( *head )->next;

        *ret = strdup( ( *head )->name );
        free( *head );
        *head = NULL;
    } 

    return result;
}