Addressing 'heap-use-after-free' error in Linked List implementation

Question

I am working on implementing a singly-linked list data structure in C. When testing the implementation in my driver file (main.c) below:

#include "llist.h"

int main (int argc, char *argv[]) {
    printf("argc: %d", argc);
    printf("\n\n");

    int num_inputs = argc;
    Node *list = NULL;

    if (argc == 1) {
        printf("No arguments passed.\n");
    } else {
        for (int i = 1; i < num_inputs; i++) {
            printf("String is: %s\n", argv[i]);
            Node *n = newNode(argv[i]);

            printf("String is : %s\n\n", argv[i]);

            push(&list, n);

            printLinkedList(list);
        }


        reverseOrder(&list);
        pop(&list);
        deleteList(&list);
    }
    
    return 0;
}

I receive the following error

AddressSanitizer: heap-use-after-free on address

where the relevant lines correspond to when I call my deleteList function in the main.c file above

My deleteList function along with the rest of my Linked List functions are below:

#include "llist.h"

// Frees all allocated memory associated with the list pointers iteratively
void deleteList(Node **list) {
    while(*list != NULL) {
        Node *ptr = *list;
        *list = (*list)->next;
        free(ptr->data);
        free(ptr);
    }
}

// Frees all allocated memory associated with a single node

void deleteNode(Node **toDelete) {
    Node * del = *toDelete;
    free(del->data);
    free(del);
}

// Allocates memory for a new string and returns a pointer to the memory

Node *newNode(char *string) {
    unsigned long len = strlen(string);
    printf("length : %lu \n\n", len);

    Node *temp = (Node*)malloc(sizeof(Node));
    temp->data = (char*)malloc((len + 1)*sizeof(char));
    strcpy(temp->data, string);
    temp->next = NULL;

    return temp;
}

// Removes a node from the front of a list

Node *pop(Node **list) {
    Node *newptr = (*list)->next;

    if (newptr == NULL) {
        return *list;
    } else {
        deleteNode(list);
    }

    return newptr;
}

// Adds a node to the front of a list

void push(Node **list, Node *toAdd) {
    toAdd->next = *list;
    *list = toAdd;
}

// Return a list of pointers in order

void reverseOrder(Node **list) {
    Node* prev = NULL;
    Node* current = *list;
    Node* next;

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

    *list = prev;
}

// Prints the string stored in a single node

void printNode(Node *singleNode) {
    printf("Data : %s\n", singleNode->data);
}

// Prints an entire linked list. Nodes are printed from first to last

void printLinkedList(Node *linkedList) {
    Node *temp = linkedList;
    
    while(temp!=NULL) {
        printf("Data : %s\n", temp->data);
        temp = temp->next;
    }
}

where the line *list = (*list)->next; is the one the compiler specifically mentions in the error output.

The program executes correctly when I run the program with just one argument ("./main abcdef")

Output

argc: 2

String is: abcdef
length : 6 

String is : abcdef

Linked List Contents : abcdef

However, I receive the error when trying to run my program with more than one argument ("./main abc def")

Output with error:

argc: 3

String is: abc
length : 3 

String is : abc

Linked List Contents : abc
String is: def
length : 3 

String is : def

Linked List Contents : def
Linked List Contents : abc
=================================================================
==125404==ERROR: AddressSanitizer: heap-use-after-free on address 0x602000000018 at pc 0x0000004f8265 bp 0x7ffceb8712b0 sp 0x7ffceb8712a0

Answer 1

Your deleteNode implementation is wrong. It should be the same that you use in deleteList. In fact you can simply call deleteNode repeatedly from deleteList.

There is a reason, why you are using a pointer to pointer in deleteNode and you are not making use of it.

The reason is, because in deleteNode the deleted node is not removed and this means that you are accessing later on a next pointer which is already deleted.

This fixes your problem.

// Frees all allocated memory associated with a single node
void deleteNode(Node **list)
{
    Node *ptr = *list;
    *list = (*list)->next;
    free(ptr->data);
    free(ptr);
}

// Frees all allocated memory associated with the list pointers iteratively
void deleteList(Node **list)
{
    while (*list != NULL)
    {
        deleteNode(list);
    }
}