Minor memory leak in tetris.c
0xjmux opened this issue · 3 comments
0xjmux commented
I'm testing out using issues to track bugs as I fix them. This is another one of those
Running test_tetris through valgrind, the result is:
$ valgrind --track-origins=yes ./build/test_tetris
==341505== Memcheck, a memory error detector
==341505== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==341505== Using Valgrind-3.18.1 and LibVEX; rerun with -h for copyright info
==341505== Command: ./build/test_tetris
==341505==
==== RUNNING UNIT TESTS ON TETRIS =====
[path]/tetrisgame/test/suite_1.c:476:test_checkValidMove:PASS
[path]/tetrisgame/test/suite_1.c:477:test_T_testPieceRotate:PASS
[path]/tetrisgame/test/suite_1.c:478:test_clearRows:PASS
[path]/tetrisgame/test/suite_1.c:479:test_checkSpawnNewPiece:PASS
[path]/tetrisgame/test/suite_1.c:482:test_arr_helpers:PASS
Config loaded from ./test/files/gamestate-full-row-not-detected.ini!
==341505== Conditional jump or move depends on uninitialised value(s)
==341505== at 0x10DEA2: val_in_arr (tetris.c:646)
==341505== by 0x10DBDF: check_and_clear_rows (tetris.c:527)
==341505== by 0x10A9E6: test_clearRowsDumpedGame_1 (suite_1.c:269)
==341505== by 0x110F71: UnityDefaultTestRun (unity.c:2202)
==341505== by 0x10B5E2: main (suite_1.c:483)
==341505== Uninitialised value was created by a stack allocation
==341505== at 0x10DB2F: check_and_clear_rows (tetris.c:502)
==341505==
[path]/tetrisgame/test/suite_1.c:483:test_clearRowsDumpedGame_1:PASS
Config loaded from ./test/files/gamestate-J-lined-up-dbl-clear.ini!
==341505== Conditional jump or move depends on uninitialised value(s)
==341505== at 0x10DEA2: val_in_arr (tetris.c:646)
==341505== by 0x10DBDF: check_and_clear_rows (tetris.c:527)
==341505== by 0x10DDFC: check_and_spawn_new_piece (tetris.c:609)
==341505== by 0x10AD27: test_clearRowsDumpedGame_2 (suite_1.c:325)
==341505== by 0x110F71: UnityDefaultTestRun (unity.c:2202)
==341505== by 0x10B600: main (suite_1.c:484)
==341505== Uninitialised value was created by a stack allocation
==341505== at 0x10DB2F: check_and_clear_rows (tetris.c:502)
==341505==
[path]/tetrisgame/test/suite_1.c:484:test_clearRowsDumpedGame_2:PASS
-----------------------
7 Tests 0 Failures 0 Ignored
OK
==341505==
==341505== HEAP SUMMARY:
==341505== in use at exit: 36 bytes in 2 blocks
==341505== total heap usage: 14 allocs, 12 frees, 17,756 bytes allocated
==341505==
==341505== LEAK SUMMARY:
==341505== definitely lost: 36 bytes in 2 blocks
==341505== indirectly lost: 0 bytes in 0 blocks
==341505== possibly lost: 0 bytes in 0 blocks
==341505== still reachable: 0 bytes in 0 blocks
==341505== suppressed: 0 bytes in 0 blocks
==341505== Rerun with --leak-check=full to see details of leaked memory
==341505==
==341505== For lists of detected and suppressed errors, rerun with: -s
==341505== ERROR SUMMARY: 3 errors from 2 contexts (suppressed: 0 from 0)
0xjmux commented
Leak was a result of a call to strdup() in utils.c
} else if (MATCH_KEY("last_gravity_tick_usec")) {
// some manual manipulation needed here since both vals on one line
char *timeval_str = strdup(value);
tg->last_gravity_tick_usec.tv_sec = atoi(strtok(timeval_str,","));
tg->last_gravity_tick_usec.tv_usec = atoi(strtok(NULL, ","));Since strdup() allocates memory, it was missing a free(timeval_str);`.
This leak wasn't actually in tetris.c, and wouldn't ever be hit in an embedded implementation.
0xjmux commented
The "unitialized value was created by a stack allocation" error came from line utils.c:527 :
if (!val_in_arr(row_with_offset, rows_to_clear, rows_idx + 1)) {
rows_to_clear[rows_idx] = row_with_offset;
rows_idx += 1;
}rows_idx does double duty as the index of the next item to be added to the list and the count of the number of items in the list, but the rows_idx + 1 means that it's actually making val_in_arr() check one element past the number of element that are already in the array; hence the notice about unitialized values.
0xjmux commented
After both of those are fixed, we have:
-----------------------
7 Tests 0 Failures 0 Ignored
OK
==344074==
==344074== HEAP SUMMARY:
==344074== in use at exit: 0 bytes in 0 blocks
==344074== total heap usage: 14 allocs, 14 frees, 17,756 bytes allocated
==344074==
==344074== All heap blocks were freed -- no leaks are possible
==344074==
==344074== For lists of detected and suppressed errors, rerun with: -s
==344074== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0
Next commit will close this issue.