Issue with function memory allocation

Question

After I compile and run the function, I get a segmentation fault: 11. I believe malloc should be performed correctly so I am not sure why I get a seg fault. Any insights would be greatly appreciated!

typedef struct matrix matrix;

struct matrix {
   unsigned int n_rows;
   unsigned int n_cols;
   float **entries;
};

//refer to matrix.h
matrix *matrix_zero(unsigned int n_rows, unsigned int n_cols){
  struct matrix* new = (struct matrix*)malloc(sizeof(struct matrix));
  new->entries = malloc(n_rows * n_cols * sizeof(float));
  new->n_rows=n_rows;
  new->n_cols=n_cols;
  for(int x = 0; x < n_rows; x++){
    for(int y = 0; y < n_cols; y++){
      new->entries[x][y] = 0;
    }
  }
  return new;
}

/* main: run the evidence functions above */
 int main(int argc, char *argv[])
 {
   struct matrix* test1 = matrix_zero(3,3);
   // matrix_show(test1);
 }

Do not cast the result of malloc: https://stackoverflow.com/q/605845/6872717 — alx - recommends codidact, May 13 '19 at 17:26
Why is entries a pointer to a pointer to float? Why ot a pointer to float? Why do you want to allocate that matrix-thingy dynamically ... 2 ints and a pointer?? — Swordfish, May 13 '19 at 17:28
With `new->entries[x][y]` the compiler has no way of knowing the row length. This can only work if you first allocate an array of pointers to each row, and then the memory for the columns, for each row. — Weather Vane, May 13 '19 at 17:28
A pointer to pointer is not an array of array. You should enable compiler warnings, your compiler could have probably told you what is wrong. — EOF, May 13 '19 at 17:30
@CacahueteFrito and it is better to remember the `*` there for the `sizeof` trick when advising others ;) — Antti Haapala -- Слава Україні, May 13 '19 at 19:04
And it's better to use `sizeof(*new->entries)` instead of `sizeof(float)` so that if the type changes in the future, for example to double, your code doesn't break (again). Thanks to @AnttiHaapala for correcting me. — alx - recommends codidact, May 13 '19 at 20:38
`n_rows * n_cols * sizeof(float)` should be rewritten as `sizeof(float) * n_rows * n_cols` to avoid overflow — alx - recommends codidact, May 13 '19 at 21:05

score 4 · Answer 1 · answered May 13 '19 at 17:38

The problem appears to be in your allocation for matrix->entries. The struct defines a pointer to a pointer, but you allocate a float pointer float* vs float**. You need to allocate n_rows number of float* and each of those needs to point to an allocations of n_cols number of float value. For example:

int i;
// No error checking shown here
new->entries = malloc(sizeof(float*) * n_rows);
for (i = 0; i < n_rows; i++) {
    new->entries[i] = malloc(sizeof(float) * n_cols);
}

score 0 · Answer 2 · answered May 17 '19 at 04:44

You have allocated an array for rows by cols size, but there's no way for the compiler to know the actual row size for each row, so the notation entries[i] indeed expects a pointer to a simple array of floats, and not a bidimensional array. This is one of the main differences in structure from a n-dimensional array and arrays of pointers in C. The compiler knows how to dimension it only when you fully qualify the array dimensions (as when you declare it as float entries[N][M]; --- look that you cannot use variable expressions in the dimensions, only static compile-time constants)

You have two approaches here:

Use a single dimension array an compute the index based on the rows sizes:

typedef struct matrix matrix;

struct matrix {
    unsigned int n_rows;
    unsigned int n_cols;
    float *entries;  /* we make this to point to a single array of n_rows * n_cols entries */
};


new->entries = malloc(n_rows * n_cols * sizeof(float));
new->n_rows=n_rows;
new->n_cols=n_cols;
for(int x = 0; x < n_rows; x++){
    for(int y = 0; y < n_cols; y++){
        new->entries[x * n_cols + y] = 0.0; /* entry position should be as shown */
}

Use individual row arrays of n_cols entries (this has been shown in another answer by @joel already)

typedef struct matrix matrix;

struct matrix {
    unsigned int n_rows;
    unsigned int n_cols;
    float **entries;  /* this time the entries field points to an array of pointers to float. */
};

new->entries = malloc(n_rows * sizeof *new->entries);  /* individual cells are 'float *', not 'float' this time. */
new->n_rows=n_rows;
new->n_cols=n_cols;
for(int x = 0; x < n_rows; x++){
    new->entries[x] = malloc(n_cols* sizeof **new->entries); /* this time float *'s are here */
    for(int y = 0; y < n_cols; y++){
        new->entries[x][y] = 0; /* entry position should be as shown */
    }
}

Both methods have remarks:

First method requires only one malloc(3) for the entries array so this makes it easier to allocate and deallocate, but some implementations could limit the actual size of a single malloc(3) in case you want to allocate huge matrices. This makes the deallocation for the whole matrix also easier.
Second method only requires a malloc of n_rows pointers and n_rows mallocs of n_cols floats. This will make possible to allocate huge matrices (you never allocate the whole matrix in one chunk) but you'll have to deallocate all rows first, then the array of pointers to the rows, before deallocating the matrix struct.
I recommend you to use malloc(n_cols * sizeof *new->entries) instead of malloc(n_cols * sizeof (float *)), so you don't need to change this expression in case you change the definition type of new->entries.

Finally, think that there's no magic in the C language in respect of calling functions. You probably erroneously assumed that making malloc( n_rows * n_cols * sizeof(float) ) converted automatically the pointer to a bidimensional array, but there's no magic there, malloc(3) is a normal C function like the ones you can write, and that's the reason it requires the actual number of bytes, and not the dimensions (in elements) of the array.

Issue with function memory allocation

2 Answers2