Memory leak in a Student pointer Node list in C++

Question

I have a Node list that each Node contains a pointer pointing to a Student variable (this is a class), and a pointer pointing to next Node. Here is my code for insertAtTail.

void studentRoll::insertAtTail(const Student &s) {
    if (head == NULL) {
        this->head = new Node;
        this->head->next = NULL;
        this->head->s = new Student(s);
        this->tail = head;
    }
    else {
        this->tail->next = new Node;
        this->tail = this->tail->next;
        this->tail->next = NULL;
        this->tail->s = new Student(s);
    }
}

I used valgrind to debug, and I got:

==11106== 16 bytes in 1 blocks are definitely lost in loss record 1 of 2
==11106==    at 0x4C2D1CA: operator new(unsigned long) 
(vg_replace_malloc.c:334)
==11106==    by 0x402BE7: StudentRoll::insertAtTail(Student const&) 
(studentRoll.cpp:15)
==11106==    by 0x401CF1: main (testStudentRoll01.cpp:19)
==11106==
==11106== 16 bytes in 1 blocks are definitely lost in loss record 2 of 2
==11106==    at 0x4C2D1CA: operator new(unsigned long) 
(vg_replace_malloc.c:334)
==11106==    by 0x402C5B: StudentRoll::insertAtTail(Student const&) 
(studentRoll.cpp:22)
==11106==    by 0x401E2C: main (testStudentRoll01.cpp:27)
==11106==

Can someone help me with it? I think there are some problems about:

this->head->s = new Student(s);

and

this->tail->s = new Student(s);

But I cannot delete them because I need these "Students." And there are pointers point to "Students."

Thanks!!

Update: here is my destructor

StudentRoll::~StudentRoll() {
    Node *iter = head;
    while (iter) {
        Node *next = iter->next;
        iter->s->~Student();
        delete iter;
        iter = next;
    }
    head = tail = NULL;
}

Answer 1

Can someone help me with it? I think there are some problems about:

this->head->s = new Student(s);

and

this->tail->s = new Student(s);

But I cannot delete them because I need these "Students." And there are pointers point to "Students."

This problem probably indicates that you should redesign your program. In C++, you should express ownership semantics and make clear what objects own what resources and are responsible for their cleanup. Ownership semantics in C++ are expressed via various pointer types:

If a certain, single object owns some heap memory, rather than using raw pointers and new and delete directly, use std::unique_ptr. std::unique_ptr is better because it conveys your intent to readers and uses RAII to help prevent memory leaks.

On the other hand, if an object does not own a piece of memory, use a reference or a raw pointer instead. (In the future, the C++ standard library may get a non-owning smart pointer.)

If your linked list data structure owns the student objects, it should be the one to deallocate them. In this case, use std::unique_ptr:

void studentRoll::insertAtTail(const Student &s) {
    if (head.get() == nullptr) {
        this->head = std::make_unique<Node>();
        this->head->next = nullptr;
        this->head->s = std::make_unique<Student>(s);
        this->tail = &*head; // Get a raw pointer
    }
    else {
        this->tail->next = std::make_unique<Node>();
        this->tail = &*this->tail->next; // Get a raw pointer
        this->tail->next = nullptr;
        this->tail->s = std::make_unique<Student>(s);
    }
}

Instead of using std::unique_ptr, another option would be to simply make Student a data member of your Node type. However, this decision may indicate different intent and have different implications. For example, if you wanted to transfer ownership of the Student object from the Node object to somewhere else, you should use std::unique_ptr. If you kept the Student object directly as a member, you might achieve a similar effect by calling the Student's move constructor, but some of the semantics would still be different. For example, pointers to the Student would be invalidated. See https://stackoverflow.com/a/31724938/8887578 for more comparisons of the two approaches.

If the student objects outlive the linked list, it should not be their owner and would be better off taking a non-owning pointer to such an object. In this case, do not allocate new student objects, but take a pointer from somewhere else:

void studentRoll::insertAtTail(const Student* s) {
    if (head.get() == nullptr) {
        this->head = std::make_unique<Node>();
        this->head->next = nullptr;
        this->head->s = s;
        this->tail = &*head;
    }
    else {
        this->tail->next = std::make_unique<Node>();
        this->tail = &*this->tail->next;
        this->tail->next = nullptr;
        this->tail->s = s;
    }
}

I don't know the context of your program (e.g. if it is a school exercise for writing linked lists), but in serious code, you should use the standard library's std::list instead of rolling your own linked list. In many cases, however, a std::vector (similar to dynamically-grown array) is more appropriate than a linked list.

Additionally, instead of giving Node a parameterless, default constructor and then later assigning its s member, you should pass the student pointer to it in its constructor.