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;

Need the [Rule of Three](http://stackoverflow.com/questions/4172722/what-is-the-rule-of-three). — jxh, Jul 26 '12 at 19:03
@user315052: Yes, I understand that a copy of `a` is pushed onto the vector and I need a deep copy here. I should follow the Rule of Three. However, the code should not throw such errors. Rafael's answer helped me point out the error! — brainydexter, Jul 26 '12 at 19:09
@brainydexter: If I was certain it was the root cause of your crash, I would have posted an answer. As a comment, it indicates a suggestion that you should follow (because it will lead to similar head scratching later if not fixed). Regards — jxh, Jul 26 '12 at 19:12
@user315052 I appreciate you posting the link (and hence +1 :) and guiding me in the right direction. I was aware of the shallow copy happening here which should be fixed.Before I went ahead and did that, I was thinking in my head, Why is this damn thing weeping and crashing on me. :) A len+1 got me there! Nevertheless, thanks! — brainydexter, Jul 26 '12 at 19:15

score 8 · Accepted Answer · answered Jul 26 '12 at 19:03

8

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);
}

answered Jul 26 '12 at 19:03

Rafael Baptista

11,181
5
39
59

I agree with @Rafael! Sigh, I didn't see this one :( – brainydexter Jul 26 '12 at 19:07
@brainydexter Pay attention though that this is by far not the only error. See comments above (which should really be answers) – Konrad Rudolph Jul 26 '12 at 19:07

score 1 · Answer 2 · edited May 23 '17 at 12:01

1

(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)";
    }
};

edited May 23 '17 at 12:01

Community

1
1

answered Jul 26 '12 at 19:34

jxh

69,070
8
110
193

Yes and I (re)read that link to TROT you posted. I hadn't practiced some pointer fun in a while, so I thought I'd give these things a spin again. Using classes like string really makes your life easy. – brainydexter Jul 26 '12 at 19:39
Though sometimes you have to write raw C! – Brian Reinhold Aug 20 '16 at 12:59
@BrianReinhold: A smart pointer would be preferred over raw pointers, and will still let the code stay RoT compliant. – jxh Aug 23 '16 at 17:43

Heap corruption detected after normal block

2 Answers2