C++ double free or corruption (out): Even with copy constructor and assignment operator

Question

my code is producing * Error in `./a.out': double free or corruption (out): 0x00007ffe400eb0e0 *

Whenever it's run, i assumed this was an issue based on either my copy constructor or how I delete my dynamic array but i'm having trouble working out where the issue arises:

My Class:

class Student {
  public:
    Student();
    Student(const Student&);
    Student & operator= (const Student&);
    ~Student();
    void setStudentData(int *, int &, int, string);
    int getNumOfSubjTaken();
    int getAverageMark();
    int getLowestMark();
    int getHighestMark();
    string getFullName();
    void sortMarks(int &);

  private:
    string fullName;
    int *marks;
    int numOfSubjects;

};

Copy Constructor:

Student::Student(const Student& pupil) {
  marks = new int[numOfSubjects = pupil.numOfSubjects];
  for (int i = 0; i < numOfSubjects; i++) {
   marks[i] = pupil.marks[i];
  }
  fullName = pupil.fullName;
  //removed after edit: marks = pupil.marks;
}

Assignment Operator:

Student &Student::operator=(const Student &pupil) {
 if (this != &pupil) {
   for (int i = 0; i < numOfSubjects; i++) {
     marks[i] = pupil.marks[i];
     }
   fullName = pupil.fullName;
    numOfSubjects = pupil.numOfSubjects;
    marks = pupil.marks;
 }
  return *this;

}

Deconstructor:

Student::~Student(){
 if (marks != NULL) {
   delete [] marks;
 }
  marks = NULL;
  numOfSubjects = 0;
  fullName = "";
}

Set Function (Mutator):

 void Student::setStudentData(int *markArray, int &numStudents, int numSub, string fullName) {

  marks = new int[numSub];
  for (int i = 0; i < numSub; i++) {
    marks[i] = markArray[i];
  }

   this->numOfSubjects = numSub;
   this->fullName = fullName;
}

And it was only after i implemented my write function that this issue was occurring:

void writeFile(fstream &fout, char *argv[], Student *pupil, int &numRecs)        {
  const char sep = ' ';
  const int nameWidth = 5;
  const int numWidth = 7;

  fout.open(argv[2]);

 if (!fout.good()) {
    cout << "Error: Invalid data in " << argv[1] << " file." << endl;
    cout << "The program is terminated.";
    exit(EXIT_FAILURE);
  }
  else {
    // creating the table output
    fout << left << setw(nameWidth) << setfill(sep) << "Full Name";
    fout << left << setw(numWidth) << setfill(sep) << "mark1";
    fout << left << setw(numWidth) << setfill(sep) << "mark2";
    fout << left << setw(numWidth) << setfill(sep) << "mark3";
    fout << left << setw(numWidth) << setfill(sep) << "mark4";
    fout << left << setw(numWidth) << setfill(sep) << "average";
    fout << left << setw(numWidth) << setfill(sep) << "min";
    fout << left << setw(numWidth) << setfill(sep) << "max";
    fout << endl;

    for (int i = 0; i < numRecs; i++) { //numRecs being number of records/students
      fout << left << setw(nameWidth) << setfill(sep) << pupil[i].getFullName();
      for (int j = 0; j < pupil[i].getNumOfSubjTaken(); j++) { //writes each mark up to
        //fout << left << setw(numWidth) << setfill(sep) << pupil[i].marks[j];
    //This line doesn't work, but i need to be able to write the marks.
  }
  if (pupil[i].getNumOfSubjTaken() < 4) {
    for (int k = pupil[i].getNumOfSubjTaken(); k != 4; k++) {
      fout << left << setw(numWidth) << setfill(sep) << "  ";
    }
  }
  fout << left << setw(numWidth) << setfill(sep) << pupil[i].getAverageMark();
  fout << left << setw(numWidth) << setfill(sep) << pupil[i].getLowestMark();
  fout << left << setw(numWidth) << setfill(sep) << pupil[i].getHighestMark();
  fout << endl;
}

  }
}

I also can't seem to fout << pupil[i].marks[j]; even though it should work.

Thanks for your time and help.

Where is the destructor? Also, your assignment operator has a memory leak as you failed to deallocate the previous memory (and reallocate new memory). But why do all of that work in the assignment operator when you can simply do [copy / swap](http://stackoverflow.com/questions/3279543/what-is-the-copy-and-swap-idiom)? — PaulMcKenzie, Aug 25 '16 at 02:59
Also, your destructor can simply be `Student::~Student() {delete [] marks;}`. There is no need to check for NULL, and it makes no sense assigning values to members of an object that's being destroyed. — PaulMcKenzie, Aug 25 '16 at 03:29
`marks = pupil.marks;` in the assignment operator is wrong. `setStudentData` leaks any old data, and doesn't set `numOfSubjects`., and doesn't use 2 of its parameters — M.M, Aug 25 '16 at 04:25

score 4 · Answer 1 · edited May 23 '17 at 12:33

Your assignment operator is incorrect, as it only does a shallow copy of the marks pointer. Thus you will get a double-free error since the marks pointer in both objects (this and pupil) will be pointing to the same memory when the destructor is called on these objects.

Note that if you used std::vector<int> marks; instead of int *marks;, then there would be no need for a copy constructor, assignment operator, or destructor, as a std::vector<int> basically does what you're attempting to do in your copy constructor, assignment operator and destructor. The difference would be that std::vector<int> does it safely, efficiently, and all without mistakes.

Having said this, a fix (not the only possible fix) to your code would be to allocate new memory that matches the number of subjects of the passed-in Student object, deallocate the marks memory, and then assign marks to the newly allocated memory with the copied data.

Student &Student::operator=(const Student &pupil) 
{
   if (this != &pupil) 
   {
       // allocate new memory and copy
       int *temp = new int [pupil.numOfSubjects];
       for (int i = 0; i < pupil.numOfSubjects; i++) 
           temp[i] = pupil.marks[i];

       // deallocate old memory and assign
       delete [] marks;
       marks = temp;
       fullName = pupil.fullName;
       numOfSubjects = pupil.numOfSubjects;
   }
   return *this;
}

As an alternate to the above code, since you seem to have a working copy constructor and destructor (necessary for the following to work correctly), an easier solution is to use the copy / swap idiom.

#include <algorithm>
//...
Student &Student::operator=(const Student &pupil) 
{
   Student temp(pupil);
   std::swap(temp.numOfSubjects, numOfSubjects);
   std::swap(temp.marks, marks);
   std::swap(temp.fullName, fullName);
   return *this;
}

This utilizes the copy constructor and destructor to create a temporary object, and then swap out the internals of this with the temporary object's internals. Then the temporary object dies off with the old internals.

Thank you so much for you detailed explanation and fast responses! Unfortunately even after i alter the copy constructor I'm still returned with the same error. — Mako, Aug 25 '16 at 04:03
Then you need to post a [mcve]. Your `setStudentData` function was not posted, and I wouldn't be surprised if this is also faulty. But why does your `Student` class not have a constructor that takes the student data? Regardless, the answer given needs to be done to fix any problem with copying and assignment of the `Student` class. — PaulMcKenzie, Aug 25 '16 at 04:09

Ivan Rubinson · Answer 2 · 2016-08-25T04:18:39.090

If you want an array that can be resized, the STL already provides one. You can use std::vector<int> instead of manipulating memory yourself.

I've read your copy constructor and it seems fine. While we're at it, I think you should also make a move constructor.

In your assignment operator:

for (int i = 0; i < numOfSubjects; i++) {
    marks[i] = pupil.marks[i];
}

fullName = pupil.fullName; numOfSubjects = pupil.numOfSubjects; marks = pupil.marks;

You don't allocate this->mark. What if pupil.marks isn't the same size as this->marks?
The loop can doesn't copy the entirety of pupil.marks - only as much as this->marks can hold.
You copy things to this->marks (which is good, a deep copy) but then you do marks = pupil.marks.
- The deep copy becomes redundant
- It makes both this and pupil "own" the same marks, meaning both will try to deallocate it in their destructor, causing your issue.
- This line introduces a memory leak.

In your destructor:

if (marks != NULL) {
    delete [] marks;
}
marks = NULL;
numOfSubjects = 0;
fullName = "";

Comparing a pointer to NULL is a C way of doing things. It's basically a macro that's usually equals 0. It's more correct to compare to nullptr.
There's no need to set marks to nullptr after you've deallocated it. No point assigning to the rest of the members either.

In your meaty function:

for (int i = 0; i < numRecs; i++) {
    //numRecs being number of records/students
    fout << left << setw(nameWidth) << setfill(sep) << pupil[i].getFullName();
    for (int j = 0; j < pupil[i].getNumOfSubjTaken(); j++) {
        fout << left << setw(numWidth) << setfill(sep) << pupil[i].marks[j];
    }
}

If numRecs is bigger than the size of the pupil array, you get an overflow.

All of those points are a cause for bug(s).

*"It's more correct to compare to `nullptr`."* - Arguably, it's more correct to not compare the pointer against `nullptr`. It is safe to call `operator delete[]` on a `nullptr`, which results in a no-op. — IInspectable, Aug 25 '16 at 21:29

C++ double free or corruption (out): Even with copy constructor and assignment operator

2 Answers2