Destructor of template class causing segmentation fault

Question

This is the short version template class, with the important methods:

template<class T>
class Matrix
{
protected:
    int width;
    int height;
    T ** values;
public:
    Matrix(int width, int height);
    ~Matrix();
    T get(int i, int j);
    void set(int i, int j, T value);
};

template<class T>
Matrix<T>::Matrix(int width, int height) {
  this->width = width;
  this->height = height;

  this->values = new T*[width];
  for(int i=0; i<width; i++)
    this->values[i] = new T[height];
}

template<class T>
Matrix<T>::~Matrix() {
  cout << "~ 1" << endl;
  for(int i=0; i<this->width; i++) {
    cout << "~ 2" << endl;
    for(int j=0; j<this->height; j++) {
      cout << "~ 3" << endl;
      delete [] &this->values[i][j];
    }
    cout << "~ 4" << endl;
    delete [] &this->values[i];
    cout << "~ 5" << endl;
  }
}

template<class T>
T Matrix<T>::get(int i, int j) {
  return this->values[i][j];
}

template<class T>
void Matrix<T>::set(int i, int j, T value) {
  this->values[i][j] = value;
}

In my main project, every time the executable needs delete an object from this class, the program exits with a segmentation fault error.

Below a small program where the error also occurs:

int main() {
  cout << "1" << endl;
  Matrix<int> * matrix = new Matrix<int>(300, 300);
  cout << "2" << endl;
  for(int i=0; i<300; i++)
    for(int j=0; j<300; j++)
      matrix->set(i, j, i+j);
  cout << "3" << endl;
  delete matrix;
  cout << "4" << endl;
  return 1;
}

Anyone can tell me what's wrong with this destructor?

It looks like you're creating your Matrix "sideways." The first allocation should be the rows, and then each row allocates the columns. — sweenish, Mar 30 '22 at 18:53
In the constructor you have 1 + width `new` calls, but in the destructor you have width + width * height `delete` calls. — Fatih BAKIR, Mar 30 '22 at 18:55
Looking closer at the destructor, for each row you should `delete [] values[i]`, and then delete [] values. You're trying to delete each individual element, and then never delete the original pointer. — sweenish, Mar 30 '22 at 18:56
And I'd recommend flattening this to 1D anyway, and having your class do the arithmetic to fake 2D. It will be friendlier to your cache. — sweenish, Mar 30 '22 at 18:57
@sweenish I tried this, but got the error `double free or corruption (!prev)` when I executed the program — Kleber Mota, Mar 30 '22 at 19:05
@KleberMota -- Start over again, and don't call `new` or `delete` a single time. There are container classes, i.e. `std::vector`, that removes the need to do any of the manual memory gymnastics. This simple 2 line `main` program has problems: `int main() { Matrix m(1,1); Matrix m2 = m1; }`. If that simple program has issues, no telling what else is wrong with the `Matrix` class. — PaulMcKenzie, Mar 30 '22 at 19:16
Also: `Matrix * matrix = new Matrix(300, 300);` -- C++ is not Java. `Matrix matrix(300, 300);` is all that should have been done. There is no need to dynamically allocate objects to create them. — PaulMcKenzie, Mar 30 '22 at 19:20
Here's a [simplification](https://godbolt.org/z/56bTGxr9M) that uses a smart pointer. Not only does it help with accidental copies/moves, it reduces the amount of code you have to write a lot. — Ted Lyngmo, Mar 30 '22 at 19:23
And here is [another simplification](http://coliru.stacked-crooked.com/a/361f9a1b37bc2864), and this is without implementing the advice given of making this a 1-D array (which is good advice). The point is that you should start off with something that works, and then improve it later, and the fastest way to start off with something that works is to use the container classes. — PaulMcKenzie, Mar 30 '22 at 19:25
And if you really and truly wanted a `T**` to represent a 2D array, there are much better means of creating one, for example [this](https://stackoverflow.com/questions/21943621/how-to-create-a-contiguous-2d-array-in-c/21944048#21944048). The data is allocated in contiguous memory, only 2 calls to `new[]` and `delete[]` are done to manage the 2D array, instead of your version that allocates each row, thus fragmenting the heap. — PaulMcKenzie, Mar 30 '22 at 19:27

score 1 · Answer 1 · answered Mar 30 '22 at 19:16

Your destructor is coded wrong.

Each new[] needs a corresponding delete[]. Your constructor calls new[] 1 time to allocate the 1st dimension of values, and then calls new[] width times for the 2nd dimension of values.

As such, your destructor needs to call delete[] width times for the 2nd dimension, and call delete[] 1 time for the 1st dimension, eg:

template<class T>
Matrix<T>::~Matrix() {
  cout << "~ 1" << endl;
  for(int i=0; i<width; i++) {
    cout << "~ 2" << endl;
    delete[] values[i];
  }
  cout << "~ 3" << endl;
  delete[] values;
}

Also be aware that your Matrix class is not following the Rule of 3/5/0 to manage the value pointer correctly, as it lacks a copy constructor and copy assignment operator, and a move constructor and move assignment operator. So that will cause problems for your destructor if your Matrix objects are ever copied/moved.

Destructor of template class causing segmentation fault

1 Answers1