CODEWITHSUNDEEP
c++destructordelete-operator
vimalloc
vimalloc

Reputation: 4187

Why does use-after-free happen when a linked list node deletes its neighbors in the destructor?

I am having a problem with delete and destructor (I am sure I am making a stupid mistake here, but haven't been able to figure it out as of yet).

When I step through into the destructor, and attempt to call delete on a pointer, the message shows up "Cannot access memory at address some address."

The relevant code is:

/*
 * Removes the front item of the linked list and returns the value stored
 * in that node.
 *
 * TODO - Throws an exception if the list is empty
 */
std::string LinkedList::RemoveFront()
{
    LinkedListNode *n = pHead->GetNext(); // the node we are removing
    std::string rtnData = n->GetData(); // the data to return

    // un-hook the node from the linked list
    pHead->SetNext(n->GetNext());
    n->GetNext()->SetPrev(pHead);

    // delete the node
    delete n;
    n=0;

    size--;
    return rtnData;
}

and

/*
 * Destructor for a linked node.
 *
 * Deletes all the dynamically allocated memory, and sets those pointers to 0.
 */
LinkedListNode::~LinkedListNode()
{
    delete pNext; // This is where the error pops up
    delete pPrev;
    pNext=0;
    pPrev=0;
}

Upvotes: 3

Views: 1562

Answers (3)

Péter Török
Péter Török

Reputation: 116306

It seems that you are deleting the next and previous nodes of the list from the destructor. Which, if pNext and pPrev are LinkedListNode*, means that you are recursively deleting the whole list :-(

Try this:

std::string LinkedList::RemoveFront()
{
    LinkedListNode *n = pHead->GetNext(); // the node we are removing
    std::string rtnData = n->GetData(); // the data to return

    // un-hook the node from the linked list
    pHead->SetNext(n->GetNext());
    n->GetNext()->SetPrev(pHead);

    n->SetNext(0);
    n->SetPrev(0);
    // delete the node
    delete n;
    n=0;

    size--;
    return rtnData;
}

LinkedListNode::~LinkedListNode()
{
}

(Actually you don't even need to reset the prev and next pointers to 0 since you are going to delete the node anyway. I left those statements in because they at least put the node into a consistent state, which is a good idea in general. It may make a difference if you later e.g. change your memory management strategy and decide to store unused nodes for later reuse.)

Upvotes: 5

Mike Dinsdale
Mike Dinsdale

Reputation: 1511

It seems that your LinkedListNode is deleting its neighbours, so when you delete one node it then proceeds to destroy the entire list - note you don't set pNext and pPrev to NULL when you remove your node.

Also your LinkedListNode destructor is problematic even in the case when you want the whole list to be destroyed: having both delete pNext and delete pPrev will lead to multiple calls of the same destructor (and I think eventually a stack overflow).

Upvotes: 1

Krzysztof Bujniewicz
Krzysztof Bujniewicz

Reputation: 2417

Actually you shouldn't be messing with neighbours in a node. That's for list class to do - connect them properly. In the destructor you can set them to null, but unless you have allocated dynamically something else - you don't have to call delete

Upvotes: 0

Related Questions