How to malloc/free a struct with a contained 2D array

Question

So my goal is to malloc a maze struct that contains a 2D array; however, when I try to allocate memory for each "cell" of the 2D array I can't seem to free it properly afterwards. Is there a way that I could malloc the struct in one line or at least in a way that I can easily free the allocated memory with the free_maze function? I've attached my .c file along with the header file that defines the struct. Additionally, I have attached an example of a maze that is contained within a text file.

#include <stdlib.h>
#include "maze.h"


Maze* malloc_maze(int num_rows, int num_cols){
  Maze* maze = malloc(sizeof(*maze));
  if (maze == NULL){
    free(maze);
    return NULL;
  }
  maze -> cells = malloc(sizeof(maze -> cells)*(num_cols));

  if (maze -> cells == NULL){
    free(maze);
    return NULL;
  }
  for(int i = 0; i < num_cols; i++){
    maze -> cells[i] = malloc(sizeof(*(maze -> cells))*(num_rows));
  }
  maze -> num_rows = num_rows;
  maze -> num_cols = num_cols;
  return maze;
}

void free_maze(Maze* maze){
  free(maze);
}

Maze* read_maze(FILE* fp){
  Maze* maze;
  char c = fgetc(fp);
  int rows = 0;
  int cols = 0;
  int chars = 0;
  while(c != EOF){
    chars++;
    c = fgetc(fp);
  }
  rewind(fp);
  while(c != '\n'){
    cols++;
    c = fgetc(fp);
  }
  rows = chars / cols;
  cols--;
  maze = malloc_maze(rows, cols);
  rewind(fp);
  for(int row_count =0; row_count <= rows; row_count++){
    for(int col_count = 0; col_count < cols; col_count++){
      fseek(fp, (row_count*(cols+1)+col_count), SEEK_SET);
      maze -> cells[col_count][row_count] = fgetc(fp);
    }
  }
  maze -> num_rows = rows;
  maze -> num_cols = cols;
  return maze;
}

bool write_maze(const char* filename, const Maze* maze){
  FILE* ha;
  ha = fopen(filename, "w");
  if(ha == NULL){
    return false;
  }
  rewind(ha);
  int rows = maze -> num_rows;
  int cols = maze -> num_cols;
  for(int i = 0; i < rows; i++){
    for(int j = 0; j < cols; j++){
    fputc(maze -> cells[j][i], ha);
    }
    fputc('\n', ha);
  }
  fclose(ha);
  return true;
}

/////////////////header file//////////////////////////

#ifndef MAZE_H
#define MAZE_H 

#define WALL 'X'
#define PATH ' '

#include <stdio.h>
#include <stdbool.h>

typedef struct _Maze {
   int num_rows; 
   int num_cols; 
   char** cells; 
} Maze;

Maze* malloc_maze(int num_rows, int num_cols);

void free_maze(Maze* maze){
        __attribute__((nonnull));
}

Maze* read_maze(FILE* fp){
        __attribute__((nonnull));
}

bool write_maze(const char* filename, const Maze* maze){
        __attribute__((nonnull));
}

///////////////example maze within .txt file/////////////////////


XXXXX XXX
X       X
X XXX XXX
X X X   X
X X XXXXX
X       X
XXXXX XXX

Answer 1

Given an allocator function, the deallocator writes itself - you free the pointers in roughly the reverse order they were allocated.

So, given that the allocator is (only reformatted from the question — functionality unchanged):

Maze *malloc_maze(int num_rows, int num_cols)
{
    Maze *maze = malloc(sizeof(*maze));
    if (maze == NULL)
    {
        free(maze);
        return NULL;
    }
    maze->cells = malloc(sizeof(maze->cells) * (num_cols));

    if (maze->cells == NULL)
    {
        free(maze);
        return NULL;
    }
    for (int i = 0; i < num_cols; i++)
    {
        maze->cells[i] = malloc(sizeof(*(maze->cells)) * (num_rows));
    }
    maze->num_rows = num_rows;
    maze->num_cols = num_cols;
    return maze;
}

the deallocator should be:

void free_maze(Maze *maze)
{
    for (int i = 0; i < num_cols; i++)
        free(maze->cells[i]);
    free(maze->cells);
    free(maze);
}

This makes sure the code doesn't try to access memory after it is freed.

---

However, closer analysis of the allocator shows that there are some (minor) problems. For example, normally you treat the pair of indexes as maze->cells[row][col], but the memory allocation requires it to be used as maze->cells[col][row]. Both can work, but the row-column order is more usual in C. Also, the sizes in the second and third malloc() calls are incorrect. Fortunately for you, the second one allocates in units of sizeof(char **) instead of sizeof(char *), but those are the same size, so it "doesn't matter". The third one allocates sizeof(char *) units, instead of sizeof(char), so there is much more memory allocated than memory (normally, sizeof(char *) is 4 or 8 bytes but sizeof(char) is 1 by definition).

So, you might do better to use this, which keeps the maze->cells[col][row] access notation:

Maze *malloc_maze(int num_rows, int num_cols)
{
    Maze *maze = malloc(sizeof(*maze));
    if (maze == NULL)
        return NULL;
    maze->cells = malloc(sizeof(maze->cells[0]) * num_cols);
    if (maze->cells == NULL)
    {
        free(maze);
        return NULL;
    }
    for (int i = 0; i < num_cols; i++)
    {
        maze->cells[i] = malloc(sizeof(maze->cells[0][0]) * num_rows);
        if (maze->cells[i] == 0)
        {
            for (int j = 0; j < i; j++)
                 free(maze->cells[j]);
            free(maze->cells);
            free(maze);
            return NULL;
        }
    }
    maze->num_rows = num_rows;
    maze->num_cols = num_cols;
    return maze;
}

This cleans up the partially allocated memory on allocation failure. It doesn't change the deallocation code (unless you want to add a null check, but if the allocation failed, you shouldn't be calling the deallocation code).

Answer 2

NOTE: I believe the code automatically takes care of alignment in OP's case. I am still working on to tweak it to work with any data type of members of Maze. The alignment is broken if sizeof(char*) exceeds sizeof(T) where T is the type of cell[0][0].

You can allocate memory for the entire maze in one malloc call. This will allow you to free the memory in one free call. It improves performance because:

• it requires a single malloc (and free) call
• the memory allocated is contiguous (cache friendly)

Maze *malloc_maze(int num_rows, int num_cols)
{
    const size_t mem_size = sizeof(Maze)
                        + num_cols*sizeof(((Maze*)0)->cells[0]) /* indirection array */
                        + sizeof((((Maze*)0)->cells[0][0]))*num_cols*num_rows; /* matrix */

    void *block = malloc(mem_size);
    if(block == NULL)
        return NULL;

    Maze *maze = block;
    maze->cells = (void*)(maze + 1);
    block = &maze->cells[0] + num_cols;
    for(int i = 0; i < num_cols; i++)
    {
        maze->cells[i] = block; 
        block = &maze->cells[i][0] + num_rows;
    }
    
    maze->num_rows = num_rows;
    maze->num_cols = num_cols;
    return maze;
}
void free_maze(Maze *maze)
{
    free(maze);   
}