c++linked-listvalgrindfreememcheck

How to free a deleted node in the middle of a list without valgrind errors?


This is a little test to help demonstrate and review the ouput of valgrind's memcheck. Can someone help me figure out how to remove AND free a node from the middle of the list? If I comment out the free(cur) and free(cur->lock) from the remove node section, then memcheck tells me I have a memory leak, but if I keep them there then I am doing an invalid read at the top of the loop. Is there a way out of this conundrum?

TEST(UtilityGeneralUnittest, valgrindTests)
{
    //declare a node type
    typedef struct node{
        int size;
        int value;
        struct node *next;
        pthread_mutex_t *lock;
    }node_t;

    //make the head
    node_t *head;
    head = (node_t*)malloc(1 * sizeof(node_t));
    head->size = 0;
    head->next = NULL;
    head->lock = (pthread_mutex_t*)malloc(1 * sizeof(pthread_mutex_t));
    pthread_mutex_init(head->lock, NULL);

    //create array for storing values
    int array[10];


    //build a list with random numbers
    for (int i = 0; i < 10; i++)
    {
        node_t *newNode;
        newNode = (node_t*)malloc(1 * sizeof(node_t));
        newNode->value = rand() % 100 + 1;
        newNode->next = NULL;
        newNode->lock = (pthread_mutex_t*) malloc(1 * sizeof(pthread_mutex_t));
        pthread_mutex_init(newNode->lock, NULL);
        array[i] = newNode->value;
        if (head->next == NULL)
        {
            head->next = newNode;
            head->size++;
        }
        else
        {
            node_t *tmp = head->next;
            head->next = newNode;
            newNode->next = tmp;
            head->size++;
        }
    }
    // assert the list added nodes
    ASSERT_EQ(10, head->size);

    //sanity check; print the list
    node_t *printer = head;
    while(printer->next != NULL)
    {
        printer = printer->next;
        std::cout << "value: ";
        std::cout <<  printer->value << ", ";
    }
    std::cout << "\n";
    // the meat and potatoes: deleting with locks.
    int removeMe = array[rand() % 10];
    bool verifyDel = true;
    int checkVal = removeMe;
    node_t *prev;
    node_t *cur;

    prev = head;
    pthread_mutex_lock(prev->lock);
    while((cur = prev->next) != NULL) //******** this is the problem
    {
        pthread_mutex_lock(cur->lock);
        if(cur->value == removeMe)
        {
            prev->next = cur->next;
            pthread_mutex_unlock(cur->lock);
            pthread_mutex_unlock(prev->lock);
            cur->next = NULL;
            head->size--;
            free(cur->lock); ///******** this is the problem
            free(cur); ///****** this is the problem
        }
        pthread_mutex_unlock(prev->lock);
        prev = cur;
    }
    //pthread_mutex_unlock(prev->lock);


    //verify node has been removed
    printer = head;
    while(printer->next != NULL)
    {
        printer = printer->next;
        if(printer->value == checkVal)
        {
            verifyDel = false;
        }
        std::cout << "value: ";
        std::cout <<  printer->value << ", ";
    }
    std::cout << "\n";
    ASSERT_TRUE(verifyDel);

    //clean up: delete the list
    while((printer = head) != NULL)
    {
        head = head->next;
        free(printer->lock);
        free(printer);
        std::cout << "free!!!" << std::endl;
    }


}

Solution

  • Looking at your loop (simplified):

    while ((cur = prev->next) != NULL) //* problem
    {
        if (cur->value == removeMe)
        {
            prev->next = cur->next;
            cur->next = NULL;
            free(cur);              //* problem
        }
        prev = cur;
    }
    

    The problem is with the assignment prev = cur, but only when the if block is evaluated. That has freed cur, so next time around the loop, cur=prev->next references deleted storage.

    You can fix that by inserting else to assign prev only when cur wasn't removed:

    while ((cur = prev->next)) {
        if (cur->value == removeMe) {
            prev->next = cur->next;
            cur->next = NULL;
            free(cur);
        } else {
            prev = cur;
        }
    }