Getting debug assertion errors in program

Question

I'm just trying to create a simple program to practice some C++, but I'm unsure as to why I'm getting this current error. The output provides my desired outcome, but after succesful output I keep getting debug assertion errors. Is it a memory leak or something? I have no idea what it could be.

Header:

#include <iostream>
class Record {
    char* rec;
public:
    Record();
    Record(const char*);
    Record(const Record&);
    ~Record();
    void display(std::ostream&);
    Record& operator=(const char*);
};

std::ostream& operator<<(std::ostream& os, Record& r);

cpp:

#define _CRT_SECURE_NO_WARNINGS
#include "Record.h"

Record::Record() {
    rec = nullptr;
}

Record::Record(const char* s) {
    if(s != nullptr) {
    rec = new char[strlen(s) + 1];
    strcpy(rec, s);
    } else {
    *this = Record();
    }
}

Record::Record(const Record& r) {
    *this = r;
}

Record::~Record() {
    delete [] rec;
}

void Record::display(std::ostream& os) {
    os << rec;
}

Record& Record::operator=(const char* s) {
    if (rec != s)
    delete [] rec;
    if(s != nullptr) {
    rec = new char[strlen(s) + 1];
    strcpy(rec, s);
    } 
    else {
    rec = nullptr;
    }
    return *this;
}

std::ostream& operator<<(std::ostream& os, Record& r) {
    r.display(os);
    return os;
}

Main:

#include <iostream>
#include "Record.h"
using namespace std;

int main() {
    Record rec1("inheritance"), rec2 = rec1;

    cout << rec1 << endl;
    cout << rec2 << endl;
    rec1 = "overloading";
    cout << rec1 << endl;
    rec2 = rec1;
    cout << rec2 << endl;

    return 0;
}

Answer 1

Don't call the assignment operator, *this =, from the const char* and copy constructors. This is generally bad practise. In your case, because you haven't defined an assignment operator that takes a const Record&, the default assignment operator is called, which simply copies the pointer and does a shallow copy, meaning that both Records will have the same pointer in their recs -- this was pointed out and described in detail in PaulMcKenzie's answer. However even if you did define that assignment operator, if you did the same thing as your existing assignment operator, the member variable rec would be uninitialized and deleteing it causes undefined behaviour.

See Calling assignment operator in copy constructor for a discussion, and what you could do instead of calling the assignment operator from a constructor.

Edit

One way to make your program work would be for your constructors to look something like this.

void Record::InitFrom(const char* s)
{
    if(s != nullptr) {
        rec = new char[strlen(s) + 1];
        strcpy(rec, s);
    } else {
        rec = nullptr;
    }
}

Record::Record(const char* s) {
    InitFrom(s);
}

Record::Record(const Record& r) {
    InitFrom(r.s);
}

You could also incorporate the new InitFrom method into your assignment operator.

Record& Record::operator=(const char* s) {
    if (rec != s) { // this test only really necessary if assigning from Record
        delete [] rec;
        InitFrom(s);
    }
    return *this;
}

You also might have an assignment operator that takes a const Record&. You do need the destructor.

Answer 2

I'll put this as an answer, since it is important in how your class behaves and concluding that "everything is working" is one of the reasons why C++ is not that easy of a language.

The main() program you wrote does not test something very simple. Look here:

int main() {
   Record rec1("inheritance");
   Record rec2 = rec1;
}

If you debug this code, you will see that this function is called for the rec2 = rec1 line:

Record::Record(const Record& r) {
    *this = r;
}

Ok, so the copy constructor is called. But what does this line of code do?:

*this = r;

It does not call the assignment operator that you wrote that takes a const char*. Instead, it calls the default assignment operator that takes a Record&, The problem is that -- you didn't write one. So what winds up happening is that the compiler generated assignment operator is called, which does a shallow copy.

In the main() program, when main() returns, both rec2 and rec1 will have their respective destructors called. The problem is that rec2 will delete the pointer value, ok, but then rec1 will delete the same pointer value (no good), causing a corruption of the heap. I ran your code with Visual Studio 2013, and immediately an assertion dialog popped up when main() returned.

So you need to write a user defined assignment operator that takes this signature:

Record& Record::operator=(const Record&)

Answer 3

It's difficult to say for sure what's going one without seeing the value with which you have initialized the Record. Based on your description of the program's behavior and the code, I can think of one possible explanation of what's happening, although there might be others.

My guess is the std::ostream is handing the char* like a c-string, which expects a sequence of charcters terminated by a \0. If your Record has been initialized with a sequence of characters that doesn't terminate with a \0, it would keep doing pointer incrementation, streaming out a character at a time, until it reaches an invalid section of memory. This will cause undefined behaviour, which might trigger a debug assertion in your implementation of the standard (i.e. the compiler you are using).

I provide you this guess as you say this is a learning exersize, so I don't question your class design but try to help you understand what's happening. The comments elsewhere are quite relevant however. If you have record store a std::string instead of a character array, this problem (and several other possible problems) will not occur. That answer may not help you lean what you are trying to learn of course.