Read access violation when trying to create copy constructor for linked list

Question

I am having an issue in regards with my linkedlist implementation, where I am trying to create a copy constructor.

// Copy Constructor
List342(const List342& source)
{
    *this = source;
}

List342& operator=(const List342& source)
{
    Node<T>* s_node = nullptr; // Source node
    Node<T>* d_node = nullptr; // Destination node

    if (this == &source)
    {
        return *this;
    }

    // Empty memory on destination
    DeleteList();

    // If the source is empty, return the current object.
    if (source.head_ == nullptr)
    {
        return *this;
    }

    // Copy source node to destination node, then make destination node the head.
    d_node = new Node<T>;
    d_node->data = (source.head_)->data;
    head_ = d_node;
    s_node = (source.head_)->next;

    // Loop and copy the nodes from source
    while (s_node != nullptr)
    {
        d_node->next = new Node<T>;
        d_node = d_node-> next;
        d_node->data = s_node->data;
        s_node = s_node->next;
    }
    return *this;
    
}

For some reason, VS Studio throws a read access violation at me on the line d_node->data = s_node->data despite the while loop trying to prevent this.

The culprit may be lying in DeleteList, but for some reason my other methods, like printing the linkedlist, have no issues after calling DeleteList, as it prints nothing. I'm just wondering if there's any flaws in this DeleteList method.

// Delete all elements in the linked list
// Not only do you need to delete your nodes,
// But because the Node data is a pointer, this must be deleted too
void DeleteList()
{
    // Similar to the linkedlist stack pop method except you're running a while
    // loop until you the is empty.
    Node<T>* temp;
    while (head_ != nullptr)
    {
        temp = head_;
        head_ = head_->next;
        // For some reason if I try to delete temp->data, I keep getting symbols not loaded or debug 
        // assertion errors
        // delete temp->data;
        // What I can do here is set it to null
        // Then delete it. This may have to do how uninitialized variables have random memory assigned
        temp->data = nullptr;
        delete temp->data;
        delete temp;
    }
}

Here is the Node definition:

template <class T>
struct Node
{
   T* data;
   //string* data;
   Node* next;
}

Answer 1

By having Node::data be declared as a pointer, your code is responsible for following the Rule of 3/5/0 to manage the data pointers properly. But it is not doing so. Your copy assignment operator is shallow-copying the pointers themselves, not deep-copying the objects they point at.

Thus, DeleteList() crashes on the delete temp->data; statement, because you end up with multiple Nodes pointing at the same objects in memory, breaking unique ownership semantics. When one Node is destroyed, deleting its data object, any other Node that was copied from it is now left with a dangling pointer to invalid memory.

If you must use a pointer for Node::data, then you need to copy-construct every data object individually using new so DeleteList() can later delete them individually, eg:

d_node = new Node<T>;
d_node->data = new T(*(source.head_->data)); // <--
...
d_node->next = new Node<T>;
d_node = d_node->next; 
d_node->data = new T(*(s_node->data)); // <--
...

However, this is no longer needed if you simply make Node::data not be a pointer in the first place:

template <class T>
struct Node
{
    T data; //string data;
    Node* next;
}

That being said, your copy constructor is calling your copy assignment operator, but the head_ member has not been initialized yet (unless it is and you simply didn't show that). Calling DeleteList() on an invalid list is undefined behavior. It would be safer to implement the assignment operator using the copy constructor (the so-called copy-swap idiom) , not the other way around, eg:

// Copy Constructor
List342(const List342& source)
    : head_(nullptr)
{
    Node<T>* s_node = source.head_;
    Node<T>** d_node = &head_;

    while (s_node)
    {
        *d_node = new Node<T>;
        (*d_node)->data = new T(*(s_node->data));
        // or: (*d_node)->data = s_node->data;
        // if data is not a pointer anymore...
        s_node = s_node->next;
        d_node = &((*d_node)->next);
    }
}

List342& operator=(const List342& source)
{
    if (this != &source)
    {
        List342 temp(source);
        std::swap(head_, temp.head_);
    }
    return *this;
}

However, the copy constructor you have shown can be made to work safely if you make sure to initialize head_ to nullptr before calling operator=, eg:

// Copy Constructor
List342(const List342& source)
    : head_(nullptr) // <--
{
    *this = source;
}

Or:

// Default Constructor
List342()
    : head_(nullptr) // <--
{
}

// Copy Constructor
List342(const List342& source)
    : List342() // <--
{
    *this = source;
}

Or, initialize head_ directly in the class declaration, not in the constructor at all:

template<typename T>
class List342
{
...
private:
    Node<T> *head_ = nullptr; // <--
...
};