Need advice returning an instance of object using a pointer - bad_alloc at memory location?

Question

I'm having some trouble trying to return an instance from a list of instances from a function.

I have a set where I store instances of my Student class. I'm trying to return an instance from the "find" function to be able to find this instance based on the student name, I am calling this function in another class/file. I want to be able to get this object instance in another file, edit certain properties and have it update in that list of instances.

My find function in Student.cpp:

static set<Student, StudentCmp> studentInstances;

Student* Student::find(int studentId) {
    Student *foundStudent = new Student();
    for (Student s: studentInstances) {
        if(s.getStudentID() == studentId) {
            foundStudent = &s;
        }
    }
    
    return foundStudent;
}

In another class:

*Student::find(1).setStudentName("John");

Yes this student ID does exist in the list, and inside the find function everything is working fine. It finds the ID and the relevant address, however when I try to point to this variable using the address given to me by my find function I get this error:

Unhandled exception at 0x7622DFD8 in Driver.exe: Microsoft C++ exception: std::bad_alloc at memory location 0x006EF044

I'm assuming that the address is going out of bounds? I don't know why but it looks like the address no longer points to my object when it goes out of the function, I looked this up to find answers and people said to use the "new" keyword to keep the object on the heap but this doesn't seem to work for me.

Any advice to achieve what I want to do?

Answer 1

The issue here is that you are returning a pointer not to an object in the set or the pointer you allocated but instead you are returning a pointer to a function local object that is destroyed when you exit the function.

for (Student s: studentInstances) 

Makes a copy of each Student in studentInstances. What you need is a reference like

for (Student& s: studentInstances) 

so that when you return a pointer you are returning a pointer to the object that lives in studentInstances

---

Do note that you also have a memory leak if you find an object because you never deallocate foundStudent if you found a student. Your if statement should be

if(s.getStudentID() == studentId) {
    delete foundStudent;
    foundStudent = &s;
}

---

Personally I would refactor the function to

Student* Student::find(int studentId) {
    for (Student& s: studentInstances) {
        if(s.getStudentID() == studentId) {
            return &s;
        }
    }
    return nullptr;
}

Now you either return a pointer to a valid object or a null pointer. You can check for that in the call site easily and there is no manual memory management and no way to cause a leak either. You just have to ensure you always check for a null pointer result after you call the function.

Answer 2

There's quite a bit wrong with this function. Part of it is using unmanaged/dumb pointers, part of it is using new.

Probably the fundamental problem is where Student objects are supposed to live. At first glance, it makes sense to have them live in studentInstances, a std::set of Students. But you're trying to change the Student objects in a std::set, which is generally a bad idea. Since the order of a std::set is determined by its elements, changing those elements can break the order of the set, and that breaks the entire set.

The easy fix would be to use a set of smart pointers, then. And tha would fix most of your problems - well, except for for (Student s: studentInstances). That makes a copy of each Student. You need to know how in C++, by default everything is copied.