Segmentation fault (core dumped) when I delete pointer

Question

I am trying to remove duplicates from a linked list, and encountered a problem, which is probably obvious and straightforward but I haven't used C++ in many years and I couldn't find out what I am doing wrong by reading similar questions on SO.

Below is parts of my code. I removed irrelevant parts (eg. constructors, other methods, etc).

template<class T>
class Node {
  Node() : data(NULL), next(NULL), prev(NULL) {}
  explicit Node(T d) : data(d), next(NULL), prev(NULL) {}
  explicit Node(T d, Node<T> *nxt, Node<T> *prv) : data(d), next(nxt),    prev(prv) {}
  ~Node() { delete next; delete prev; }

  T data;
  Node *next;
  Node *prev;
};

template<class T>
class LinkedList {
  LinkedList() : head(NULL) {}
  explicit LinkedList(Node<T> *head_node) : head(head_node) {}
  LinkedList& operator=(const LinkedList &copy_list);
  ~LinkedList(); //didn't implement this but I guess delete head is all?

  Node<T>* head;
};


template<class T>
LinkedList<T> RemoveDuplicates(const LinkedList<T> &linked_list) {
  //my = overload creates a whole new list with the same data as the original one
  LinkedList<T> result = linked_list; 

  Node<T> *cur = result.head;
  Node<T> *prev = NULL;

  while (cur) {
    if (...) { //duplicate found  
      Node<T> *temp = cur;
      prev->next = cur->next;
      cur = cur->next;
      cur->prev = prev;
      free(temp); //Here is my problem!!!
    }
    else {
      prev = cur;
      cur = cur->next;
    }
  }
  return result;
}

So first, I did delete temp and I got Segmentation fault. I then realized you only delete what you new. Fair enough, but I am newing each Node when building the whole list in main:

Node<char> *h = new Node<char>('H'); //I have a constructor to handle this
Node<char> *e = new Node<char>('E');
Node<char> *l1 = new Node<char>('L');
Node<char> *l2 = new Node<char>('L');
Node<char> *o = new Node<char>('O');

h->next = e;
h->prev = NULL;

e->next = l1;
e->prev = h;

//and so on

So why I am not allowed to delete something that was newed somewhere else? Is it because it was newed outside the current scope?

Second, freeing the space works fine, but obviously not the right thing to do because I didn't malloc but newed!

What am I doing wrong? How to properly kill that removed node?

Edit1: Made it more descriptive according to replies to my post Edit2: Rule of 3 methods added

Answer 1

You cannot use free() if you allocated memory with new

You can use malloc() with free() or new with delete. You also should not use malloc() and free() with objects as they do not call the objects constructor and destructor unlike new and delete

So since you allocated the nodes with new we can change

free(temp); //Here is my problem!!!

to

delete temp;

Answer 2

This is an addendum to other answers which correctly identify and solve the OP's immediate problem. A quick walk-though is required to explain what will happen next.

  Node<T> *temp = cur;
  prev->next = cur->next;
  cur = cur->next;
  cur->prev = prev;

At this point temp has not been cleared so next and prev still point to the two nodes that used to surround cur and are now linked together. This will cause serious problems if not for:

  free(temp); //Here is my problem!!!

free fails because the value pointed to by temp was allocated by new. NathanOliver's answer has this covered, but lurking beneath is what will happen with

delete temp;

This will invoke the destructor to clean up like a good little object. Unfortunately the Node destructor looks like this:

~Node() { delete next; delete prev; }

temp->next still points to a live node. So does temp->prev.

Next and prev get deleted. Which calls their destructors, deleteing the two nodes they touch, and invokes a deathstorm that will destroy all of the Nodes in the list.

If the double-deletes don't kill the program first. Each linked node will try to delete the node that just deleted them. Not good.

Odds are pretty good that the Node destructor should just look after itself and leave the destruction of the other Nodes to the LinkedList class.

Answer 3

In this case having the code of the constructor and also of destructor is very important.

By default you should use "new" in constructor and "delete" in destructor. To be more specific, is very important to allocate all the resources you need in constructor and deallocate in destructor, this way you ensure you don't have any memory leaks.

free(temp);//You don't need this, also you don't need delete.

Please post your constructor and your destructor code.

L.E:

I think you should do something like this:

template<class T>
class LinkedList
{
  private:
    struct Node {
        T m_data;
        Node *m_next;
        Node *m_prev;
    };
    Node* m_first;
    Node* m_last;
    int m_count;
  public:
    LinkedList();
    ~LinkedList();
    T GetFirst();
    T GetLast();
    void AddNode(T node);
    void RemoveAt(int index);
    T GetAt(int index);
    int Count();
};

//constructor
template <typename T>
LinkedList<T>::LinkedList(){
    m_count = -1;//-1 == "The list is empty!
}

template <typename T>
void LinkedList<T>::AddNode(T data){
    Node* temp = new Node; //new is called only here -> delete is called in destructor or in RemoveAt
    temp->m_data = data;
    if (m_count > -1){//If the list contains one or more items
        temp->m_next = m_first;
        temp->m_prev = m_last;
        m_last->m_next = temp;
        m_last = temp;
        m_count++;
    }
    else if (m_count == -1)
    {//If no items are present in the list
        m_first = temp;
        m_first->m_next = temp;
        m_first->m_prev = temp;
        m_last = temp;
        m_last->m_next = temp;
        m_last->m_prev = temp;
        m_count = 0;
    }
}

template <typename T>
void LinkedList<T>::RemoveAt(int index){
    if (index <= m_count)//verify this request is valid
    {
        Node* temp = m_last;
        for (int i = 0; i <= index; ++i){
            temp = temp->m_next;
        }
        temp->m_prev->m_next = temp->m_next;
        temp->m_next->m_prev = temp->m_prev;
        delete temp;
        m_count--;
    }
}

template <typename T>
T LinkedList<T>::GetAt(int index)
{
    Node* temp = m_first;
    if (index <= m_count && index > -1){
        int i = 0;
        while(i < index){
            temp = temp->m_next;
            i++;
        }
    }
    return temp->m_data;
}

template <typename T>
T LinkedList<T>::GetFirst() {
    return m_first->data;
}

template <typename T>
T LinkedList<T>::GetLast(){
    return m_last->data;
}

template <typename T>
int LinkedList<T>::Count(){
    return m_count;
}

template <typename T>
LinkedList<T>::~LinkedList(){
   while(m_count > -1){
        Node* temp = m_first;
        m_first = temp->m_next;
        delete temp;//delete in destructor
        m_count--;
    }
}

Answer 4

So why I am not allowed to delete something that was newed somewhere else? Is it because it was newed outside the current scope?

You're allowed to delete things newed elsewhere. This is not a problem as long as you're pointing to a valid address. The seg-fault shows up when you then try to use one of the pointers that was pointing to the now-deleteed address.

Second, freeing the space works fine, but I feel that's like cheating!

You should only use free when you've used malloc and delete when you've used new. You should never mix them up. Also, after you delete, get into the habit of assigning NULL to the pointer variable.

Because I may need things to be done on my Node's destructor.

The class encapsulating the data should also do its own memory management internally. For example, the LinkedList should manage the memory of the nodes.

This means that, if you have something similar to LinkedList<T>::add(Node<T> *data), you should instead consider something like LinkedList<T>::add(T data); then the list class itself takes care of handling the nodes.

Otherwise, you run the risk of sending a newed node into the list, then the list deletes it internally at some point, but somewhere else, the list's client still holds a reference to a now-invalid node. When the client tries to use it, bam!: seg-fault again.

What am I doing wrong? How to properly kill that removed node?

Besides mixing the news with frees, the segfault is likely being triggered by an invalid memory access, so you should find the pointer that is no longer pointing to valid (i.e. undeleted) memory.

Answer 5

There are many answers stating that new/delete and malloc()/free() should always be paired so, but I feel that it should be said why exactly.

As in your initializations,

Node<char> *h = new Node<char>('H');

the new keyword returns a fully typed object object pointer, defined in the statement itself, while malloc() simply allocates a given memory space without assigning a type, and so returns a void*. Additionally, new/delete deal with memory from the Free Store while malloc()/free() allocate/free memory on the Heap, although some compilers might implement new/free in terms of malloc()/free in some instances.

Also of some importance is that new and delete call the constructor and destructor respectively, while malloc() and free() simply allocate and deallocate heap memory, with no regard for the state of the memory itself.