From 55675cc9debde623ab768f80a2c85e167519e016 Mon Sep 17 00:00:00 2001 From: Andrew Kesterson Date: Fri, 31 Jul 2026 11:02:40 -0400 Subject: [PATCH] Record the twelve defects reading the code for documentation turned up Writing an honest @throws list means reading the implementation against the contract its header claimed, and the two disagreed in more places than the status codes. Recorded as items 14-25 under Defects, ordered by blast radius, each with file, line, consequence and what closing it would touch. They are also noted inline as @note or @warning on the function concerned, so a reader of the generated documentation finds them without coming here first. The three that matter most: - akgl_path_relative returns from inside its ENOENT handler, skipping the RELEASE_ERROR that FINISH carries, so it leaks one of libakerror's 128 error-context slots per call. That is not a rare path - it is the ordinary one, taken whenever an asset names a neighbour relative to its own file. - akgl_sprite_load_json writes json_array_size() entries into a 16-element frameids array with no bound check, and does it through a uint32_t * cast of a uint8_t *. The same unbounded shape appears twice more in the tilemap loader, for objects per layer and tilesets per map. - akgl_heap_release_character zeroes the character without walking its state-to-sprite map, so every sprite reference it took is lost and the SDL property set holding the map is leaked. Releasing characters between levels exhausts the sprite pool. akgl_character_state_sprites_iterate exists to do that walk and is not called from there. Also: the background music never loops, because MIX_PROP_PLAY_LOOPS_NUMBER is set on property set 0 rather than one akgl_load_start_bgm owns; and akgl_get_json_with_default defaults on AKERR_INDEX while the array accessors report a short array as AKERR_OUTOFBOUNDS, so the optional-element form silently does not work for arrays. No code changes. Every src/*.c:NNN citation in the new section was checked against the file after the preceding commit shifted the line numbers. Co-Authored-By: Claude Opus 5 (1M context) --- TODO.md | 153 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 153 insertions(+) diff --git a/TODO.md b/TODO.md index c157c3c..d542467 100644 --- a/TODO.md +++ b/TODO.md @@ -704,6 +704,159 @@ Each was found by a test written to assert correct behavior. an additive release under an unchanged soname. Worth remembering the next time a handful of symbols looks too small to bump for. +### Found while rewriting the Doxygen comments + +Each of these came out of reading an implementation against the contract its +header claimed. They are recorded inline as `@note` or `@warning` on the +function concerned, so a reader of the generated documentation finds them +without coming here first. Ordered by blast radius. + +15. **`akgl_path_relative` leaks one error-context slot per call on its fallback + path.** `src/util.c:120` returns `akgl_path_relative_root(...)` from inside + the `HANDLE(e, ENOENT)` block. `FINISH` is what carries `RELEASE_ERROR`, so + returning before it never gives the context back. libakerror hands these out + of a fixed `AKERR_ARRAY_ERROR[128]`, and this is not a rare path — it is the + *ordinary* one, taken every time an asset names a neighbour relative to its + own file rather than to the working directory. Every tileset image, layer + image and spritesheet in a map costs a slot, permanently. Once the array is + exhausted every subsequent failure anywhere in the process has nowhere to + report from. + + Fix: assign the result to a local, `break`, and return after `FINISH`; or + hoist the fallback out of the handler entirely and let the `HANDLE` block + only record that a retry is wanted. Touches `src/util.c:117-121` only. + +16. **`akgl_sprite_load_json` does not bound the `frames` array.** + `src/sprite.c:165` sets `obj->frames` from `json_array_size()` and + `src/sprite.c:167` then writes that many entries into + `frameids[AKGL_SPRITE_MAX_FRAMES]`, which is 16. A sprite definition with 17 + or more frames writes past the array into the rest of `akgl_Sprite`, and + past the struct into the neighbouring pool slot. It is also written through + a `uint32_t *` cast of a `uint8_t *`, so each element write touches four + bytes; that happens to work on a little-endian machine because the following + iterations overwrite the spill and the last write lands in the struct's + alignment padding, but it is not portable and it is what makes the overflow + reach four bytes past `frames` rather than one. + + Fix: refuse a `frames` array longer than `AKGL_SPRITE_MAX_FRAMES` with + `AKERR_OUTOFBOUNDS`, and read into an `int` local before narrowing to + `uint8_t`. Touches `src/sprite.c:164-169`. + +17. **Two more unbounded array loads in the tilemap loader.** + `src/tilemap.c:387-389` walks a Tiled object layer straight into + `curlayer->objects[j]` with no check against + `AKGL_TILEMAP_MAX_OBJECTS_PER_LAYER` (128), and `src/tilemap.c:278-280` + fills `dest->tilesets[i]` with no check against `AKGL_TILEMAP_MAX_TILESETS` + (16). + Both are the same shape as item 16 and both are reachable from an ordinary + map file: 128 objects is not a large object layer. Note that + `akgl_tilemap_load_layers` *does* bound its loop and raises + `AKERR_OUTOFBOUNDS`, so the pattern to copy is already in the same file. + + Fix: add the bound check at the top of each loop body, matching + `akgl_tilemap_load_layers`. Touches `src/tilemap.c:387` and + `src/tilemap.c:278`. + +18. **`akgl_get_json_with_default` defaults on a status the array accessors never + raise.** `src/json_helpers.c:164-165` handles `AKERR_KEY` and `AKERR_INDEX`, + but `akgl_get_json_array_index_object`, `_integer` and `_string` all report + a short array as `AKERR_OUTOFBOUNDS`. So the "this element is optional" form + silently does not work for an array — the error propagates instead of being + replaced by the default. Only the object accessors, which do raise + `AKERR_KEY`, are actually served by this function today. Nothing in tree + passes an array accessor's error here, which is why it has not been noticed. + + Fix: add a third `HANDLE_GROUP(err, AKERR_OUTOFBOUNDS)`. Note that the + existing two rely on `HANDLE_GROUP` not emitting a `break`, so the `KEY` + case falls through into the `INDEX` body — a third arm has to go *above* the + one holding the `memcpy`, not below it. Touches `src/json_helpers.c:164-167`. + +19. **The background music never loops.** `src/assets.c:20` initialises + `bgmprops` to 0, `src/assets.c:44` sets `MIX_PROP_PLAY_LOOPS_NUMBER` on it, + and `src/assets.c:46` plays the track with it. 0 is SDL's "no property set" + sentinel, not a set this function owns, so the write is rejected — + unchecked — and the play call is given no options. The music plays once and + stops. `akgl_load_start_bgm` reports success either way. + + Fix: `SDL_CreateProperties()` into `bgmprops`, check it, and destroy it in + `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. + + Fix: read the existing entry first and `akgl_heap_release_sprite` it, and + check `SDL_SetPointerProperty`'s return. Touches `src/character.c:43-55`. + +21. **`akgl_heap_release_character` abandons the whole state-to-sprite map.** + `src/heap.c:150` zeroes the character without walking `state_sprites`, so + every sprite reference the character took in `akgl_character_sprite_add` is + lost and the `SDL_PropertiesID` holding the map is never destroyed. Loading + and releasing characters in a loop — level to level — exhausts the sprite + pool and leaks an SDL property set each time. + `akgl_character_state_sprites_iterate` with `AKGL_ITERATOR_OP_RELEASE` exists + precisely to do this walk and is not called from here. + + Fix: enumerate `state_sprites` with that iterator, then + `SDL_DestroyProperties`, before the `memset`. Touches `src/heap.c:145-151`. + +22. **`akgl_path_relative_root` uses `FAIL_RETURN` inside its `ATTEMPT` block.** + `src/util.c:75` returns past `CLEANUP` on the over-long-path branch, so the + two scratch strings claimed at `src/util.c:67-68` are never released. This is + the exact hazard AGENTS.md documents under the error-handling protocol, and + the same class as the heap-string leak already fixed in + `akgl_get_json_tilemap_property`. Smaller blast radius than item 15 because + the branch is rare, but it is a mechanical fix. + + Fix: `FAIL_BREAK`. Touches `src/util.c:75`. + +23. **Three smaller leaks on failure paths.** All the same shape: a resource + acquired before an `ATTEMPT`/`CLEANUP` pair, or released only on the success + path. + + - `src/renderer.c:26-32` — `akgl_render_init2d` releases the two pooled + strings holding the screen dimensions only after both `aksl_atoi` calls + succeed, so a non-numeric `game.screenwidth` leaks two string slots. + - `src/controller.c:80` — `akgl_controller_open_gamepads` calls + `SDL_free(gamepads)` only after the loop completes, so a gamepad that + fails to open leaks the enumeration array. + - `src/text.c:57-64` — `akgl_text_rendertextat` destroys the surface and + texture only on the success path, so a failed texture upload or a failed + draw leaks both. On a HUD line redrawn every frame that is a leak per + frame. + +24. **`akgl_get_property` reads past the end of the property value.** + `src/registry.c:182` copies a fixed `AKGL_MAX_STRING_LENGTH` bytes out of + whatever `SDL_GetStringProperty` returned, rather than that string's length. + The destination is a full-sized pool string so nothing is corrupted, but the + source is an ordinary NUL-terminated string owned by SDL and the read runs + up to PATH_MAX bytes past its end. Benign in practice and immediately fatal + under ASAN, which is the reason to fix it rather than leave it. + + Fix: `aksl_strlcpy` or an explicit length. Touches `src/registry.c:181-186`. + +25. **`akgl_character_load_json_state_int_from_strings` checks the same argument + twice.** `src/character.c:123` guards `states` and `src/character.c:124` + guards `states` again under the message "NULL destination integer" — the + guard for `dest` was never written. A `NULL` `dest` is dereferenced at + `src/character.c:132`. Only reachable from inside the character loader, which + always passes a real pointer, so it is a latent hole rather than a live bug. + + Fix: change the second guard's subject to `dest`. Touches + `src/character.c:124`. + +26. **`akgl_actor_render` computes a sprite's drawn height from its width.** + `src/actor.c:276` sets `dest.h = curSprite->width * obj->scale` where it + should read `curSprite->height`. Every actor is therefore drawn square, and + a non-square sprite is stretched or squashed. Invisible in the current + assets because they are square; it will not stay that way. + + Fix: one word. Touches `src/actor.c:276`. Worth a test that renders a + deliberately non-square sprite and asserts on the destination rectangle. + ## Build notes The vendored SDL satellite libraries are built into per-project subdirectories