Heap corruption detected after normal block

Question

I have the following code and am not sure why am I getting the heap corruption detected error when it hits the destructor of Myclass. I believe I am deallocating the memory properly ??

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

class MyClass{
private:
    char* mp_str;
public:
    MyClass():mp_str(NULL){}
    ~MyClass(){
        delete [] mp_str;
    }

    void setString(const char* str);
    void printString();
};

int main(){
    MyClass* a = new MyClass();
    std::vector<MyClass> myVector;

    myVector.push_back(*a);

    a->setString("Hello World");
    myVector[0].setString("Goodbye world");

    a->printString();
    myVector[0].printString();

    return 1;
}

void MyClass::setString(const char* const str){
    if(!str)
        return;

    size_t len = strlen(str);

    if(!this->mp_str){
        this->mp_str = new char[len];
        memset(mp_str, 0, len+1);
    }
    strncpy(mp_str, str, len);
}

void MyClass::printString(){
    if(this->mp_str)
        cout << mp_str;
    else
        cout << "No string found";
}

EDIT: (Fixed code)

void MyClass::setString(const char* const str){
    if(!str)
        return;

    size_t len = strlen(str);

    if(!this->mp_str){
        this->mp_str = new char[len+1];
        memset(mp_str, 0, len+1);
    }
    strncpy(mp_str, str, len);
}

in main(), I also added

delete a;

before calling return 1;

Solution 1

You need to allocate the length of the string +1, to account for the null. You are memsetting it right.

if(!this->mp_str){
    this->mp_str = new char[len+1];
    memset(mp_str, 0, len+1);
}

Solution 2

(Posted after Rafael's answer was accepted, as it should be.)

The buffer overrun was definitely the root cause of this particular crash, but the crash could have been avoided by simplifying the implementation while adjusting for adherence to the Rule of Three. To wit, since you implemented a destructor (to deal with mp_str), you should also have implemented a copy constructor and an assignment operator.

But, another way to adhere to TRoT is to avoid needing to implement any of those things at all. In this case, using a std::string instead of a char * would have solved both the crash and be TRoT compliant:

class MyClass{
private:
    std::string mp_str;
public:
    void setString(const char* str) { mp_str = str ? str : ""; }
    void printString() {
        if (mp_str.size()) std::cout << mp_str;
        else std::cout << "(mp_str is empty)";
    }
};