From 3ee8c60491ae60c0abe64fcab976a2674f40784c Mon Sep 17 00:00:00 2001 From: Andrew Kesterson Date: Sat, 1 Aug 2026 06:28:34 -0400 Subject: [PATCH] Record the fixes whose TODO.md edits were dropped Eleven entries still described the old code: known-and-still-open items 1, 2, 5, 9 and 11, error-handling items 16, 17 and 18, and defects 20, 29 and 30. The code and the tests were right; the document was not. The edits were lost the same way each time. The scripts that made them built the whole file in memory and wrote it once at the end, so an assertion failure partway through -- on an entry whose text had already been reworded -- threw away that script's earlier, correct replacements along with the failed one. Written one entry at a time now. Co-Authored-By: Claude Opus 5 (1M context) --- TODO.md | 182 +++++++++++++++++++++++++++++++++----------------------- 1 file changed, 108 insertions(+), 74 deletions(-) diff --git a/TODO.md b/TODO.md index 48266eb..d9c896e 100644 --- a/TODO.md +++ b/TODO.md @@ -176,38 +176,48 @@ resolved for every pair it listed. ### 3. Error-handling pattern 16. **`*_RETURN` macros are used inside `ATTEMPT` blocks, which skips `CLEANUP`.** - The established pattern is `FAIL_*_BREAK` / `CATCH` inside `ATTEMPT` and - `*_RETURN` outside it. Violations: - - `src/tilemap.c:52` — `SUCCEED_RETURN` on the success path of - `akgl_get_json_tilemap_property` returns directly from inside `ATTEMPT`, - bypassing the `CLEANUP` at line 54 that releases `tmpstr` and `typestr`. - Every successful property lookup leaks two heap strings, and the string - pool is only 256 entries. - - `src/tilemap.c:620` — `FAIL_RETURN` inside `ATTEMPT`. - - `src/controller.c:286-289,306` — `FAIL_ZERO_RETURN` / `FAIL_NONZERO_RETURN` - inside `ATTEMPT`; `src/controller.c:383` — `SUCCEED_RETURN` inside `ATTEMPT`, - which also leaves `akgl_controller_default` with no return statement on the - path that falls out of `FINISH`. - - `src/game.c:457,462` — `FAIL_NONZERO_RETURN` inside `ATTEMPT`, skipping the - `fclose` in `CLEANUP` at line 482. + **Fixed in 0.5.0.** Ten sites, found by scanning rather than from this list + -- it named six and missed the four in `akgl_collide_rectangles`. -17. **NULL-check discipline varies by function.** In `src/json_helpers.c` only - `akgl_get_json_string_value` (line 71) and `akgl_get_json_array_index_string` - (line 128) validate `key`/`dest`; the other eight accessors validate only the - container and then dereference `dest` unconditionally. In `src/physics.c` the - arcade backends check `actor` but `akgl_physics_null_gravity`, - `_null_collide`, and `_null_move` (lines 15-34) check only `self`. In - `src/renderer.c:65,74`, `akgl_render_2d_frame_start` and `_frame_end` - dereference `self->sdl_renderer` with no check on `self`, while - `akgl_render_2d_draw_texture` (line 82) checks `self` first. + The one that mattered was the success path of + `akgl_get_json_tilemap_property`, which leaked two of the string pool's 256 + entries on every lookup that *found* what it was asked for. A map load does + that several times per layer. -18. **Error-context variable naming is split between `errctx` and `e`, - sometimes within one file.** `src/util.c` uses `e` in the path helpers - (lines 32-116) and `errctx` in the geometry helpers (lines 118-259); - `src/heap.c` uses `errctx` everywhere except `akgl_heap_init_actor` (line 50). - `src/actor.c`, `character.c`, `json_helpers.c`, `registry.c`, `sprite.c`, and - `tilemap.c` favor `errctx`; `game.c`, `physics.c`, `renderer.c`, and - `controller.c` favor `e`. Pick one. + Two needed more than swapping the macro. In + `akgl_get_json_tilemap_property` a plain `break` would have fallen through + to the "property not found" `FAIL_RETURN` after `FINISH`, reporting a miss + for something found, so the success path sets a flag. In + `akgl_collide_rectangles` the eight early exits were followed by + `*collide = false;`, which would have overwritten the hit that broke out of + the block; each corner test writes the flag itself, so that line is gone + rather than moved. + + `akgl_controller_default` was the other behavioural one: its + `SUCCEED_RETURN` was the last statement in the `ATTEMPT` block, so the path + that falls out of `FINISH` reached the closing brace of a non-void function. + + **`scripts/check_error_protocol.py` keeps this closed**, as the + `error_protocol` test. It also enforces the other rule with a silent failure + mode -- no `return` out of a `HANDLE` block. + +17. **NULL-check discipline varies by function.** **Fixed in 0.5.0.** The eight + typed JSON accessors that validated their container and then wrote through + `dest` unconditionally now check `key` and `dest` as the two string + accessors always did; the null physics backend checks its actors like the + arcade one; and `akgl_render_2d_frame_start`, `_frame_end` and `_shutdown` + check `self`, which the first two read straight through. + + `tests/renderer.c` calls all three with `NULL`, which segfaulted before. + +18. **Error-context variable naming is split between `errctx` and `e`.** + **Fixed in 0.5.0**, in its own commit because it is a rename and nothing + else. All 45 remaining sites are `errctx`; applying `\be\b -> errctx` to + each removed line reproduces the added line exactly, and the counts match at + 333 either way. + + `e` keeps its meaning where the convention wants it -- an incoming context + being inspected, as in `akgl_get_json_with_default(e, ...)`. ### 4. Types and macros @@ -658,13 +668,20 @@ Each was found by a test written to assert correct behavior. ### Known and still open 1. **`akgl_render_and_compare` compares a texture against itself.** - `src/util.c:228` and `src/util.c:245` both draw `t1`; `t2` is never rendered, - so the function always passes and the image assertions in `tests/sprite.c` - assert nothing. + **Fixed in 0.5.0**: the second pass draws `t2`. Both passes drew `t1`, so it + always reported a match and every image assertion built on it -- including + the ones in `tests/sprite.c` -- asserted nothing. + 2. **`akgl_tilemap_release` double-frees tileset textures.** - `src/tilemap.c:849` destroys `dest->tilesets[i].texture` inside the *layers* - loop; it should be `dest->layers[i].texture`. It also never NULLs the - pointers, so a second release is a use-after-free. + **Fixed in 0.5.0.** The layers loop tested `layers[i].texture` and destroyed + `tilesets[i].texture`, so every tileset texture was freed twice on a single + release and no image layer's texture was freed at all. Each pointer is + cleared as it goes now, which also makes a second release safe rather than a + use-after-free. + + `tests/tilemap.c` loads the fixture map, releases it three times, and + asserts every texture pointer is `NULL`. + 3. **`akgl_registry_init` never initializes the properties registry.** **Fixed in 0.5.0**, alongside internal-consistency item 36, which is the same function. `akgl_registry_init()` calls `akgl_registry_init_properties()` now, @@ -681,8 +698,11 @@ Each was found by a test written to assert correct behavior. closes item 40, which was about the output-parameter shape it disagreed with the rest of the family on. 5. **`akgl_compare_sdl_surfaces` memcmps without checking geometry.** - `src/util.c:208` compares `s1->pitch * s1->h` bytes of `s2` without verifying - the surfaces share dimensions, pitch, or format. + **Fixed in 0.5.0**: dimensions, pitch and pixel format are compared first, + and any difference is a mismatch. `tests/util.c` compares a 32x32 surface + against an 8x8 one in both directions, which used to read 4 KiB past the end + of the smaller. + 6. **`akgl_string_initialize` overflows by four bytes when `init` is NULL.** **Fixed in 0.5.0**: it zeroes `sizeof(obj->data)`. The four bytes it used to run past the end of the object landed on the *next* pool slot's `refcount`, @@ -709,14 +729,16 @@ Each was found by a test written to assert correct behavior. `next_character` do not. `tests/heap.c` pins the current behavior and says so; decide whether to make them symmetric or document the split. 9. **`tests/util.c` defines `test_akgl_collide_point_rectangle_logic` but - `main()` never calls it.** + `main()` never calls it.** **Fixed in 0.5.0**; it is called, and passes. + 10. **`controller.h` declares functions that do not exist.** **Fixed in 0.5.0**; the definitions carry the declared `akgl_controller_handle_*` names now. See internal-consistency item 7, and `scripts/check_api_surface.sh`, which is what stops this class of drift coming back. 11. **`akgl_controller_pushmap` and `akgl_controller_default` accept negative map - ids.** Both check `controlmapid >= AKGL_MAX_CONTROL_MAPS` but not - `controlmapid < 0`, so a negative id indexes before `GAME_ControlMaps`. + ids.** **Fixed in 0.5.0**: both check the lower bound as well. + `tests/controller.c` passes -1 and -4096 to each. + 12. **A failed controller-DB fetch silently destroys the tracked fallback.** `include/akgl/SDL_GameControllerDB.h` is committed deliberately so the library still builds if upstream disappears. But `mkcontrollermappings.sh` @@ -879,14 +901,18 @@ without coming here first. Ordered by blast radius. `CLEANUP`. Touches `src/assets.c:20-47`. 20. **`akgl_character_sprite_add` leaks a sprite reference when a state is - remapped.** `src/character.c:51` writes the new sprite over any existing - entry for that state without releasing the one it displaces, whose refcount - was incremented when it was added. The displaced sprite's pool slot is never - reclaimed. The write itself is also unchecked, so a failure to record the - mapping is reported as success. + remapped.** **Fixed in 0.5.0**: it reads the existing entry first and + releases it once the new binding is recorded, and the write is checked. - Fix: read the existing entry first and `akgl_heap_release_sprite` it, and - check `SDL_SetPointerProperty`'s return. Touches `src/character.c:43-55`. + The new reference is taken *before* the write and given back if the write + fails, so there is no window where a sprite is bound with nothing behind it. + Rebinding a state to the sprite already there is not treated as a + displacement. + + `tests/character.c` binds, rebinds, and then runs 200 alternating rebinds, + asserting the displaced sprite's count comes back each time. This is the + half **Carried over** item 1 calls replacement; there is still no API to + *remove* a binding without replacing it. 21. **`akgl_heap_release_character` abandons the whole state-to-sprite map.** `src/heap.c:150` zeroed the character without walking `state_sprites`, so @@ -1012,37 +1038,45 @@ Ordered by blast radius. Numbering continues the **Defects** list above. error-handling protocol, which warns about `*_RETURN` inside `ATTEMPT` but not about returning out of `HANDLE`. -29. **`akgl_tilemap_load` leaks five pooled strings per load.** Measured by - counting non-zero `HEAP_STRING` refcounts across load/release cycles: five - per cycle, exactly, and `akgl_tilemap_release` gives none of them back. The - 52nd map load in a process finds the pool empty. +29. **`akgl_tilemap_load` leaks five pooled strings per load.** **Fixed in + 0.5.0**, in two steps, and the split changes what the number means. - Blast radius: every level transition. A game with fifty levels, or one that - reloads a level on death, hits it. The fix is a sweep of the loader's - `CLEANUP` blocks — `akgl_tilemap_load`, `akgl_tilemap_load_layers`, - `akgl_tilemap_load_layer_tile`, `akgl_tilemap_load_layer_objects`, and - `akgl_tilemap_load_tilesets_each` all claim scratch strings — and it wants a - test that asserts the pool is unchanged across a load/release cycle. That is - its own commit, not a footnote to a benchmark. + Three of the five were `akgl_get_json_tilemap_property` leaking two scratch + strings on every *successful* property lookup, through a `SUCCEED_RETURN` + inside its `ATTEMPT` block -- internal-consistency item 16. That took the + measured leak from five per load to two. - Until it is fixed, the tilemap-load benchmark in `tests/perf_render.c` - reclaims the pool by hand between iterations, and says so. + The remaining two were each a claim with no matching release: + `akgl_tilemap_load_layers` never gave back the string it read every layer's + `type` into, and `akgl_tilemap_load` never gave back the `dirname` its + relative paths resolve against. Both release in `CLEANUP` now. + + `tests/tilemap.c` asserts the pool is exactly where it started after one + load/release cycle and after 64 -- enough that a leak of one string per load + could not finish. Finding the last two meant dumping the contents of every + still-claimed slot after a cycle rather than reading the code again; + `'tilelayer'` and an assets directory named themselves immediately. + + Fixed alongside, same file and same class: + `akgl_tilemap_load_layer_objects` released its scratch string after reading + each object's name and then kept using the slot -- and + `akgl_get_json_string_value` reuses a non-`NULL` destination *without* + taking another reference, so the slot was free while still live and any + other claim could have been handed it. + + The tilemap-load benchmark in `tests/perf_render.c` no longer needs to + reclaim the pool by hand between iterations. 30. **Two JSON accessors turn string-pool exhaustion into a segfault.** - `akgl_get_json_string_value` ends its `ATTEMPT` with - `FINISH(errctx, false)` at `src/json_helpers.c:89`, so a failed - `akgl_heap_next_string` is swallowed rather than passed up, and - `src/json_helpers.c:91` then `strncpy`s `AKGL_MAX_STRING_LENGTH` bytes - through the pointer it never set. `akgl_get_json_array_index_string` has the - same pair at `src/json_helpers.c:143` and `src/json_helpers.c:145`. Item 29 - is what makes this reachable, and this is what item 29 looks like from the - outside: not `AKGL_ERR_HEAP`, a crash inside `strncpy` with a NULL - destination. + **Fixed in 0.5.0**: `FINISH(errctx, true)` in both, so a failed + `akgl_heap_next_string` reaches the caller instead of being swallowed and + then `strncpy`d through. - Fix: `FINISH(errctx, true)` in both. One word each, but it changes what - callers see on a path they currently cannot survive, so it wants a test that - exhausts the pool deliberately and asserts `AKGL_ERR_HEAP` comes back out of - both functions. + `tests/json_helpers.c` claims every slot in the pool and asserts + `AKGL_ERR_HEAP` comes back out of both accessors with the destination left + untouched. Against the old code that test segfaults rather than failing -- + and so did the new tilemap property-lookup test, which is how this was + confirmed rather than merely believed. 31. **`akgl_game_update` segfaults if `akgl_game_init` did not run.** **Fixed in 0.5.0**: `akgl_game_update_fps` installs `akgl_game_lowfps` when