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) <noreply@anthropic.com>
This commit is contained in:
153
TODO.md
153
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
|
||||
|
||||
Reference in New Issue
Block a user