Freeing a strdup inside of a linked list

Question

I have a linked list in which I am trying to free a char* which I used strdup for.

These are the methods I have for adding a node into my list:

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

node * add_last(char* name, node *head ) {
      node* new_node;
      node* current;
      new_node = (node*)malloc(sizeof(node));
      char* c = strdup(name);
      new_node->name = c;
      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;
  }

For removing nodes:

 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;
  }

And for cleaning up the list:

void deleteList(node** head){
      node* current = *head;
      node* next;

      while(current != NULL){
              next = current->next;
              free(current->name);
              free(current);
              current = next;
      }

      *head = NULL;
  }

But valgrind says I have "definitely lost" memory when it comes to the strdup I have in my add_last function (but oddly not my add_first function).

Why is it only happening for the add_last method and how can I free it properly? I thought I was freeing all the nodes and the names in my deleteList method?

Answer 1

You are needlessly duplicating the string from the last-node removal algorithm, and leaking the original string. That algorithm should find the last node in the list, pass ownership of the name pointer to the out-param, then delete the node without deleting the name (since the out-param now owns it).

I.e. You should be doing something along these lines (fair warning, not compile-tested, but you hopefully get the idea):

int remove_last( node **head, char **ret )
{
    int res = -1;
    if (*head)
    {
        // find last node in the list
        while ((*head)->next)
            head = &(*head)->next;

        // out-param takes ownership
        *ret =(*head)->name;

        // free last node *without* freeing the string
        free(*head);
        *head = NULL;

        // result indicates success
        res = 0;
    }
    return res;
}