C++ glibc double-free error

Question

I'm getting this weird error that I haven't been able to figure out. I'm studying for an exam, so I'm using structs instead of classes because that's what we did in class. Here's my code

#include <iostream>
#include <string>
using namespace std;


struct Course;
ostream &operator<<(ostream &os, Course c);


struct Course {
        string name;
        Course(string n) {
                name = n;
        }
        Course(const Course &c) {
                cout << "Copying course" << endl;
                name = c.name;
        }
        Course &operator=(const Course &c) {
                cout << "Assigning course" << endl;
                name = c.name;
                return *this;
        }
};


struct Student {
        int id;
        Course *courses[5];
        int size;
        Student(int num) {
                id = num;
                for (int i = 0; i < 5; i++) {
                        courses[i] = new Course("Course");
                }
                size = 0;
        }
        Student(const Student &s) {
                cout << "Copying student" << endl;
                id = s.id;
                for (int i = 0; i < 5; i++) {
                        Course *temp = new Course(s.courses[i]->name);
                        courses[i] = temp;
                }
        }
        Student &operator=(const Student &s) {
                cout << "Assigning student" << endl;
                id = s.id;
                for (int i = 0; i < 5; i++) {
                        courses[i] = s.courses[i];
                }
                return *this;
        }
        ~Student() {
                for (int i = 0; i < 5; i++) {
                        delete courses[i];
                }
        }
        void print() {
                cout << id << ": " << endl;
                for (int i = 0; i < 5; i++) {
                        cout << courses[i]->name << endl;
                }
        }
        void addCourse(Course *c) {
                delete courses[size];
                courses[size] = c;
                size++;
        }
};

ostream &operator<<(ostream &os, Course *c) {
        return os << c->name << endl;
}

int main() {
        Student one(2342134);
        Course cs246("cs246");
        Course cs245("cs245");
        one.addCourse(&cs246);
        one.addCourse(&cs245);
        one.print();
        Student two = one;
        two.print();
}

Here's the error

2342134:
cs246
cs245
Course
Course
Course
Copying student
2342134:
cs246
cs245
Course
Course
Course
*** glibc detected *** ./a.out: double free or corruption (fasttop): 0x0000000000cd8db0 ***
======= Backtrace: =========
/lib/x86_64-linux-gnu/libc.so.6(+0x7db26)[0x7f359a7ddb26]
/usr/lib/x86_64-linux-gnu/libstdc++.so.6(_ZNSsD1Ev+0x40)[0x7f359adf87c0]
./a.out[0x40111e]
./a.out[0x401152]
./a.out[0x400dc7]
/lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0xed)[0x7f359a78176d]
./a.out[0x400b89]
======= Memory map: ========
00400000-00402000 r-xp 00000000 00:34 45960378                           /u3/z38ahmed/cs246/1161/exam/2.22_Object/2.22.5_Copy-Constructor-and-Assignment/a.out
00601000-00602000 r--p 00001000 00:34 45960378                           /u3/z38ahmed/cs246/1161/exam/2.22_Object/2.22.5_Copy-Constructor-and-Assignment/a.out
00602000-00603000 rw-p 00002000 00:34 45960378                           /u3/z38ahmed/cs246/1161/exam/2.22_Object/2.22.5_Copy-Constructor-and-Assignment/a.out
00cc7000-00cf9000 rw-p 00000000 00:00 0                                  [heap]
7f359a460000-7f359a55b000 r-xp 00000000 fc:00 915519                     /lib/x86_64-linux-gnu/libm-2.15.so
7f359a55b000-7f359a75a000 ---p 000fb000 fc:00 915519                     /lib/x86_64-linux-gnu/libm-2.15.so
7f359a75a000-7f359a75b000 r--p 000fa000 fc:00 915519                     /lib/x86_64-linux-gnu/libm-2.15.so
7f359a75b000-7f359a75c000 rw-p 000fb000 fc:00 915519                     /lib/x86_64-linux-gnu/libm-2.15.so
7f359a760000-7f359a914000 r-xp 00000000 fc:00 915563                     /lib/x86_64-linux-gnu/libc-2.15.so
7f359a914000-7f359ab13000 ---p 001b4000 fc:00 915563                     /lib/x86_64-linux-gnu/libc-2.15.so
7f359ab13000-7f359ab17000 r--p 001b3000 fc:00 915563                     /lib/x86_64-linux-gnu/libc-2.15.so
7f359ab17000-7f359ab19000 rw-p 001b7000 fc:00 915563                     /lib/x86_64-linux-gnu/libc-2.15.so
7f359ab19000-7f359ab1e000 rw-p 00000000 00:00 0
7f359ab20000-7f359ab36000 r-xp 00000000 fc:00 914560                     /lib/x86_64-linux-gnu/libgcc_s.so.1
7f359ab36000-7f359ad35000 ---p 00016000 fc:00 914560                     /lib/x86_64-linux-gnu/libgcc_s.so.1
7f359ad35000-7f359ad36000 r--p 00015000 fc:00 914560                     /lib/x86_64-linux-gnu/libgcc_s.so.1
7f359ad36000-7f359ad37000 rw-p 00016000 fc:00 914560                     /lib/x86_64-linux-gnu/libgcc_s.so.1
7f359ad38000-7f359ae3a000 r-xp 00000000 fc:00 396701                     /usr/lib/x86_64-linux-gnu/libstdc++.so.6.0.21
7f359ae3a000-7f359b039000 ---p 00102000 fc:00 396701                     /usr/lib/x86_64-linux-gnu/libstdc++.so.6.0.21
7f359b039000-7f359b041000 r--p 00101000 fc:00 396701                     /usr/lib/x86_64-linux-gnu/libstdc++.so.6.0.21
7f359b041000-7f359b043000 rw-p 00109000 fc:00 396701                     /usr/lib/x86_64-linux-gnu/libstdc++.so.6.0.21
7f359b043000-7f359b046000 rw-p 00000000 00:00 0
7f359b048000-7f359b06a000 r-xp 00000000 fc:00 915535                     /lib/x86_64-linux-gnu/ld-2.15.so
7f359b268000-7f359b26a000 rw-p 00000000 00:00 0
7f359b26a000-7f359b26b000 r--p 00022000 fc:00 915535                     /lib/x86_64-linux-gnu/ld-2.15.so
7f359b26b000-7f359b26d000 rw-p 00023000 fc:00 915535                     /lib/x86_64-linux-gnu/ld-2.15.so
7f359b26d000-7f359b274000 rw-p 00000000 00:00 0
7ffdfdac4000-7ffdfdae5000 rw-p 00000000 00:00 0                          [stack]
7ffdfdb78000-7ffdfdb7a000 r-xp 00000000 00:00 0                          [vdso]
ffffffffff600000-ffffffffff601000 r-xp 00000000 00:00 0                  [vsyscall]
Aborted

I don't know what glibc means, but I don't see where I'm double-freeing. When I comment out the Student destructor, then the code works fine. Why is that? I have allocated memory for each course, so I'm freeing that memory in the destructor, so I would think that the program wouldn't work without it, but it does. EDIT: One more thing. It doesn't make a difference when I comment out the Student two = one; line, so I'm guessing that the error is in the addCourse() and/or destructor

Sorry that this is such a long question Thanks a lot SO

Answer 1

Your assignment operator is incorrect, as pointed out by the other answers.

However to fix your issue easily, just use copy / swap, since you already coded a copy constructor and destructor for your class:

#include <algorithm>
//...
Student &operator=(const Student &s) 
{
   cout << "Assigning student" << endl;
   Student temp(s);
   std::swap(temp.id, id);
   std::swap(temp.courses, courses);
   return *this;
}

We just created a temporary Student copy from the passed-in Student, and then swapped out the current student's internal members with the temporary's. Again, this can only work with a working copy constructor and destructor for Student (which you already provided).

---

The other issue is the addCourse function. The problem is that you're giving it pointers to objects that were not dynamically allocated. When the destructor is invoked, it assumed that all pointers came from a call to new, when they didn't.

 void addCourse(Course *c) {
        delete courses[size];
        courses[size] = c;
        size++;
    }

~Student() {
    for (int i = 0; i < 5; i++) {
        delete courses[i];  // <-- Assumes all courses were allocated dynamically
    }
}

//...

Course cs246("cs246");
Course cs245("cs245");
one.addCourse(&cs246); // <-- Not allocated with new
one.addCourse(&cs245); // <-- Not allocated with new

This is a flaw in your design. Either disallow this by yelling at main

don't do this, I only take stuff allocated with new", and have this code:

Course* cs246 = new Course("cs246");
Course* cs245 = new Course("cs245");
one.addCourse(cs246);
one.addCourse(cs245);

Live Example

or

Take the courses by reference and copy them (similar to how vector works) and manage the memory yourself in the class.

void addCourse(const Course& c) 
{
    delete courses[size];
    Course* temp = new Course(c);
    courses[size] = temp;
    size++;
}

//...

Course cs246("cs246");
Course cs245("cs245");
one.addCourse(cs246);
one.addCourse(cs245);

Live Example

Answer 2

In Student &operator=(const Student &s) you are copying the pointers to courses from s, so you get double deletion.

I believe for the purposes of your homework you can just do the same as in the copy constructor - create new courses with the same names.

Answer 3

You have problem in your assignment operator:

   Student &operator=(const Student &s) {
            cout << "Assigning student" << endl;
            id = s.id;
            for (int i = 0; i < 5; i++) {
                    courses[i] = s.courses[i];
            }
            return *this;
    }

you copy raw pointers not objects and then 2 separate Student objects own the same Course ones, so on destructor of the second Student same Course destroyed second time. Probably you wanted:

  *(courses[i]) = *(s.courses[i]);

instead.