Copy constructor memory leaks

Question

I am trying to learn the basics of writing my custom constructors but I cannot figure out what I'm doing wrong. I know that for my purposes it would be enough to let the compiler do its' job, but I'm curious how I could fix my definitions.

#include <iostream>
#include <stdexcept>

class Matrix {
    public:
      Matrix(int rows, int cols);    //my custom constructor
     ~Matrix();                      //my custom destructor
      Matrix(const Matrix& m);       //my custom copy constructor
      Matrix& operator= (const Matrix& m);  //my custom assignment operator
    private:
      int rows_, cols_;
      double* data_;
    };


Matrix::Matrix(int rows, int cols): rows_ (rows), cols_ (cols){  
if (rows == 0 || cols == 0)
    throw std::out_of_range("Matrix constructor has 0 size");
data_ = new double[rows * cols];     
}
 
 
Matrix::~Matrix()       
{
    delete[] data_;
}


Matrix::Matrix(const Matrix& m) : rows_(m.rows_), cols_(m.cols_)
{
data_ = new double[rows_ * cols_];
data_=m.data_;
} 


Matrix& Matrix::operator=(const Matrix& m){     
if(this != &m){
        double* newdata_=new double[m.cols_*m.rows_];
        *newdata_=*data_;
        delete[] data_;
        data_=newdata_;
        rows_=m.rows_;
        cols_=m.cols_;
        }   
return *this;

}
    

Then in the main part of the program:

int main(){

Matrix m1(2,2);//creating a matrix of size 2x2


Matrix m2=m1;  //this doesn't work
Matrix m3(m1); //nor this




return 0;
}

The error upon running the executable is: free(): double free detected in tcache 2

Am I correct in thinking that neither the copy constructor nor the assignment operator lead to the destructor being called? Why is that?

Answer 1

The basic problem is that you are not copying the data after you've allocated the memory in the copy constructor.

Matrix::Matrix(const Matrix& m) : rows_(m.rows_), cols_(m.cols_)
{
    data_ = new double[rows_ * cols_];
    data_ = m.data_;  // <-- This is wrong
} 

The line with the // <-- This is wrong comment not only wipes away the preceding line (where the memory is allocated), it doesn't copy any of the actual data. All it does is copy the pointer value. So you now have data_ and m.data_ pointing to the same memory, thus the memory leak and double-delete errors.

The fix is to actually copy the data to the newly allocated memory.

The other potential error, which is not easy to spot, is that you failed to initialize all of the data. Even if we fixed this to do the copy, you are running into undefined behavior.

Here is the fix to both of these problems:

#include <algorithm>
//...
Matrix::Matrix(int rows, int cols): rows_ (rows), cols_ (cols)
{  
    if (rows == 0 || cols == 0)
        throw std::out_of_range("Matrix constructor has 0 size");
    data_ = new double[rows * cols](); // <-- Note the () to zero-initialize the data    
}

Matrix::Matrix(const Matrix& m) : rows_(m.rows_), cols_(m.cols_)
{
    data_ = new double[rows_ * cols_];
    std::copy_n(m.data_, m.rows_ * m.cols_, data_);
} 

---

There is one more error, and that is in the assignment operator. You are making the same mistake of erroneously using = to do copying, instead of the requisite function to do the copying of data between two buffers.

Matrix& Matrix::operator=(const Matrix& m)
{     
    if(this != &m)
    {
        double* newdata_=new double[m.cols_*m.rows_];
        *newdata_=*data_; // <-- This does not copy the data
        //

The fix is similar to the fix that was done in the copy constructor using std::copy_n. But it is so similar, that you can actually use the copy constructor to do all of this work instead of having duplicate code. This technique of using the copy constructor within the assignment operator is called the copy / swap idiom.

Matrix& Matrix::operator=(const Matrix& m)
{     
  if(this != &m)
  {
     Matrix temp(m);  // <-- Copy is made
     std::swap(temp.data_, data_);
     std::swap(temp.rows_, rows_);
     std::swap(temp.cols_, cols_);
  }      
  return *this;
}

Basically a copy is made, and then we just swap out the current object's data with the copy's data. Then the copy dies off with the old data.