Segmentation fault but array object is not going out of bounds (C++)

Question

There are other posts about common causes of segmentation faults, but I don't think the built-in array object I've created here (result) doesn't go out of bounds when I assign values to it.
I think this could be helpful to people in the future who have arrays not out of bounds, and I also haven't seen a lot of stuff about making 2D built-in array objects - examples I've seen are almost entirely vectors or std:array objects.

Here is runnable, relevant code:

matrix.h

#ifndef MATRIX_H
#define MATRIX_H

#include <initializer_list>
using std::initializer_list;

typedef unsigned int uint;

class Matrix {

  public:

    Matrix(uint rows, uint cols);  
    ~Matrix();  
    Matrix add(double s) const;  
    const uint numRows() const;  
    const uint numCols() const;  
    double & at(uint row, uint col);  
    const double & at(uint row, uint col) const;  

  private:

    uint rows, cols;
    double ** matrix;

    void makeArray() {
      matrix = new double * [rows];
      for(uint i = 0; i < rows; ++i) {
        matrix[i] = new double [cols];
      }
    }

};

#endif

matrix.cpp

#include "matrix.h"
Matrix::Matrix(uint rows, uint cols) {
  //Make matrix of desired size
  this->rows = rows;
  this->cols = cols;

  makeArray();

  //Initialize all elements to 0
  for(uint i = 0; i < rows; ++i) {
    for(uint j = 0; j < cols; ++j) {
      this->matrix[i][j] = 0.0;
    }
  }
}
Matrix::~Matrix() {
  for(uint i = 0; i < numRows(); ++i) {
    delete[] matrix[i];
  }
  delete[] matrix;
}
const uint Matrix::numRows() const {
  return this->rows;
}

const uint Matrix::numCols() const {
  return this->cols;
}

double & Matrix::at(uint row, uint col) {
  return matrix[row][col];
}

const double & Matrix::at(uint row, uint col) const {
  return matrix[row][col];
}

Matrix Matrix::add(double s) const {
  uint r = this->numRows();
  uint c = this->numCols();

  Matrix * result;
  result = new Matrix(r, c);

  for(uint i = 0; i < r; ++i) {
    for(uint j = 0; j < c; ++j) {
      result->at(i,j) = (this->at(i,j)) + s;
    }
  }

  return * result;
}

main.cpp

#include <iostream>
#include <cstdlib>
#include "matrix.h"

using namespace std;

typedef unsigned int uint;

int main() {

  Matrix * matrix;
  matrix = new Matrix(3, 2); //Works fine

  double scaler = 5;

  matrix->at(2,1) = 5.0; //Works fine

  Matrix r = matrix->add(scaler); //DOESN'T WORK

  return EXIT_SUCCESS;
}

Any ideas why the add function is causing a segmentation fault error? The for-loop I used to fill the result Matrix object doesn't go out of bounds, and I'm not familiar enough with C++ to know what else could be causing it.
Thanks in advance.

There are probably other issues. Don't try to manage dynamic memory allocation yourself, unless you're absolutely sure you need to. — πάντα ῥεῖ, Jul 10 '16 at 19:44
Should we ignore the memory leak in your `add()` member, and just concentrate on a much bigger issue, namely [What is the Rule of 3](https://stackoverflow.com/questions/4172722/what-is-the-rule-of-three) ? — WhozCraig, Jul 10 '16 at 19:44
`Matrix * result; [...] return * result;` why do you do this? Btw I dont see any good reason for a single `*` in your code — 463035818_is_not_an_ai, Jul 10 '16 at 19:51
@WhozCraig Where do you see a memory leak? If anything I thought the issue might be that I'm destroying the Matrix object `result` before I can actually do anything with it, but I'm not positive. — Mazzone, Jul 10 '16 at 19:54
`return * result;` - the object pointed to by `result` is orphaned once the function returns. That statement doesn't retain it, it `copies` it (which gets us back to what I originally said about [What is the Rule of 3?](https://stackoverflow.com/questions/4172722/what-is-the-rule-of-three). And I completelly concur with juan below. A `std::vector` would solve most, if not all, of the problems here. — WhozCraig, Jul 10 '16 at 19:57
Unrelated to the error, but a general suggestion: since you don't want to represent a jagged matrix, and the dimensions are fixed at construction, it may be better to store the data in one single contiguous array. You could further simplify the code by using an `std::vector` as data storage. — juanchopanza, Jul 10 '16 at 19:57
@WhozCraig - I would love to use a vector, trust me. But this is how we're being required to write it for school. Pretty standard educational practices: teach us to do things terribly, and in a way that will never be done in a real job. — Mazzone, Jul 10 '16 at 20:06
@Mazzone OK, so go for separation of concerns: write your own dynamically sized data buffer class, make it safely copyable and assignable, and use it in your matrix class. — juanchopanza, Jul 10 '16 at 20:14
@Mazzone -- You're returning a `Matrix` by value in your `add` function and you failed to implement a user-defined copy constructor or assignment operator. You don't even have stubs of these functions, to indicate that you had knowledge of them.. You say you have to write things this way, but were you not told about these basic things, like the "rule of 3"? Why are you being told to do things a certain way, and you're not told all of the preliminaries required to accomplish the goal? — PaulMcKenzie, Jul 10 '16 at 20:37
@PaulMcKenzie I wish I could tell you why we're being forced to use built-in arrays instead of what the entire planet agrees is better for everything about this assignment: vectors. But I can't. Also can't tell you why it wasn't made clear that it's impossible to even return a Matrix like I'm trying to without it being made from a copy constructor. Such is modern-day, hyper-expensive education. — Mazzone, Jul 10 '16 at 20:42

score 1 · Accepted Answer · answered Jul 10 '16 at 20:11

The problem is lack of a manually defined copy constructor or assignment operator, given that the class manages a resource (memory).

If an instance of the class is assigned, or used to create a copy, the result will be two distinct objects that reference the same memory. When those two objects which refer to the same memory are destroyed, the memory is released twice. The result of that is undefined behaviour.

Look up the "rule of three" for a solution. In C++11, that often becomes a "rule of five" or a "rule of zero" (which involves using techniques to avoid the problem in the first place).

There is also a pretty significant problem in the add() function, since it dynamically creates a Matrix, then returns a copy of it. That causes a memory leak, even if the problem with copying the object is resolved. That function actually looks like something which would be written in a garbage collected language - the problem being that C++ is not garbage collected.

Segmentation fault but array object is not going out of bounds (C++)

1 Answers1