Assigning object to itself cauing strange problems

Question

I'm working on a string class in C++ using the C standard library.

The class is fairly simple: The constructor takes a string and allocates the amount of memory required for it into a char pointers using malloc, the constructor frees it, and if required, it reallocs. The rest is reading/writing functions that don't require more or less memory.

One function I made changes the contents to lower case, puts it in a new object (created on the stack) and returns it. However, I tried doing something along the lines of: str = str.toLower(); and needless to say, it failed horribly.

Every other time I launch the exe it crashes. After hours after mindlessly searching, I made message boxes tell me when something is allocated and freed and what it is. Turns out for some reason, it kept allocating and deallocating various items from where ever. I'm assuming it was due to a dangling pointer, so I fixed that.

However, it's still allocating and freeing a bunch of things and crashing.

Here is the relevant code:

Constructor/Destructor:

String::String(const char* str)
{
    data = nullptr;

    MessageBox(NULL, str, "Allocating", MB_OK);

    //getSizeOfCharArray does not include the NULL at the end, so add 1
    data_size = getSizeOfCharArray(str)+1;
    data = (char*)malloc(sizeof(char)*data_size);
    strcpy(data, str);
}

String::~String()
{
    MessageBox(NULL, data, "Freeing", MB_OK);

    if (data != nullptr)
    {
        free(data);
        data = nullptr;
    }
}

Lowercase function:

String String::toLower()
{
    char* lower = (char*)malloc(sizeof(char)*data_size);
    for (int i = 0; i < data_size; i++)
    {
        lower[i] = tolower(data[i]);
    }
    String temp(lower);
    free(lower);
    return temp;
}

Main:

int main()
{
    String str("String contents");

    str = str.toLower(); /* This line is what causes everything
                             to go wrong. But why? */
    cout << str.c_str() << endl; //c_str returns a const char* pointer

    //Alternatively, I can do this and it's fine.
    cout << str.toLower().c_str() << endl;
}

The boxes go as follows:

Allocating "String contents"

Allocating "string contents"

Freeing "string contents"

Allocating "/z" (actually some other character that looks like z)

Freeing "/z" (same strange character)

Allocating "/z" (same)

Freeing "/z" (they're all not z)

Allocating "/z"

Freeing "/z"

Allocating "/z"

Freeing "/z"

Allocating "/z"

Freeing "/z"

Allocating "/z"

Freeing "/z"

Allocating "/z"

Freeing "/z"

I have got other dialog text, but apparently "/z" is a common one.

It goes on like this for a bit.

What could be possibly causing this?

Answer 1

If you don't have the operator= overloaded a shallow copy will be done instead of a deep copy. I think it would be great if you can paste the operator=.

Answer 2

What you're attempting makes no sense to attempt, but okay.

In C++, if you don't explicitly declare an assignment operator overload for your classes, one is implicitly created for you which performs a shallow copy. The same goes for a copy constructor and default constructor and destructor. The presence of any other user-defined constructors will inhibit the default constructor from being implicitly created for you, so if you still want it to exist in addition to your constructor that takes a const char*, you'll have to explicitly declare it.

When using dynamic memory in classes, we have what we like to call the Rule of 3 or the Trilogy of Evil. What this means is that you must explicitly write a (1)destructor or you risk leaking memory, and you must explicitly write or delete a (2)copy constructor and (3)copy assignment operator, or else you risk sharing the same memory across multiple instances, which will at least cause the same memory to be deleted more than once if not other undesirable behavior as well.

Since you didn't post what your String class looks like, here's a sample of what you might need to do:

class String
{
public:
    String();    // Default constructor
    String(const char* s);    // Custom constructor
    String(const String& rhs);    // Copy constructor
    String& operator=(const String& rhs);    // Copy assignment operator
    ~String();    // Destructor
    String(String&& rhs);    // (Optional) Move constructor
    String& operator=(String&& rhs);    // (Optional) Move assignment

    // TODO: Other public functions

private:
    char* m_string;
};

String::String() : m_string(nullptr)
{
}
String::String(const char* s)
{
    // TODO: Copy the string into m_string
}
String::String(const String& rhs)
{
    // TODO: Deep copy
}
String& String::operator=(const String& rhs)
{
    if(this != &rhs)
    {
        // TODO: Deep copy
    }
    return *this;
}
String::~String()
{
    delete m_string;
}

Also, and this was just bugging me, you don't need to use Windows message boxes as a logger. Just output log messages to a console or log file.

Answer 3

Your string class requires a working, user-defined copy constructor and assignment operator.

Since the function toLower returns a String by value, then the object requires correct copy semantics due to operator= for the String class being called.

Without a user defined copy constructor, the compiler's default copy constructor will be used. The default version merely copies the pointer value to the new String object -- it doesn't know how to call malloc, and then strcpy to copy over the data. You have to write this yourself.

String::String(const String& str) : data_size(str.data_size),   
                                    data(malloc(str.data_size))
{  strcpy(data, str.data); }

The assignment operator would also need to be done. It should look something like this:

#include <algorithm>
//...
String& operator=(String str) 
{  
    std::swap(data, str.data);
    std::swap(data_size, str.data_size);
    return *this;
}

All of this is explained in the link below:

What is The Rule of Three?

Also, for C++, you use new[] and delete[] instead of malloc and free.