C++: pointer being freed was not allocated

Question

I am cleaning up a toy program I wrote for a class using XCode 5. Part of the assignment was to gain familiarity with dynamic memory allocation (meaning it must use new and delete despite the fact that it would really work fine without it). I am receiving:

HeapOfStudents(67683,0x7fff769a6310) malloc: *** error for object 0x100300060: pointer being freed was not allocated *** set a breakpoint in malloc_error_break to debug

The error happens:

Student::~Student() {
    delete firstName; // <- on this line
    delete lastName;
    delete address;
    delete birthDate;
    delete gradDate;
    delete gpa;
    delete crHours;
}

which is being called...

int main() {

    string line = "";
    vector<Student> students;


    //
    //Create new students from file
    //
    ifstream data_file ("heapData");

    if (data_file.is_open()) {

        while(getline(data_file, line))
            students.push_back(*new Student(line));  <- inside this call here

        data_file.close();
    }

    else return 0;

The full Student class is below.

Importantly, I noticed that while each line gets correctly pushed to the vector, when the next Student gets pushed, the previous Student inside the vector gets corrupted. I see the malloc error always on the third iteration of the loop (the file has 50 lines).

The program runs without exception if I change vector<Student> to vector<Student *> and student.push_back(*new Student(line)) to student.push_back(new Student(line)).

My question then is: Can someone please explain what, on a lower level, is happening inside this loop that creates this error?

My guess is that *new Student(line) uses the same pointer each time, so through each iteration, data gets corrupted because it is being freed (although this explanation raises questions), while new Student returns a new pointer each time, preserving the data that was passed to students. I'm thinking that maybe I need to write a copy constructor... This is just my guess.

Here is the Student class. Address and Date are also custom classes that are fairly similar. I didn't include them because they don't seem to be part of the issue.

//Constructors
Student::Student() {

    firstName = new string("Testy");
    lastName = new string("Testerson");
    address = new Address("000 Street St", "", "Citytown", "State", "00000");
    birthDate = new Date("01/02/9876");
    gradDate = new Date("01/02/6789");
    gpa = new string("10.0");
    crHours = new string("300");

}

Student::Student(string FN, string LN, string L1, string L2, string C, string S, string Z, string BD, string GD, string GPA, string Hr) {

    firstName = new string(FN);
    lastName = new string(LN);
    address = new Address(L1, L2, C, S, Z);
    birthDate = new Date(BD);
    gradDate = new Date(GD);
    gpa = new string(GPA);
    crHours = new string(Hr);

}

Student::Student(string line) {
    set(line);
}

//Destructors
Student::~Student() {
    delete firstName;
    delete lastName;
    delete address;
    delete birthDate;
    delete gradDate;
    delete gpa;
    delete crHours;
}

//Member Functions
void Student::set(string line) {

    firstName = new string(line.substr(0, line.find_first_of(",")));
    line = line.substr(line.find_first_of(",") + 1, string::npos);

    lastName = new string(line.substr(0, line.find_first_of(",")));
    line = line.substr(line.find_first_of(",") + 1, string::npos);

    address = new Address();

    address->setLine1(line.substr(0, line.find_first_of(",")));
    line = line.substr(line.find_first_of(",") + 1, string::npos);

    address->setLine2(line.substr(0, line.find_first_of(",")));
    line = line.substr(line.find_first_of(",") + 1, string::npos);

    address->setCity(line.substr(0, line.find_first_of(",")));
    line = line.substr(line.find_first_of(",") + 1, string::npos);

    address->setState(line.substr(0, line.find_first_of(",")));
    line = line.substr(line.find_first_of(",") + 1, string::npos);

    address->setZip(line.substr(0, line.find_first_of(",")));
    line = line.substr(line.find_first_of(",") + 1, string::npos);

    birthDate = new Date(line.substr(0, line.find_first_of(",")));
    line = line.substr(line.find_first_of(",") + 1, string::npos);

    gradDate = new Date(line.substr(0, line.find_first_of(",")));
    line = line.substr(line.find_first_of(",") + 1, string::npos);

    gpa = new string(line.substr(0, line.find_first_of(",")));
    line = line.substr(line.find_first_of(",") + 1, string::npos);

    crHours = new string(line.substr(0, line.find_first_of(",")));
    line = line.substr(line.find_first_of(",") + 1, string::npos);

}

void Student::printReport() {

    cout << *lastName << ", " << *firstName << ", " << address->getAddress()
    << ", " << birthDate->getDate() << ", " << gradDate->getDate()
    << ", " << *gpa << ", " << *crHours << endl;

}

void Student::printName() {
    cout << *lastName << ", " << *firstName << endl;
}

string Student::getName() {
    return string(*lastName + ", " + *firstName);

EDIT Here's the copy constructors and assignment operators I wrote for the student class should anyone be interested in seeing/critiquing them:

Student::Student(const Student & student){

    firstName = new string(*student.firstName);
    lastName = new string(*student.lastName);
    address = new Address(*student.address);
    birthDate = new Date(*student.birthDate);
    gradDate = new Date(*student.gradDate);
    gpa = new string(*student.gpa);
    crHours = new string(*student.crHours);

}

Student& Student::operator=(const Student & student) {
    return *new Student(student);
}

Answer 1

It looks like you're not following the Rule of Three, so Bad Things will happen if you copy a Student object. Specifically, both will contain pointers to the same objects, which both will try to delete - so one will try to delete something that the other has already deleted.

The easiest way to give the class valid copy semantics is to disallow copying, by deleting the copy constructor and copy-assignment operator:

class Student {
    Student(Student const &) = delete;
    void operator=(Student const &) = delete;

    // rest of class...
};

Otherwise, if you want the class to be copyable, implement these to do something sensible.

Also, this causes a memory leak:

students.push_back(*new Student(line));

by dynamically creating a Student, copying it into the vector, and discarding the only pointer to the dynamic object. If you're storing objects, then push a copy of a temporary:

students.push_back(Student(line));

Alternatively, you could change the container type to vector<Student*>, and remember to delete each object when you remove its pointer. (That's a reasonable thing to do as an exercise in pointer-wrangling, as you say this is, but don't do it in any program you want to maintain.)

Once you've learnt the grisly details of manual memory management, make life easier for yourself by avoiding dynamic allocation unless absolutely necessary and, when you really do need it, always managing it with smart pointers, containers, and other RAII types, not by juggling raw pointers.

Answer 2

One problem you have is that you are keeping a vector of Student objects, instead of a vector of pointers to dynamically allocated objects (of type Student* therefore).

Simply replace your students variable to std::vector<Student*> students;. From then, you can simply push_back the pointer created by new.

The thing is that vector already deals with memory allocation, so the line making the push (that you highlighted in the code) was making a copy of the Student to a position in the vector. The pointer to the dinamically allocated object would become unreachable after that.

As Barmar pointed out, having a vector of pointer also has the advantage of being able to push pointers of subclasses of Student. A simple example on that:

class PhDStudent : public Student { ... }

students.push_back(new PhDStudent(...));

Furthermore, there are many other tweaks you should consider in your class:

• Your constructor is taking string parameters by-value, which means they are deeply copied from their origin. Using const string& is preferable here, to avoid unnecessary copies.

• As already pointed out by some other answers (by Mike Seymour, cdhowie, Anton Savin and whoever is going to point this out), you should follow the Rule of Three. If you want your class to be copiable, you should also implement the copy constructor and the copy assignment operator. If you're using C++11, you can take advantage of a move constructor and a move assignment as well, so as to reduce the number of allocations when moving those objects.

Answer 3

The issue is likely happening because the default copy and assignment operators do not do the right thing -- they copy the pointers. So if you create a temporary Student somewhere you are going to delete the pointers when that temporary is destructed, while the original still points to the same (now deleted) objects. When it is destructed, it is going to try to delete pointers that were already deleted, hence the error.

You need to follow the rule of three: if you need a custom destructor, assignment operator, or copy constructor, you need all of them.

Add the following public members to your class:

Student(Student const &);
Student & operator=(Student const &);

And define them like so:

Student::Student(Student const & other)
{
    firstName = new string(other.firstName);
    // And so on, for each pointer member.
}

Student & Student::operator=(Student const & other)
{
    *firstName = other.firstName;
    // And so on, for each pointer member.

    return *this;
}

Note that all of this can be avoided by using string firstName; instead of string * firstName; -- in that case, the default destructor, copy constructor, and assignment operator would do the right thing and you wouldn't need to define any of them.

Further, be aware that your Address and Date classes could have the same problem! You have to do these kinds of gymnastics any time you use raw pointers as class members.

Answer 4

It may happen that you don't want Students to be copied, but you still want to put them into a vector. In C++11 it can be solved by just adding a move constructor:

class Student {
public:
    Student() { ...}
    ~Student() {...}
    Student(const Student&) = delete; // optional: will be deleted implicitly

    Student(Student&& other) {
        firstName = other.firstName;
        other.firstName = nullptr;
        // ...
    }
};

This way copy constructor and assignment operator will be implicitly deleted, so you'll not be able to make copies. But vector and other containers will use move constructor just fine. This of course puts certain limitations on Student usage.

vector<Student> students;
students.push_back(Student()); // OK, student moves
Student s;
students.push_back(s); // Error: copy constructor for Student is deleted