Copying a Pointer to Allocated Memory

Question

This is a general programming question, and I hope the answers will offer an alternative approach to the problem rather than a quick fix or hack. I have two objects, each of which has some pointers to allocated memory. I want to copy some internal information from one object to the other. Since the information is significantly large, I just want to copy the pointer. The problem is that when the destructor of the two objects are called, they each call the destructor on the internal information (which is now in both objects). This leads to the destructor being called twice on the same pointer.

Since this is quite a complex scenario, and it wouldn't be practical to show you the whole code. I have devised a simple example to illustrate the root of the problem. The code attaches two pre-existing lists without copying any data. As the output shows, the destructor gets called on the last two nodes multiple times. (Once as K is destroyed and again as L is destroyed, since both lists have a pointer to those nodes).

#include <iostream>

struct Node {
    int data;
    Node * next;
};

class List {
    public:
        List(const int);
        ~List();
        void append(const int);
        void append(const List&);
        void print()const;
    private:
        Node * head;
        Node * tail;
};

List::List(const int x)
{
    Node * q = new Node;
    q->data = x;
    q->next = 0;

    head = q;
    tail = q;
}

List::~List()
{
    while (head != 0){
        Node * temp = head->next;
        std::cout << "Deleting " << head->data << std::endl;
        delete head;
        head = temp;
    }
}

void List::append(const int x)
{
    Node * q = new Node;
    q->data = x;
    q->next = 0;
    tail->next = q;
    tail = q;
}

void List::append(const List& L2)
{
    this->tail->next = L2.head;
    this->tail = L2.tail;
}

void List::print()const
{
    for (Node * iter = head; iter; iter=iter->next){
        std::cout << iter->data << " ";
    }
    std::cout << std::endl;
}

int main()
{
    List L = List(1);
    L.append(3);
    std::cout << "List L:\n";
    L.print();

    List K = List(5);
    K.append(10);
    std::cout << "List K:\n";
    K.print();

    L.append(K);
    std::cout << "List L:\n";
    L.print();
}

The output is:

List L:
1 3 
List K:
5 10 
List L:
1 3 5 10 
Deleting 5
Deleting 10
Deleting 1
Deleting 3
Deleting 0
Deleting 39125056

Answer 1

Using std::shared_ptr instead of raw pointers will solve your problem.

Here's your code converted to use std::shared_ptr.

#include <iostream>
#include <memory>

struct Node {
    int data;
    std::shared_ptr<Node> next;
    Node(int d) : data(d), next(nullptr) {}
};

class List {
    public:
        List(const int);
        ~List();
        void append(const int);
        void append(const List&);
        void print()const;
    private:
        std::shared_ptr<Node> head;
        std::shared_ptr<Node> tail;
};

List::List(const int x)
{
    Node * q = new Node(x);
    head.reset(q);
    tail = head;
}

List::~List()
{
}

void List::append(const int x)
{
    Node * q = new Node(x);
    tail->next.reset(q);
    tail = tail->next;
}

void List::append(const List& L2)
{
    this->tail->next = L2.head;
    this->tail = L2.tail;
}

void List::print()const
{
    for (std::shared_ptr<Node> iter = head; iter.get() != nullptr; iter=iter->next){
        std::cout << iter->data << " ";
    }
    std::cout << std::endl;
}

int main()
{
    List L = List(1);
    L.append(3);
    std::cout << "List L:\n";
    L.print();

    List K = List(5);
    K.append(10);
    std::cout << "List K:\n";
    K.print();

    L.append(K);
    std::cout << "List L:\n";
    L.print();
}

The output:

List L:
1 3
List K:
5 10
List L:
1 3 5 10

Update

A bare bones implementation of the std::share_ptr functionality:

template <typename T>
struct SharedPtr
{
   SharedPtr() : dataPtr(new Data(nullptr)) {}
   SharedPtr(T* n): dataPtr(new Data(n)) {}
   SharedPtr(SharedPtr const& copy) : dataPtr(copy.dataPtr)
   {
      dataPtr->useCount++;
   }

   ~SharedPtr()
   {
      dataPtr->useCount--;
      if ( dataPtr->useCount == 0 )
      {
         delete dataPtr;
      }
   }

   void reset(T* n)
   {
      dataPtr->useCount--;
      if ( dataPtr->useCount == 0 )
      {
         delete dataPtr;
      }
      dataPtr = new Data(n);
   }

   T* get() const
   {
      return dataPtr->n;
   }

   T* operator->() const
   {
      return get();
   }

   SharedPtr& operator=(SharedPtr const& rhs)
   {
      if ( this != & rhs )
      {
         dataPtr->useCount--;
         if ( dataPtr->useCount == 0 )
         {
            delete dataPtr;
         }

         dataPtr = rhs.dataPtr;
         dataPtr->useCount++;
      }
      return *this;
   }

   struct Data
   {
      Data(T* in) : n(in), useCount(1) {}
      ~Data() { if ( n != nullptr ) delete n; }
      T* n;
      size_t useCount;
   };
   Data* dataPtr;
};

Answer 2

In the main function, you declared two local variable L, K, they will be deconstructed before program exists.

In your code, you're append the list K to list L, when deconstructing, K is deconstructed first, as the K still holds the pointer the Nodes 1,3, which means this nodes will be free. But the Node 3 in L still holds the pointer to K's head, that's how the error happens.

Put breakpoints in the deconstructors, you'll find out how it occurs.

Answer 3

Instead of using a raw pointer to your nodes, use std::shared_ptr<Node> and remove the explicit delete from your destructor. A shared pointer will keep the Node in memory as long as there is a shared_ptr instance pointing to it in scope. When there are no longer any shared_ptr instances pointing to a Node, the Node will be automatically deleted.