C++ Heap Corruption Detected error occurred

Question

When I tried to operate my code, it's executed well except Debug Error popping out. Error message is here.

And my Full Code is here.

#include <iostream>
using namespace std;

class String {
public :
    String() { 
        strData = NULL; 
        len = 0;
        cout << "constructor executed !" << endl;
    }

    String(const char *str){
        len = strlen(str);
        strData = new char[len+1];
        strcpy_s(strData, sizeof(str), str);

        cout << sizeof(str) << endl;
        cout << sizeof(len) << endl;
        cout << sizeof(strData) << endl;
        cout << strData << endl;
    }
    ~String() {
        delete[] strData; //Fails here!
    }
    char* GetStrData() const{ 
        return strData;
    }
    int GetLen() const {
        return len;
    }
private : 
    char* strData; 
    int len;
};

int main() {
    String str; 
    str = String("Hi");
}

and result is here

I presumed this message is because of size of strData but I don't know exactly.

I need help from you guys. thanks.

Answer 1

This line:

str = String("Hi");

is (very roughly) the equivalent of:

String tmp("hi")
str.strData = tmp.strData;
str.len = tmp.len;

So when both tmp and str get deleted, you end up calling delete twice on the same address.

You need to write a custom operator=() method in your String class to handle that.

Once you've opened that pandora's box, you should be crossing your Ts and dotting your Is correctly. This is known as the rule of 0/3/5. See this question for details:

Answer 2

Short Answer:

The str = String("Hi"); statement invokes the default copy-assignment operator (implicitly-declared) special member function of your class. While destruction (two objects are being created in your program), the same memory will be destroyed twice because two objects in your program points to the same memory and they both consider themselves as the owner of the memory.

Long Answer:

Since, you haven't defined the copy-assignment operator and compiler generated version would copy the pointers (shallow copy) to the other class.

If no user-defined copy assignment operators are provided for a class type (struct, class, or union), the compiler will always declare one as an inline public member of the class.

The implicity-declared copy-assignment operator generated by compiler would look like:

String& operator=(const String& other)
{
    strData = other.strData;
    len = other.len;
    return *this;
}

So, the implicitly-declared copy-assignment operator member function does the shallow copy and hence, it just copy the pointer's (not their value). Before the end of str = String("Hi"); statement, you will be having two String objects (str object and temporary object created by String("Hi")) and their strData will be pointing to the same character array. When the temporary generated by String("Hi") will be destroyed, it will free the memory and when str object will be destroyed, it will also try to free/delete the same memory and hence, you are getting your heap corrupted.

Learn about copy-and-swap idiom. Try this:

String& operator=(String other)
{
    swap(other, *this);
    return *this;
}

void swap(String& first, String& second)
{
    std::swap(first.strData, second.strData);
    std::swap(first.len, second.len);
}

Try the program below. I have commented out the copy-assignment operator to demonstrate a pointer getting destroyed twice.

#include <iostream>

#include <string.h>

class String {
public :
    String(void): strData(nullptr), len(0)
    { 
        std::cout << "constructor executed !" << std::endl;
    }

    String(const char *str)
    {

        len = strlen(str);
        strData = new char[len + 1];
        std::copy(str, str + len + 1, strData);
        std::cout << "Constructing memory: " << strData << std::endl;
        std::cout << sizeof(str) << std::endl;
        std::cout << sizeof(len) << std::endl;
        std::cout << sizeof(strData) << std::endl;
        std::cout << strData << std::endl;
    }

 //    String& operator=(String other)
    // {
    //  swap(other, *this);
    //  return *this;
    // }

    ~String()
    {
        if (strData == nullptr)
            return;

        std::cout << "Destroying memory: " << (void *)strData << std::endl;
        delete[] strData; //Fails here!
    }

    char* GetStrData(void) const
    { 
        return strData;
    }

    int GetLen(void) const
    {
        return len;
    }

    void swap(String& first, String& second)
    {
        std::swap(first.strData, second.strData);
        std::swap(first.len, second.len);
    }

private : 
    char* strData; 
    int len;
};

int main() 
{
    String str; 
    str = String("Hi");
    return 0;
}

Output:

constructor executed !
Constructing memory: Hi
8
4
8
Hi
Destroying memory: 0x7fffc5628080
Destroying memory: 0x7fffc5628080

Answer 3

In your constructor, the line strcpy_s(strData, sizeof(str), str); is problematic. The second parameter is supposed to be the size of the destination buffe len + 1 as you used in the new operator. Remember sizeof(strData) is the size of the pointer, it's always 8 bytes for 64bit machines or 4 bytes in 32bit machines. It is not the length of the string.