Free Memory of 2d Array in struct

Question

I'm writing a little game in C and I wanted to test it with Valgrind. Here is a little code example:

#include <stdio.h>
#include <stdlib.h>

typedef struct Game {
    int** field;
} Game;

void buildfield(Game* game, int length);
void printfield(Game* game, int length);
void freefield(Game* game, int length);

int main()
{
    struct Game* game = NULL;
    game = malloc(sizeof (struct Game));
    buildfield(game, 10);
    printfield(game, 10);
    freefield(game, 10);
    free(game);
    return 0;
}

void buildfield(Game* game, int length)
{
    game->field = (int**)malloc((sizeof (int*)) * 20);
    int i;
    for (i = 0; i < 20; i++) {
        game->field[i] = (int*) malloc((sizeof (int)) * length);
    }

    for (int line = 0; line < length; line++) {
        for (int col = 0; col < 81; col++) {
            game->field[col][line] = 0;
        }
    }
}

void printfield(Game* game, int length)
{
    for (int i = 0; i < length; i++) {
        printf("\n");
        for (int j = 0; j < 20; j++) {
            printf("%d",game->field[i][j]);
        }
    }
}

void freefield(Game* game, int length)
{
    for (int i = 0; i < length; ++i) {
        free(game->field[i]);
    }
    free(game->field);
}

Valgrind is saying:

HEAP SUMMARY:
==6239==     in use at exit: 968 bytes in 22 blocks
==6239==   total heap usage: 22 allocs, 0 frees, 968 bytes allocated

Why don't I free the allocations?

`for(i = 0; i < 20; i++)` you allocate 20 fields yet free only 10 with `freefield(game, 10);` — kaylum, Sep 27 '21 at 12:19
I think the bug is on purpose, but the question is why valgrind isn't reporting any frees at all. — Iguananaut, Sep 27 '21 at 12:22
`for(i = 0; i < 20; i++)` and `freefield(game, 10)`: instead of hard codes numbers use contants or `#define`s, e.g: `#define NBFIELDS 20` and `for(i = 0; i < NBFIELDS; i++)` and `freefield(game, NBFIELDS)` — Jabberwocky, Sep 27 '21 at 12:23
In `for (int col = 0; col < 81; col++)` - You well enough don't have 80 columns, so there's an out-of-bounds access, which is *Undefined Behavior*. — Ruks, Sep 27 '21 at 12:24
When I run your code I get a segmentation fault (for the part mentioned above by @Ruks) so of course `free()` never gets called because the program crashes before that point. — Iguananaut, Sep 27 '21 at 12:26
`game->field = (int**)malloc((sizeof (int*)) * 20);` -> ` game->field = (int**)malloc((sizeof (int*)) * length);` — 0___________, Sep 27 '21 at 12:27
All the problems in this code would go away if you stopped using "magic numbers" and instead used named variables/constants. — Lundin, Sep 27 '21 at 12:30
At the end of the Valgrind log, after the HEAP SUMMARY it tells you which argument you need to rerun with to see details of leaked memory. Did you try that? (Valgrind is slightly user unfriendly here, after rerunning it will tell you about a second argument that you should use). — Paul Floyd, Sep 27 '21 at 15:20

Francis · Answer 1 · 2021-09-28T08:35:04.943

Well in the function buildfield() You have alloted 20 blocks of memory

game->field = (int **) malloc(sizeof(int *) * 20);

Hence the most you can access by [] operator is game->field[19] But in the loop the program tries to access more blocks ahead of game->field[19] Causing a segmentation fault And the program crashes then and there. Without ever returning to main() let alone reach the free() statement. So it means your program never completed in the first place and crashed midway.

for (int line = 0; line < length; line++)
{
    for (int col = 0; col < 81; col++)  //when col becomes 20
    {
        game->field[col][line] = 0;     //this statement will try to access the non-alloted memory block when col is 20
    }
}

To check if your program crashes midway add some print statements at the end of the statements related to accessing memory or use a debugger. Because that's the most common source of runtime errors.

Also for this code try to keep in mind the maximum no. of memory blocks alloted that can be accessed by a specific pointer and change the condition of the marked for loop so that it will access memory within the allocation limit that is the no. of blocks you alloted using the malloc function.

The problem here is not the free() not working. But the segmentation fault. Assuming the code you provided is the exact replica of your code

*To check if your program crashes midway add some print statements* or use a **debugger**? — alex01011, Sep 27 '21 at 13:36

ryyker · Answer 2 · 2021-09-27T13:49:01.600

"Why don't I free the allocations?"

As noted in comments, there is extra complication in the code caused by use of unexplained values ( magic numbers ) to both create and free memory. For reasons explained in the link, among other problems, this can make matching count of frees with count of allocations difficult. The mis-match in times each of these is called in the reason Valgrind indicated blocks of memory remaining at the end of execution.

The following is your code with suggestions, including those specific to clearly applying the same number of calls to free() as to [m][c]alloc() (I chose to use calloc() here to avoid another loop (or memset()) to initialize memory.)

Note also, you may need to change the values that this example uses for #defines to meet your needs, but you only need to change them in one place, (at top of file.)

typedef struct Game {
    int** field;
} Game;

void buildfield(Game *game, int length);
void printfield(Game *game, int length);
void freefield(Game *game, int length);

#define COUNT  20//replace all magic numbers 
#define LENGTH 10//(change values of #defines to change shape and size of memory)

int main(void)//use a complete prototype for main
{
    struct Game* game = NULL;//pointer needs memory
    game = malloc(sizeof (struct Game));

    buildfield(game, LENGTH);
    printfield(game, LENGTH);
    freefield(game, LENGTH);
    free(game);
    return 0;
}

void buildfield(Game *game, int length)
{   //Note - not optimal to cast return of [c][m]alloc in C (only in C++)
    game->field = calloc(COUNT, sizeof(game->field));
    int i;
    for (i = 0; i < COUNT; i++) {
        game->field[i] = calloc(LENGTH, (sizeof (game->field[i])) );
    }
}

void printfield(Game *game,int length)
{
    for (int i = 0; i < COUNT; i++) {
        printf("\n");
        for (int j = 0; j < LENGTH; j++) {
            printf("%d",game->field[i][j]);
        }
    }
}

void freefield(Game *game,int length)
{
    for (int i = 0; i < COUNT; ++i) {
        free(game->field[i]);
    }
    free(game->field);
}

Free Memory of 2d Array in struct

2 Answers2