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