Having Issues with Segmentation Error in Doubly Linked List Implementation

Question

I'm pretty new to C and I can't figure out why I'm getting a segmentation fault from this code:

void delete_list(LIST *list)
{
  NODE* n = list->head;
  while(n != list->tail)
  {
    NODE* next = n->next;
    free(n);
    n = next;
  }
  free(n);
  free(list);
}

I'm pretty certain this is the part of my code that's has the error, but if not I can post the rest of my code. Both NODE and LIST are created using malloc(). The code is the barebones framework for a doubly-linked-list.

EDIT 2: Here's the code to reproduce what I'm doing. Very basic test case but it is failing. First is the header file list.h, then the main file list.c. I also eradicated the first edit to clean up the question a bit.

#ifndef __LIST_H__
#define __LIST_H__

typedef struct node {
  char *value;  /* Pointer to the string we are storing. */
  struct node *previous;  /* Pointer to the preceding node in the list. */
  struct node *next;  /* Pointer to the next node in the list. */
} NODE;

typedef struct list {
  NODE *head;  /* Pointer to the first node in the list. */
  NODE *tail;  /* Pointer to the last node in the list. */
} LIST;

/* Function prototypes the public interface. */
LIST *new_list(const char *value);
void prepend(LIST *list, const char *value);
void append(LIST *list, const char *value);
void delete_list(LIST *list);

#endif

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

#include "list.h"

NODE *new_node(const char *value);

LIST *new_list(const char *value)
{
  LIST *list = (LIST*) malloc(sizeof(LIST));
  NODE* n = new_node(value);
  list->head = n;
  list->tail = n;

  return list;
}

void prepend(LIST *list, const char *value)
{
  NODE* n = new_node(value);
  n->next = list->head;
  list->head->previous = n;
  list->head = n;
}

void append(LIST *list, const char *value)
{
  NODE* n = new_node(value);
  n->previous = list->tail;
  list->tail->next = n;
  list->tail = n;
}

void delete_list(LIST *list)
{
  NODE* n = list->head;
  while(n != list->tail)
  {
    NODE* next = n->next;
    free(n);
    n = next;
  }
  free(n);
  free(list);
}

NODE *new_node(const char *value)
{
  NODE *node = (NODE*) malloc(sizeof(NODE));

  node->value = (char*) malloc((strlen(value) + 1) * sizeof(char));
  strcpy(node->value, value);
  node->previous = NULL;
  node->next = NULL;
  
  return node;
}

void print_list(const LIST *list)
{
  /* Print the contents of a list. */
  for (NODE *node = list->head; node != NULL; node = node->next) {
    printf("%s\n", node->value);
  }
}

int main()
{
  char *middle = "middle";
  char *last = "last";
  char *first = "first";
  LIST* list = new_list(middle);
  prepend(list, first);
  append(list, last);
  print_list(list);
  delete_list(list);
  print_list(list);
}

Answer 1

You obviously can't

  delete_list(list);
  print_list(list);

The list is gone after delete. (you invoke Undefined Behavior at that point) The only other issue you have is you fail to free() node->value.

You can fix that with:

void delete_list(LIST *list)
{
  NODE* n = list->head;
  while(n != list->tail)
  {
    NODE* next = n->next;
    free(n->value);
    free(n);
    n = next;
  }
  free (n->value);
  free(n);
  free(list);
}

Your code works fine then.

Memory Use/Error Check

$ valgrind ./bin/list
==9065== Memcheck, a memory error detector
==9065== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==9065== Using Valgrind-3.13.0 and LibVEX; rerun with -h for copyright info
==9065== Command: ./bin/list
==9065==
first
middle
last
==9065==
==9065== HEAP SUMMARY:
==9065==     in use at exit: 0 bytes in 0 blocks
==9065==   total heap usage: 8 allocs, 8 frees, 1,130 bytes allocated
==9065==
==9065== All heap blocks were freed -- no leaks are possible
==9065==
==9065== For counts of detected and suppressed errors, rerun with: -v
==9065== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)

Alternative Delete List

You may find the following alternative for delete_list() a bit more readable -- up to you:

void delete_list (LIST *list)
{
  NODE *n = list->head;
  
  while (n) {
    NODE *victim = n;
    n = n->next;
    free (victim->value);
    free (victim);
  }
  free (list);
}