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

you might find help here: http://stackoverflow.com/questions/6199729/how-to-solve-munmap-chunk-invalid-pointer-error-in-c?rq=1 — gabbar0x, Dec 10 '15 at 17:33
I think you need a copy constructor that copies the `Matr` arrays. The default copy constructor will just copy the `Matr` pointer, which becomes invalid when the temporary `new_matr` is destroyed. — Barmar, Dec 10 '15 at 17:46
It would be simpler and more efficient if `MultipliedByMatrix` returned a `Matrix*`. It could allocate it with `Matrix *new_matr = new Matrix(this->GetSize())` and return that pointer, without having to copy all the contents. — Barmar, Dec 10 '15 at 17:50
To make your sample code into a [MCVE](/help/mcve), you'll need to inline `matrix.h` and a suitable `main()` into your code. Then folks will be able to compile and run (make sure it's buildable on its own - you'll need to include at least one standard header). Please edit your post when you have a minimal stand-alone program that demonstrates the problem. — Toby Speight, Dec 10 '15 at 19:05
Barmar: If MultipliedByMatrix returns a Matrix*, then the client has to start worrying about resource management, and will run into the same problem. At the very least it should be a std::unique_ptr. A better solution though is to write a move constructor and "return std::move(new_matr);" Matrix already holds a pointer/unique_ptr, so there's no need to copy more than a pointer. — Martin Bonner supports Monica, Dec 11 '15 at 06:16
To restate what the two answers currently have: *Never write "new"* (always use std::make_unique or std::make_shared). — Martin Bonner supports Monica, Dec 11 '15 at 06:17

score 2 · Answer 1 · edited Dec 10 '15 at 18:44

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.)

thank you for the answer, I wrote a copy constructor and it worked — Kateryna Hurenko, Dec 11 '15 at 16:52

score 0 · Answer 2 · edited May 23 '17 at 12:30

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):

Read What is meant by Resource Acquisition is Initialization (RAII)? and try to understand the answers.
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.
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
  }
}

munmap_chunk(): invalid pointer in C++ program

2 Answers2