Check memory with the suites that already exist
`cmake --build build --target memcheck` runs every registered CTest suite under valgrind. There are no new test programs, and there should not be any: tests/benchutil.h notices valgrind in LD_PRELOAD and drops the benchmark scale to 0.0005, which turns the perf suites into the broadest path coverage in the tree at a cost valgrind can survive. A benchmark walks one path a hundred thousand times; a leak check wants every path walked once. Same binaries, one flag apart, and the whole run is about thirty seconds. scripts/memcheck.sh wraps `ctest -T memcheck` because that command records defects and still exits 0, which cannot gate anything. It also forces the headless drivers, so the vendor GPU stack is never loaded -- that removes thousands of unfixable findings inside amdgpu_dri.so without suppressing anything, and it is what the suites are written for anyway. Only definite losses, invalid accesses and uninitialised reads count; "still reachable" is what SDL and FreeType keep for the process lifetime and says nothing about this library. The three suppressions in scripts/valgrind.supp are each a decision that a finding belongs to somebody else. Six defects, all filed in TODO.md under "Memory checking", the first of which is the one that matters: not one of the four json_load_file calls in src/ is ever matched by a json_decref, so every asset load abandons its parsed document -- 1.5 KB per sprite, 2.1 KB per character, 9 KB per load of the 2x2 fixture map, and on the order of a megabyte for a real one. A game that reloads a level on death leaks a level's worth of JSON every time. akgl_get_property also reads up to 4 KiB past the end of every property value it copies, and the savegame name tables read past the end of every registry key and write what they find into the file. tests/util.c zeroes three fixtures it used to leave as stack garbage. The library was never at fault there -- the tests handed it uninitialised floats and then made one real call with them -- but sixteen findings of noise in a new gate is how a gate gets ignored. The unit suites' TIMEOUT goes from 30 to 300 because CTest applies the same property to the checked run, where everything is twenty times slower. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
143
TODO.md
143
TODO.md
@@ -1101,6 +1101,149 @@ Notes on the ones worth arguing about:
|
||||
layers, several tilesets — would make target 13 measurable and would probably
|
||||
find something.
|
||||
|
||||
## Memory checking
|
||||
|
||||
`cmake --build build --target memcheck` runs **the suites that already exist**
|
||||
under valgrind — `ctest -T memcheck` with the headless drivers forced, wrapped by
|
||||
`scripts/memcheck.sh` so that a finding is an exit status rather than a line in a
|
||||
log nobody reads. There are no memory-check test programs, and there should never
|
||||
be any: `tests/benchutil.h` notices that it is running under valgrind and divides
|
||||
every benchmark's iteration count by two thousand, which turns the perf suites
|
||||
into the broadest path coverage in the tree at a cost valgrind can survive. The
|
||||
whole run is about thirty seconds.
|
||||
|
||||
The two halves fit together on purpose. A benchmark is a program that walks one
|
||||
path a hundred thousand times; a leak check wants every path walked once. Same
|
||||
binaries, same registration, one flag apart.
|
||||
|
||||
Third-party findings are suppressed in `scripts/valgrind.supp`, and the run
|
||||
forces `SDL_VIDEO_DRIVER=dummy` / `SDL_RENDER_DRIVER=software` /
|
||||
`SDL_AUDIO_DRIVER=dummy` so the vendor GPU stack is never loaded — that removes
|
||||
thousands of unfixable findings inside `amdgpu_dri.so` without suppressing
|
||||
anything at all. Only *definite* losses and invalid accesses are counted; "still
|
||||
reachable" is what SDL and FreeType keep for the process lifetime and says
|
||||
nothing about this library.
|
||||
|
||||
### Defects the memory checker found
|
||||
|
||||
Ordered by blast radius. Numbering continues the lists above. Every size below
|
||||
is measured, not estimated.
|
||||
|
||||
34. **Every JSON loader leaks its parsed document.** There are four
|
||||
`json_load_file` calls in `src/` and not one `json_decref` anywhere in the
|
||||
library, so the whole parsed tree — objects, hashtables, strings — is
|
||||
abandoned on both the success and failure paths:
|
||||
|
||||
| Loader | Site | Leaked per call |
|
||||
|---|---|---:|
|
||||
| `akgl_sprite_load_json` | `src/sprite.c:140` | ~1,500 bytes |
|
||||
| `akgl_character_load_json` | `src/character.c:232` | ~2,150 bytes |
|
||||
| `akgl_tilemap_load` | `src/tilemap.c:693` | ~9,000 bytes (2x2 fixture map) |
|
||||
| `akgl_registry_load_properties` | `src/registry.c:134` | not exercised by any test |
|
||||
|
||||
Blast radius: every asset load, forever. The map figure is for the 2x2
|
||||
fixture; a real map's JSON is the size of its layer data, so a 128x128 map
|
||||
leaks on the order of a megabyte per load. A game that reloads a level on
|
||||
death leaks a level's worth of JSON each time, and this is the one item in
|
||||
this list that grows without bound.
|
||||
|
||||
Fix: `json_decref(json)` in the `CLEANUP` block of each, which is where the
|
||||
other resources are already released. It is four lines. What makes it worth
|
||||
its own commit rather than a footnote is that each one needs a test proving
|
||||
the document is released, and `akgl_registry_load_properties` has no test at
|
||||
all yet.
|
||||
|
||||
35. **`akgl_get_property` reads up to 4 KiB past the end of the property
|
||||
value.** `src/registry.c:181` copies a fixed `AKGL_MAX_STRING_LENGTH` bytes
|
||||
out of whatever `SDL_GetStringProperty` returns, and what it returns is an
|
||||
`SDL_strdup` of the value — four bytes for `"0.0"`. Valgrind reports an
|
||||
invalid read on **every call**, twelve of them in `tests/physics.c` alone,
|
||||
because `akgl_physics_init_arcade` reads six properties and
|
||||
`akgl_render_init2d` reads two more.
|
||||
|
||||
This has been in `registry.h` as a `@note` about the copy being "a fixed
|
||||
#AKGL_MAX_STRING_LENGTH bytes rather than the length" — filed as waste. It
|
||||
is not waste, it is an out-of-bounds read: today it walks into the rest of
|
||||
SDL's heap and returns garbage past the terminator, and on a value that
|
||||
lands at the end of a page it is a segfault in a getter.
|
||||
|
||||
Fix: bound the copy by `strlen` of the source, still capped at
|
||||
`AKGL_MAX_STRING_LENGTH`. Touches `akgl_get_property` only, and the header
|
||||
note becomes a description of correct behaviour instead of a confession.
|
||||
Worth a test that stores a short property, reads it back, and asserts the
|
||||
bytes after the terminator in the destination are untouched.
|
||||
|
||||
36. **The savegame name tables read past the end of every registry key, and
|
||||
write what they find into the file.** `akgl_game_save_actorname_iterator`
|
||||
(`src/game.c:219`) writes `AKGL_ACTOR_MAX_NAME_LENGTH` — 128 — bytes
|
||||
starting at the key SDL handed it, and SDL allocated that key to fit the
|
||||
name. Valgrind catches it on a 40-byte allocation. The three sibling
|
||||
iterators do the same thing at `src/game.c:248`, `src/game.c:280` and
|
||||
`src/game.c:308`, with 128, 512 and 128 byte fixed widths; only the actor
|
||||
one is reached by the current tests, because the other registries are empty
|
||||
in the save roundtrip.
|
||||
|
||||
Two consequences, and the second is the interesting one. The read can fault
|
||||
if the key sits at the end of a page. And whatever it reads goes into the
|
||||
save file, so a savegame contains up to 500 bytes of this process's heap per
|
||||
registered object — anything that happened to be next to the key. That is a
|
||||
file a player might send someone.
|
||||
|
||||
Fix: copy the name into a zeroed fixed-width buffer and write that. It
|
||||
pairs naturally with **Defects -> Known and still open** item 7, which is
|
||||
the same tables disagreeing about their widths between writer and reader.
|
||||
|
||||
37. **`akgl_controller_list_keyboards` leaks the array SDL gives it.**
|
||||
`src/controller.c:188` calls `SDL_GetKeyboards`, which allocates, and never
|
||||
calls `SDL_free` on the result. Four bytes per call in the test environment
|
||||
— one keyboard id — but it is per call, and the function is shaped like
|
||||
something a game calls when a device is hotplugged.
|
||||
|
||||
Fix: one `SDL_free`, in a `CLEANUP` block so the early-return path is
|
||||
covered too. The same question should be asked of every SDL enumeration in
|
||||
`src/controller.c`; `SDL_GetGamepads` has the same contract and the dummy
|
||||
driver reports no gamepads, so no test reaches it.
|
||||
|
||||
38. **A font, once loaded, is never freed and cannot be.**
|
||||
`akgl_text_loadfont` (`src/text.c:20`) opens a `TTF_Font`, puts the pointer
|
||||
in `AKGL_REGISTRY_FONT`, and that is the last anyone can do about it: the
|
||||
header exposes no way to close a font, and `SDL_Quit` destroying the
|
||||
property registry drops the last reference. 10,523 bytes per font — 736 of
|
||||
them SDL_ttf's, the rest FreeType's.
|
||||
|
||||
This is bounded by how many fonts a game loads, so it is not the runaway
|
||||
that item 34 is. It is still a gap in the API rather than only a leak: a
|
||||
game that switches fonts between scenes, or a tool like `charviewer` that
|
||||
reloads one while the user picks a size, has no way to give the old one
|
||||
back. Fix: `akgl_text_unloadfont(char *name)` that clears the registry entry
|
||||
and calls `TTF_CloseFont`, and a matching sweep at shutdown.
|
||||
|
||||
39. **Vendored `deps/semver`'s own unit test leaks 188 bytes** across 16 blocks,
|
||||
from the `calloc`s in `test_strcut_first` and `test_strcut_second`
|
||||
(`deps/semver/semver_unit.c:8` and `:21`). Not libakgl's code and not
|
||||
libakgl's to fix; recorded so that nobody re-diagnoses it, and because
|
||||
`semver_unit` is registered as one of our CTest tests and so shows up in our
|
||||
memcheck run. If it becomes noise, the answer is a suppression naming those
|
||||
two functions, not a local edit to a vendored file.
|
||||
|
||||
### Not defects, and why
|
||||
|
||||
- **`tests/util.c` fixtures were reading uninitialised stack floats.** Three
|
||||
null-pointer tests declared `SDL_FRect` and `point` fixtures without
|
||||
initializing them and then made one real call with them at the end, which is
|
||||
sixteen "conditional jump depends on uninitialised value" findings for a test
|
||||
that is not about coordinates. The fixtures are zeroed now. The library was
|
||||
never at fault; a memory checker that reports noise gets ignored, which is the
|
||||
only reason this was worth touching.
|
||||
- **"Still reachable" at exit is not counted.** SDL's global state, the hint
|
||||
table, the property registry and FreeType's library instance are one per
|
||||
process and are reclaimed by `SDL_Quit` / `TTF_Quit`. Counting them would
|
||||
bury the six findings above under a hundred that mean nothing.
|
||||
- **The GPU driver's findings are avoided rather than suppressed.** Running the
|
||||
suite against the real driver produces thousands of findings inside
|
||||
`amdgpu_dri.so`; running it headless produces none, and headless is what the
|
||||
suites are written for anyway.
|
||||
|
||||
## Build notes
|
||||
|
||||
The vendored SDL satellite libraries are built into per-project subdirectories
|
||||
|
||||
Reference in New Issue
Block a user