Properly freeing dynamically allocated memory

Volapiik (41)
I am having trouble freeing memory. There are many files so I have included only the main portions relevant to the issue. As far as I understand you free memory dynamically allocated to pointers by free(pointer). I have tried to use this concept in my teardown function, but it doesn't work.

defining a few structs. 
1
2
3
4
5
6
7
8
9
10
11
12
13
14
 
  typedef struct board {
    size_t width;
    size_t height;
    int* cells;  // array of integers containing data re each cell via bitmap
    snake_t* snake;
} board_t;

typedef struct game {
    int game_over;   // 1 if game is over, 0 otherwise
    int score;       // game score: 1 point for every food eaten
    board_t* board;  // pointer to the board struct for this game
} game_t;


In a separate c file I have a function to free memory
1
2
3
4
 
void teardown(game_t* game) {
    // TODO: implement!
    free(game->board->cells); // issue
}


In yet another separate c file I have a line to dynamically allocate memory
 
 
board->cells = calloc(column * row, sizeof(int));
Last edited on
mbozzi (3932)
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
 
#include <stdlib.h>
#include <stdint.h>

typedef struct snake snake_t;

typedef struct board {
    size_t width;
    size_t height;
    int* cells;  // array of integers containing data re each cell via bitmap
    snake_t* snake;
} board_t;

typedef struct game {
    int game_over;   // 1 if game is over, 0 otherwise
    int score;       // game score: 1 point for every food eaten
    board_t* board;  // pointer to the board struct for this game
} game_t;

void teardown(game_t* game) {
    // TODO: implement!
    free(game->board->cells); // issue
}

int main() 
{
  int column = 1000;
  int row = 4000;
  board_t b;
  board_t* board = &b;
  board->cells = calloc(column * row, sizeof(int));
  game_t game = { .board = board };
  teardown(&game);
}

Works for me
Volapiik (41)
Frankly I have no idea how to make a minimal reproducible. All I can do is link here
https://easyupload.io/t1q323

Edit: don't even bother downloading don't think the makefile is going to work without some more files
Last edited on
salem c (3700)
> Edit: don't even bother downloading don't think the makefile is going to work without some more files
wtf does that mean.

I downloaded.
1. You picked the worst possible archive format in .rar, with it's crappy licence terms and closed source.

2. Your makefile doesn't work.
1
2
3
 
$ cd src/
$ make
make: *** No rule to make target 'src/game.o', needed by 'snake'. Stop.


3. Your code has errors.
1
2
3
4
5
6
7
8
9
10
11
 
$ gcc *.c
game.c: In function ‘teardown’:
game.c:167:23: error: expected identifier before ‘*’ token
  167 |     free(game->board->*cells);
      |                       ^
game_over.c: In function ‘render_game_over’:
game_over.c:34:42: error: ‘game_t’ {aka ‘struct game’} has no member named ‘name_len’
   34 |     WRITEW(y_center - 2, x_center - (game->name_len / 2), game->name);
      |                                          ^~
game_over.c:34:63: error: ‘game_t’ {aka ‘struct game’} has no member named ‘name’
   34 |     WRITEW(y_center - 2, x_center - (game->name_len / 2), game->name);


Anyway, got it to link after looking in the makefile.
 
 
$ gcc -ggdb3 -Wall -Wextra -Wshadow -std=gnu11 -Wno-unused-parameter $(ncursesw5-config --cflags) *.c $(ncursesw5-config --libs) -lm


Then it barfs at startup with some cockamame range checking..

$ ./a.out
usage: snake <GROWS: 0|1> [BOARD STRING]
$ ./a.out 1 hello
Address is:  0x7ffdbd0fa5f0
             ____   
Hello       / . .\ 
CS 300      \  ---<
student!     \  /  
   __________/ /    
-=:___________/
Terminal window must be at least 0 by 818855170 characters in size! Yours is 211 by 53.


Even with this meagre start, there are memory problems.
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
 
$ valgrind ./a.out 1 hello
==7504== Memcheck, a memory error detector
==7504== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==7504== Using Valgrind-3.15.0 and LibVEX; rerun with -h for copyright info
==7504== Command: ./a.out 1 hello
==7504== 
Address is:  0x1ffefffd50
             ____   
Hello       / . .\ 
CS 300      \  ---<
student!     \  /  
   __________/ /    
-=:___________/
==7504== Conditional jump or move depends on uninitialised value(s)
==7504==    at 0x4A95AD8: __vfprintf_internal (vfprintf-internal.c:1687)
==7504==    by 0x4A7FEBE: printf (printf.c:33)
==7504==    by 0x10A6DD: check_terminal_size (render.c:38)
==7504==    by 0x10A73A: initialize_window (render.c:67)
==7504==    by 0x10AC7E: main (snake.c:126)
==7504== 
==7504== Use of uninitialised value of size 8
==7504==    at 0x4A7981B: _itoa_word (_itoa.c:179)
==7504==    by 0x4A956F4: __vfprintf_internal (vfprintf-internal.c:1687)
==7504==    by 0x4A7FEBE: printf (printf.c:33)
==7504==    by 0x10A6DD: check_terminal_size (render.c:38)
==7504==    by 0x10A73A: initialize_window (render.c:67)
==7504==    by 0x10AC7E: main (snake.c:126)
==7504== 
==7504== Conditional jump or move depends on uninitialised value(s)
==7504==    at 0x4A7982D: _itoa_word (_itoa.c:179)
==7504==    by 0x4A956F4: __vfprintf_internal (vfprintf-internal.c:1687)
==7504==    by 0x4A7FEBE: printf (printf.c:33)
==7504==    by 0x10A6DD: check_terminal_size (render.c:38)
==7504==    by 0x10A73A: initialize_window (render.c:67)
==7504==    by 0x10AC7E: main (snake.c:126)
==7504== 
==7504== Conditional jump or move depends on uninitialised value(s)
==7504==    at 0x4A963A8: __vfprintf_internal (vfprintf-internal.c:1687)
==7504==    by 0x4A7FEBE: printf (printf.c:33)
==7504==    by 0x10A6DD: check_terminal_size (render.c:38)
==7504==    by 0x10A73A: initialize_window (render.c:67)
==7504==    by 0x10AC7E: main (snake.c:126)
==7504== 
==7504== Conditional jump or move depends on uninitialised value(s)
==7504==    at 0x4A9586E: __vfprintf_internal (vfprintf-internal.c:1687)
==7504==    by 0x4A7FEBE: printf (printf.c:33)
==7504==    by 0x10A6DD: check_terminal_size (render.c:38)
==7504==    by 0x10A73A: initialize_window (render.c:67)
==7504==    by 0x10AC7E: main (snake.c:126)
==7504== 
Terminal window must be at least 79740872 by 1092850 characters in size! Yours is 211 by 53.
==7504== 
==7504== HEAP SUMMARY:
==7504==     in use at exit: 977,871 bytes in 418 blocks
==7504==   total heap usage: 457 allocs, 39 frees, 991,901 bytes allocated
==7504== 
==7504== LEAK SUMMARY:
==7504==    definitely lost: 0 bytes in 0 blocks
==7504==    indirectly lost: 0 bytes in 0 blocks
==7504==      possibly lost: 0 bytes in 0 blocks
==7504==    still reachable: 977,871 bytes in 418 blocks
==7504==         suppressed: 0 bytes in 0 blocks
==7504== Rerun with --leak-check=full to see details of leaked memory
==7504== 
==7504== Use --track-origins=yes to see where uninitialised values come from
==7504== For lists of detected and suppressed errors, rerun with: -s
==7504== ERROR SUMMARY: 37 errors from 6 contexts (suppressed: 0 from 0)

I guess all those "Conditional jump or move depends on uninitialised value" that lead to your check_terminal_size function are telling you something about why it fails at the first step.

That the program reports different x/y values for the minimum terminal size in different run-time environments is a big clue.
Volapiik (41)
try ./a.out 0 after changing free(game->board->*cells); to free(game->board->cells);
in game.c the game should popup.

image
https://paste.pics/549eb1037e3748097aeabe8770466c9a

The 1 option is handled by running make check

My issue is after running make check-15-27 which I think requires another file autograder.py. Let me upload another version https://easyupload.io/qxhvq5
sorry rar file again haha


make check results

https://paste.pics/2c08243e5ef3b59581c8c44bfe65f0b3

about the terminal window size its meant to run in a specific container so can't do much about that.

logically though free(game->board->cells) should 100 % work right?

Last edited on
salem c (3700)
I see two problems.
1
2
3
4
 
    //custom board
    for (int i = 0; i < row; i++)  //  ++i
    {
        for (int j = 0; j < column; i++)

Your inner loop increments i (again), not j.

1
2
 
    return INIT_UNIMPLEMENTED;
    return INIT_UNIMPLEMENTED;

Maybe return success?


Are you meant to change autograder.c yourself?
1
2
3
4
5
6
7
8
9
10
11
 
    // Run the snake game
    game_t final_game_state = { 0 };
    board_t board = { 0 };
    final_game_state.board = &board;

    // default the board to 0x0 so the stencil doesn't crash
    board.width = 0;
    board.height = 0;

    snake_t snake = { 0 };
    final_game_state.board->snake = &snake;

Because by correctly initialising these elements, the problem is resolved.

Which suggests there are paths through your code which don't fully initialise all members of your structures.


Volapiik (41)
We are not meant to change autograder

Look at mbozzi's code for a second I added two lines 

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
 
#include <stdlib.h>
#include <stdint.h>
#include <stdio.h>

typedef struct snake snake_t;

typedef struct board {
    size_t width;
    size_t height;
    int* cells;  // array of integers containing data re each cell via bitmap
    snake_t* snake;
} board_t;

typedef struct game {
    int game_over;   // 1 if game is over, 0 otherwise
    int score;       // game score: 1 point for every food eaten
    board_t* board;  // pointer to the board struct for this game
} game_t;

void teardown(game_t* game) {
    // TODO: implement!
    int* p = (game->board->cells);
    free(game->board->cells); // issue
    printf("Address is: %p\n", game->board->cells);
}

int main() 
{
  int column = 1000;
  int row = 4000;
  board_t b;
  board_t* board = &b;
  board->cells = calloc(column * row, sizeof(int));
  game_t game = { .board = board };
  teardown(&game);
  //int* jim = calloc(column * row, sizeof(int));
  //printf("Address is: %p\n", jim);
}


The two lines at the bottom of main print the address of dynamic memory which has a unique address to the heap.

On the other hand in teardown printing the address of (game->board->cells) is printing an address commonly found for local variables aka on the stack and not on the heap.

Unless my theory is wrong, game->board->cells shouldn't be the correct way of accessing the address to the dynamic memory.
Last edited on
seeplus (6591)
Well considering:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
 
void teardown(game_t* game) {
	// TODO: implement!
	int* p = (game->board->cells);
	printf("Address is: %p\n", game->board->cells);
	free(game->board->cells); // issue
}

int main() {
	int column = 1000;
	int row = 4000;
	board_t b;
	board_t* board = &b;

	board->cells = (int*)calloc(column * row, sizeof(int));
	printf("cells %p\n", board->cells);

	game_t game = {.board = board};

	teardown(&game);

	int* jim = (int*)calloc(column * row, sizeof(int));
	printf("jim is: %p\n", jim);
	free(jim);
}


when I run this, I get displayed:


cells 0000000000500040
Address is: 0000000000500040
jim is: 0000000000500040

Volapiik (41)
interesting what I get is 
1
2
3
 
cells 0x7fa360f2b010       
Address is: 0x7fa360f2b010
jim is: 0x5559aea186b0
Last edited on
Volapiik (41)
I am fairly sure game->board->cells is simply giving free the local address of the pointer cells instead of the actual dynamic address inside cells. Not sure how to access this value though
Last edited on
salem c (3700)
Your code to free things is right.

What is broken is your initialisation of that structure in some cases.

There is a path through your code that leaves board->cells uninitialised and pointing at garbage.
Which is a problem when you try to free it.

Writing your free function in a multitude of different ways won't fix it.

The reason board_t board = { 0 }; works is because it makes the cells pointer specifically NULL, and free(NULL); is a safe thing to do.
salem c (3700)
snake.c
1
2
 
// ------------- DO NOT MODIFY ANYTHING ABOVE THIS LINE ------------------
// Check validity of the board before rendering!

So above this line, there are multiple train wrecks.

> initialize_game(&game, argv[2]);
Routinely ignores the return result, because it's declared like this.

> board_init_status_t initialize_game(game_t* game, char* board_rep);

1
2
3
4
5
6
7
8
9
10
11
12
13
14
 
board_init_status_t initialize_game(game_t* game, char* board_rep) {
    // TODO: implement!
    game->game_over = 0;
    game->score = 0;
    if (board_rep == NULL)
    {
        initialize_default_board(game->board);
    }
    else
    {
        decompress_board_str(game->board, board_rep);
    }
    return INIT_SUCCESS;
}

Both these functions return a result (which you ignore).
decompress_board_str can return a failure, but you just claim success anyway.
Not that it matters, the caller doesn't pay any attention to it either.
In the midst of returning an error, it leaves the cells pointer as uninitialised garbage.

So much for "Check validity".

The best you can hope to achieve is by doing at least this at the start of initialize_game.
game->board->cells = NULL;
kbw (9488)
Did you fix it?

I commented out function: render_game_over, it was too broken to fix.

Using @salem c's build instructions, I compiled it with:
1
2
 
for f in *.c; do cc -g $(ncursesw5-config --cflags) -c $f; done
cc -g -o game *.o $(ncursesw5-config --libs)


Run it with:
 
 
valgrind --leak-check=full ./game 1


Get output:
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
 
==290602== Memcheck, a memory error detector
==290602== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==290602== Using Valgrind-3.16.1 and LibVEX; rerun with -h for copyright info
==290602== Command: ./game 1
==290602== 
Address is:  0x1ffefff9e0
             ____   
Hello       / . .\ 
CS 300      \  ---<
student!     \  /  
   __________/ /    
-=:___________/
==290602== 
==290602== HEAP SUMMARY:
==290602==     in use at exit: 377,686 bytes in 394 blocks
==290602==   total heap usage: 438 allocs, 44 frees, 392,983 bytes allocated
==290602== 
==290602== LEAK SUMMARY:
==290602==    definitely lost: 0 bytes in 0 blocks
==290602==    indirectly lost: 0 bytes in 0 blocks
==290602==      possibly lost: 0 bytes in 0 blocks
==290602==    still reachable: 377,686 bytes in 394 blocks
==290602==         suppressed: 0 bytes in 0 blocks
==290602== Reachable blocks (those to which a pointer was found) are not shown.
==290602== To see them, rerun with: --leak-check=full --show-leak-kinds=all
==290602== 
==290602== For lists of detected and suppressed errors, rerun with: -s
==290602== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)


Did I miss something?
Last edited on
salem c (3700)
> Did I miss something?
You need to download the latest rar file from one of the OP's replies, as this also contains the autograder.c and a Python script.

The autograder passes in half a dozen command line parameters, and it's barfing on one of those cases.
Topic archived. No new replies allowed.