Corruption of the heap in C++

Question

I'm a beginner programmer in C++. Recently, I started working on image processing using C++. I am trying to define and use the following function:

Matrix MVE(Matrix R, double tolerance)
{
    int n = R.Y();
    Matrix P(n,4);
    for (int i = 0; i < n; ++i)
    {
            P[i][0] = R[i][0];
            P[i][1] = R[i][1];
            P[i][2] = R[i][2];
            P[i][3] = 1.0;
    }
    Matrix *Q = P.T();
            double err = 1.0;
    MatVectorCol u(n);                      
    u.Set(1.0 / n);
    MatVectorCol Mv(n);                 

    while (err > tolerance)                         
    {
            Matrix uM(n, n);
            uM.SetDiagonal(u);
            Matrix *X = *Q * uM;
            Matrix *X1 = *X * P; 
            Matrix invX(4, 4);
            invX = *X1->Inverse();
     delete X;  // Error here!
            delete X1;
    }

    return invX;
}

but I get this error when I the program executes the line "delete X;" :

Windows has triggered a breakpoint in plappd.exe.

This may be due to a corruption of the heap, which indicates a bug in plappd.exe or any of the DLLs it has loaded.

This may also be due to the user pressing F12 while plappd.exe has focus.

The constructors and operations for Matrix class are define as following:

Matrix::Matrix(int _y, int _x) 
{ 
    x = _x; 
    y = _y; 
    M = new double [x * y]; 
    buff = new double [x]; 
    memset0_64(M, sizeof(double) * (x * y));
}

Matrix *Matrix::Transpose()
{
    Matrix *b = new Matrix(x, y);
    for (int i = 0; i < x; ++i)
    {
            double *b_line_i = (*b)[i];
            for (int j = 0; j < y; ++j)
                    b_line_i[j] = (*this)[j][i];
    }
    return b;
}

Matrix *Matrix::operator * (const Matrix &m)
{
    if (x == m.y)
    {
            Matrix *b = new Matrix(y, m.x);
            for (int i = 0; i < y; ++i)
            {
                    double *b_line_i = (*b)[i];
                    double *line_i = (*this)[i];
                    for (int j = 0; j < m.x; ++j)
                            for (int k = 0; k < x; ++k)
                                    b_line_i[j] += line_i[k] * m(k, j);
            }
            return b;
    }
    else
            return 0;
}

There is, for sure, something wrong with the code, but I can't figure out where it is and how to solve it.

Thanks in advance.

Did you implement a **destructor** for your Matrix class, I don't see one above ? **delete** will call it. — samus, Sep 24 '13 at 13:56
The wrong thing about your code is that you dynamically allocate new matrix within `operator*` without any kind of smart pointer to control its destruction. Stop doing that, and things will be much simpler. If you insist on dynamically allocating result, do not do that within `operator*`. — SigTerm, Sep 24 '13 at 13:58
First, don't use `new` unless you really have to (in particular, `operator*` should return by value). Then learn about the [Rule of Three](http://stackoverflow.com/questions/4172722) for when you really do need dynamic allocation. — Mike Seymour, Sep 24 '13 at 13:59
Also, your assignment operator probably doesn't do what you think it does — Hulk, Sep 24 '13 at 14:06
Having your operators return a pointer to a dynamically allocated object is not a good idea. Consider trying to do `A * B * C`, this expression has an intermediate value that you have no pointer for and __can't__ delete. Your design will leak resources. — Blastfurnace, Sep 24 '13 at 14:18

score 2 · Answer 1 · answered Sep 24 '13 at 14:09

The wrong thing about your code is that you overuse/abuse operator new and dynamic memory allocation. Do not dynamically allocate memory unless you have to - this is error-prone, and if you're a "beginner" you should definitely avoid doing that. (unless you worked with dynamically-allocated memory in another language and know what you're doing).

Instead of allocating doubles with new, use std::vector<double> or similar container. This way you won't have to write destructor.

If your class dynamically allocates memory, it MUST have destructor. C++ has no garbage collection, so nobody is going to clean up your mess and release resources for you.

If your class has destructor, it should obey rule of three. You should define copy constructor and assignment operator. Failure to do so will lead to memory leaks, undefined behavior and crashes.

Do not return raw pointers from within operators. Do not return dynamically allocated objects from operators (create separate function if you have to return dynamically allocated object). If you have to return dynamically allocated objects, use smart pointers, so their destruction will be automatic (std::shared_ptr or boost::shared_ptr should work).

Traditionally, operator* should return object by value. It should look like this:

Matrix Matrix::operator* (const Matrix &arg1, const Matrix &arg2){...}|

Corruption of the heap in C++

1 Answers1