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?

`*data_ = *m.data_;` will only copy the value of the first element of the array, right? This is actually a case where just letting the compiler do its job is insufficient because it _won't_ perform a deep copy of `data_`. — Nathan Pierson, Mar 20 '21 at 01:19
But basically there's at least three problems: `data_ = m.data_;` copies a pointer, not the value of the array. `*newdata_ = *data_;` also isn't doing a complete copy of an array. And in a couple of places you don't ever actually copy over values from `m`: your copy constructor doesn't set `rows_` or `cols_` in the LHS and you never access `m.data_` in your assignment operator. So you have memory leaks and double deletions happening because you're not actually copying arrays around. — Nathan Pierson, Mar 20 '21 at 01:23
`data_ = new double[rows_ * cols_]; data_=m.data_;` -- You do this in successive lines. You already allocated the data in the first line, but the second line wipes all that all away by changing `data_` again. So it is obvious this is wrong. — PaulMcKenzie, Mar 20 '21 at 01:31
Is the line `data_=m.data_;` in your copy constructor what you actually have in your code? It looks a bit inconsistent when compared to your copy assignment's `*newdata_=*data_;`. (Not that the copy assignment is doing things correctly, although it manages to avoid a double-free...) What do you expect this line of code to accomplish? — JaMiT, Mar 20 '21 at 01:58

PaulMcKenzie · Accepted Answer · 2021-03-20T03:44:21.560

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.

Copy constructor memory leaks

1 Answers1