munmap_chunk(): invalid pointer in C++ program

Question

I get an error "munmap_chunk(): invalid pointer", I don't know why. Problem appears when I use MultipliedByMatrix method. It can't properly delete the matrix that was created in this method.

#include "matrix.h"

Matrix::Matrix(int matr_size) {
    size = matr_size;
    Matr = new int *[size];
    for(int i = 0; i < size; i++)
        Matr[i] = new int[size];

    for(int i = 0 ; i < size; i++)
        for(int j = 0; j < size; j++)
            Matr[i][j] = rand() % 100;
    std::cout << "New matrix is created" << std::endl;
}

Matrix::~Matrix() {
    for(int i = 0; i < size; i++)
        delete[] Matr[i];
    delete[] Matr;
    Matr = NULL;
    std::cout << "Matrix is deleted" << std::endl;
}

Matrix Matrix::MultipliedByMatrix(Matrix OtherMatr) {
    Matrix new_matr = Matrix(this->GetSize());
    int new_value;

    for(int i = 0 ; i < size; i++)
        for(int j = 0; j < size; j++) {
            new_value = 0;
            new_value += Matr[j][i] * OtherMatr.GetValue(i, j);
            new_matr.SetValue(i, j, new_value);
        }
    return new_matr;
}

int Matrix::GetSize() {
    return size;
}

int Matrix::GetValue(int i, int j) {
    return Matr[i][j];
}

void Matrix::SetValue(int i, int j, int value) {
    Matr[i][j] = value;
}

Answer 1

This is not an analytical answer to the question but a piece of advice with respect to solving (or better circumventing) the problem.

Avoid memory handling on your own if you can. (And it is very likely that you actually can avoid it.)

You can read my answer on the question "1D or 2D array, what's faster?" to get a lengthy explenation why it is probably undesirable to use the kind of memory layout you're using. Furthermore, you'll find an (yet untested) example of how to implement a simple matrix container on top of std::vector.

You can look at the scheme and try to implement your own if you want. The design has several advantages compared to your implementation:

• It is templated and thus usable with int as well as many other types.
• Conformance to the standard container concept is achieved easily.
• No destructor / copy constructor / move constructor or assignment operators required: std::vector is handling the resources and does the "dirty work" for you.

If you still want to use your RAW-Pointer approach (for academic purposes or something):

1. Read What is meant by Resource Acquisition is Initialization (RAII)? and try to understand the answers.

2. Read What is The Rule of Three? properly and make sure you have implemented (preferably obeying the RAII concept) those functions:

   • copy constructor,
   • destructor,
   • assignment operator and if desired
   • move constructor and
   • move assignment operator.
3. Still read my answer to the "1D or 2D array, what's faster?" question to see how you would want to organize your allocations in order to be exception safe in case of std::bad_alloc.

Example: Your constructor the 'a little better' way:

Matrix::Matrix(std::size_t const matr_size) // you have a size here, no sign required
{
  Matr = new int*[matr_size];
  std::size_t allocs(0U);
  try
  { // try block doing further allocations
    for (std::size_t i = 0; i < matr_size; ++i)
    {
      Matr[i] = new int[matr_size]; // allocate
      ++allocs; // advance counter if no exception occured
      for(std::size_t j = 0; j < matr_size; j++)
      {
        Matr[i][j] = rand() % 100;
      }
    }
  }
  catch (std::bad_alloc & be)
  { // if an exception occurs we need to free out memory
    for (size_t i = 0; i < allocs; ++i) delete[] Matr[i]; // free all alloced rows
    delete[] Matr; // free Matr
    throw; // rethrow bad_alloc
  }
}

Answer 2

Did you write the Matrix class yourself? If so, I bet the problem is that you don't have a copy or move constructor. If so, the compiler will have generated one for you. This will copy the values of size and Matr but it won't create copies of the pointed-to arrays. When you write:

    return new_matr;

this creates a new matrix (using the copy constructor - which just copies the pointer), and then calls the destructor of new_matr (which deletes the memory which is pointed to). The calling function is then dealing with junk memory, and when it tries to eventually delete the result, all hell will break loose

You also will need to write a move assignment operator.

Alternatively make Matr a std::vector<int> (of length 'size' squared), and write:

int Matrix::GetValue(int i, int j) {
    return Matr[i*size+j];
}

(and similarly for other functions). std::vector has a proper copy and move constructor, and proper assignment behaviour - so it will all just work. (It will also be a lot faster - you save a whole pointer indirection.)