Give back every pooled string and sprite reference the loaders take
Closes Defects items 20, 22, 23 and 29, and the residual of item 38. The tilemap load leak was five pooled strings per load; the property-lookup fix in an earlier commit took it to two, and the last two were each a claim with no matching release -- the string every layer's `type` was read into, and the dirname the map's relative paths resolve against. tests/tilemap.c asserts the pool is exactly where it started after one load/release cycle and after 64. Finding those two was a matter of 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. Same file, same class, fixed with it: akgl_tilemap_load_layer_objects released its scratch string after reading each object's name and then kept using the slot, because akgl_get_json_string_value reuses a non-NULL destination without taking another reference. The slot was free while still live, so any other claim could have been handed it. akgl_character_sprite_add wrote over an existing binding without releasing the sprite it displaced, so a character that rebinds a state while alive leaked a sprite slot per rebind -- teardown only gives back what the map holds at the end. 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. The write was unchecked too. Three failure-path leaks moved into CLEANUP blocks: akgl_render_2d_init's two pooled strings, akgl_controller_open_gamepads' enumeration array, and akgl_text_rendertextat's surface and texture -- the last being a leak per frame on a HUD line. akgl_text_unloadallfonts() closes every font in the registry and destroys it, which is what item 38 left open. Deliberately not a whole akgl_game_shutdown: tearing down the mixer, SDL_ttf and SDL in the right order is a design question, and this is the part that was simply missing. It is a new public symbol, which 0.5.0 already covers -- this release has not shipped. Every fix has a test that fails against the old code. 25/25 pass, memcheck clean, reindent --check, check_api_surface and check_error_protocol all clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -203,6 +203,84 @@ akerr_ErrorContext *test_akgl_character_load_json()
|
||||
SUCCEED_RETURN(errctx);
|
||||
}
|
||||
|
||||
/**
|
||||
* @brief Rebinding a state must release the sprite it displaces.
|
||||
*
|
||||
* akgl_character_sprite_add took a reference for every binding and wrote over
|
||||
* any existing entry without releasing the one it replaced, so a character that
|
||||
* rebinds a state while alive leaked a sprite slot per rebind. Releasing at
|
||||
* teardown does not cover it: the map only holds the last binding by then, so
|
||||
* the earlier ones are unreachable.
|
||||
*
|
||||
* The write itself was also unchecked, so a failure to record the binding was
|
||||
* reported as success -- with the reference already taken.
|
||||
*/
|
||||
akerr_ErrorContext *test_character_sprite_rebind_releases_displaced(void)
|
||||
{
|
||||
PREPARE_ERROR(errctx);
|
||||
akgl_Character *testchar = NULL;
|
||||
akgl_Sprite *first = NULL;
|
||||
akgl_Sprite *second = NULL;
|
||||
akgl_Sprite *found = NULL;
|
||||
int firstbefore = 0;
|
||||
int secondbefore = 0;
|
||||
int i = 0;
|
||||
|
||||
ATTEMPT {
|
||||
CATCH(errctx, akgl_heap_next_character(&testchar));
|
||||
CATCH(errctx, akgl_character_initialize(testchar, "rebindchar"));
|
||||
// Real sprites out of the fixtures: akgl_sprite_initialize refuses a
|
||||
// NULL spritesheet, and a hand-built sprite would not be a sprite.
|
||||
CATCH(errctx, akgl_sprite_load_json("assets/testsprite.json"));
|
||||
CATCH(errctx, akgl_sprite_load_json("assets/testsprite2.json"));
|
||||
first = SDL_GetPointerProperty(AKGL_REGISTRY_SPRITE, "testsprite", NULL);
|
||||
second = SDL_GetPointerProperty(AKGL_REGISTRY_SPRITE, "testsprite2", NULL);
|
||||
FAIL_ZERO_BREAK(errctx, first, AKERR_KEY, "testsprite is not in the registry");
|
||||
FAIL_ZERO_BREAK(errctx, second, AKERR_KEY, "testsprite2 is not in the registry");
|
||||
|
||||
firstbefore = first->refcount;
|
||||
secondbefore = second->refcount;
|
||||
|
||||
CATCH(errctx, akgl_character_sprite_add(testchar, first, AKGL_ACTOR_STATE_ALIVE));
|
||||
TEST_ASSERT(errctx, first->refcount == (firstbefore + 1),
|
||||
"binding a sprite did not take a reference (%d, expected %d)",
|
||||
first->refcount, firstbefore + 1);
|
||||
|
||||
// Rebind the same state to a different sprite.
|
||||
CATCH(errctx, akgl_character_sprite_add(testchar, second, AKGL_ACTOR_STATE_ALIVE));
|
||||
TEST_ASSERT(errctx, second->refcount == (secondbefore + 1),
|
||||
"rebinding did not take a reference on the new sprite (%d, expected %d)",
|
||||
second->refcount, secondbefore + 1);
|
||||
TEST_ASSERT(errctx, first->refcount == firstbefore,
|
||||
"rebinding did not release the displaced sprite (%d, expected %d)",
|
||||
first->refcount, firstbefore);
|
||||
|
||||
CATCH(errctx, testchar->sprite_get(testchar, AKGL_ACTOR_STATE_ALIVE, &found));
|
||||
TEST_ASSERT(errctx, found == second, "the state is not bound to the new sprite");
|
||||
|
||||
// Rebinding a state to the sprite already there must be a no-op on the
|
||||
// count rather than a release followed by a claim of the same slot.
|
||||
CATCH(errctx, akgl_character_sprite_add(testchar, second, AKGL_ACTOR_STATE_ALIVE));
|
||||
TEST_ASSERT(errctx, second->refcount == (secondbefore + 2),
|
||||
"rebinding a state to the sprite it already held mishandled the count (%d)",
|
||||
second->refcount);
|
||||
|
||||
// And a long run of rebinds must not drift.
|
||||
for ( i = 0; i < 200; i++ ) {
|
||||
CATCH(errctx, akgl_character_sprite_add(testchar, first, AKGL_ACTOR_STATE_DEAD));
|
||||
CATCH(errctx, akgl_character_sprite_add(testchar, second, AKGL_ACTOR_STATE_DEAD));
|
||||
}
|
||||
TEST_ASSERT(errctx, first->refcount <= (firstbefore + 1),
|
||||
"200 rebinds left the first sprite at refcount %d", first->refcount);
|
||||
} CLEANUP {
|
||||
if ( testchar != NULL ) {
|
||||
IGNORE(akgl_heap_release_character(testchar));
|
||||
}
|
||||
} PROCESS(errctx) {
|
||||
} FINISH(errctx, true);
|
||||
SUCCEED_RETURN(errctx);
|
||||
}
|
||||
|
||||
int main(void)
|
||||
{
|
||||
PREPARE_ERROR(errctx);
|
||||
@@ -228,6 +306,7 @@ int main(void)
|
||||
CATCH(errctx, test_character_sprite_mgmt());
|
||||
CATCH(errctx, test_character_iterate_state_sprites());
|
||||
CATCH(errctx, test_akgl_character_load_json());
|
||||
CATCH(errctx, test_character_sprite_rebind_releases_displaced());
|
||||
} CLEANUP {
|
||||
} PROCESS(errctx) {
|
||||
} FINISH_NORETURN(errctx);
|
||||
|
||||
@@ -20,6 +20,7 @@
|
||||
#include <akgl/error.h>
|
||||
#include <akgl/game.h>
|
||||
#include <akgl/renderer.h>
|
||||
#include <akgl/registry.h>
|
||||
|
||||
#include "testutil.h"
|
||||
|
||||
@@ -187,6 +188,58 @@ akerr_ErrorContext *test_render_unimplemented_and_guards(void)
|
||||
SUCCEED_RETURN(errctx);
|
||||
}
|
||||
|
||||
/**
|
||||
* @brief akgl_render_2d_init must give back its pooled strings when a parse fails.
|
||||
*
|
||||
* It reads game.screenwidth and game.screenheight into two pool strings and
|
||||
* used to release them only after both had parsed, so a non-numeric value
|
||||
* returned past both and leaked two of the pool's 256 entries -- every time a
|
||||
* host started with a bad configuration and retried.
|
||||
*
|
||||
* The window creation after the parse is what makes the success path need a
|
||||
* display, so this only drives the failure path. That is the one that leaked.
|
||||
*/
|
||||
akerr_ErrorContext *test_render_2d_init_releases_strings_on_failure(void)
|
||||
{
|
||||
PREPARE_ERROR(errctx);
|
||||
akgl_RenderBackend backend;
|
||||
int baseline = 0;
|
||||
int i = 0;
|
||||
|
||||
ATTEMPT {
|
||||
CATCH(errctx, akgl_registry_init_properties());
|
||||
memset(&backend, 0x00, sizeof(akgl_RenderBackend));
|
||||
|
||||
CATCH(errctx, akgl_set_property("game.screenwidth", "not-a-number"));
|
||||
CATCH(errctx, akgl_set_property("game.screenheight", "480"));
|
||||
|
||||
baseline = test_string_pool_used();
|
||||
for ( i = 0; i < (AKGL_MAX_HEAP_STRING * 2); i++ ) {
|
||||
TEST_EXPECT_ANY_ERROR(errctx, akgl_render_2d_init(&backend),
|
||||
"initializing a 2D renderer from an unparseable width");
|
||||
}
|
||||
TEST_ASSERT(errctx, test_string_pool_used() == baseline,
|
||||
"%d failed initializations left %d pool strings claimed, expected %d",
|
||||
(AKGL_MAX_HEAP_STRING * 2), test_string_pool_used(), baseline);
|
||||
|
||||
// The second property is the one read after the first parse, so fail on
|
||||
// it too and check the other order.
|
||||
CATCH(errctx, akgl_set_property("game.screenwidth", "640"));
|
||||
CATCH(errctx, akgl_set_property("game.screenheight", "also-not-a-number"));
|
||||
baseline = test_string_pool_used();
|
||||
for ( i = 0; i < (AKGL_MAX_HEAP_STRING * 2); i++ ) {
|
||||
TEST_EXPECT_ANY_ERROR(errctx, akgl_render_2d_init(&backend),
|
||||
"initializing a 2D renderer from an unparseable height");
|
||||
}
|
||||
TEST_ASSERT(errctx, test_string_pool_used() == baseline,
|
||||
"%d failed initializations left %d pool strings claimed, expected %d",
|
||||
(AKGL_MAX_HEAP_STRING * 2), test_string_pool_used(), baseline);
|
||||
} CLEANUP {
|
||||
} PROCESS(errctx) {
|
||||
} FINISH(errctx, true);
|
||||
SUCCEED_RETURN(errctx);
|
||||
}
|
||||
|
||||
int main(void)
|
||||
{
|
||||
PREPARE_ERROR(errctx);
|
||||
@@ -226,6 +279,7 @@ int main(void)
|
||||
CATCH(errctx, test_render_backend_without_a_renderer());
|
||||
CATCH(errctx, test_render_frames_and_textures());
|
||||
CATCH(errctx, test_render_unimplemented_and_guards());
|
||||
CATCH(errctx, test_render_2d_init_releases_strings_on_failure());
|
||||
} CLEANUP {
|
||||
SDL_Quit();
|
||||
} PROCESS(errctx) {
|
||||
|
||||
47
tests/text.c
47
tests/text.c
@@ -321,6 +321,52 @@ akerr_ErrorContext *test_text_rendertextat(void)
|
||||
SUCCEED_RETURN(errctx);
|
||||
}
|
||||
|
||||
/**
|
||||
* @brief akgl_text_unloadallfonts closes every registered font and clears the registry.
|
||||
*
|
||||
* A font was reachable only by name, so a game that exited without unloading
|
||||
* each one by hand left them open -- and SDL_Quit destroying the property
|
||||
* registry took the last reference with it. Roughly 10 KB per font, most of it
|
||||
* FreeType's. Bounded by how many fonts a game loads, but there was nothing a
|
||||
* game could do about it.
|
||||
*/
|
||||
akerr_ErrorContext *test_text_unloadallfonts(void)
|
||||
{
|
||||
PREPARE_ERROR(errctx);
|
||||
|
||||
ATTEMPT {
|
||||
CATCH(errctx, akgl_registry_init_font());
|
||||
CATCH(errctx, akgl_text_loadfont("unloadall_a", TEST_FONT_PATH, 16));
|
||||
CATCH(errctx, akgl_text_loadfont("unloadall_b", TEST_FONT_PATH, 24));
|
||||
TEST_ASSERT(errctx,
|
||||
SDL_GetPointerProperty(AKGL_REGISTRY_FONT, "unloadall_a", NULL) != NULL,
|
||||
"the first font is not in the registry");
|
||||
TEST_ASSERT(errctx,
|
||||
SDL_GetPointerProperty(AKGL_REGISTRY_FONT, "unloadall_b", NULL) != NULL,
|
||||
"the second font is not in the registry");
|
||||
|
||||
TEST_EXPECT_OK(errctx, akgl_text_unloadallfonts(), "unloading every font");
|
||||
TEST_ASSERT(errctx, AKGL_REGISTRY_FONT == 0,
|
||||
"unloading every font left the registry behind");
|
||||
|
||||
// Shutdown paths run after partial startups, so a second call and a
|
||||
// call against no registry at all are both success.
|
||||
TEST_EXPECT_OK(errctx, akgl_text_unloadallfonts(), "unloading every font again");
|
||||
|
||||
// And the subsystem comes back with a fresh registry.
|
||||
CATCH(errctx, akgl_registry_init_font());
|
||||
TEST_EXPECT_OK(errctx, akgl_text_loadfont("unloadall_c", TEST_FONT_PATH, 16),
|
||||
"loading a font after unloading them all");
|
||||
TEST_EXPECT_OK(errctx, akgl_text_unloadallfonts(), "unloading that one too");
|
||||
} CLEANUP {
|
||||
if ( AKGL_REGISTRY_FONT == 0 ) {
|
||||
IGNORE(akgl_registry_init_font());
|
||||
}
|
||||
} PROCESS(errctx) {
|
||||
} FINISH(errctx, true);
|
||||
SUCCEED_RETURN(errctx);
|
||||
}
|
||||
|
||||
int main(void)
|
||||
{
|
||||
PREPARE_ERROR(errctx);
|
||||
@@ -377,6 +423,7 @@ int main(void)
|
||||
CATCH(errctx, test_text_measure_wrapped());
|
||||
CATCH(errctx, test_text_render_backend_guards());
|
||||
CATCH(errctx, test_text_rendertextat());
|
||||
CATCH(errctx, test_text_unloadallfonts());
|
||||
} CLEANUP {
|
||||
if ( testfont != NULL ) {
|
||||
TTF_CloseFont(testfont);
|
||||
|
||||
@@ -250,6 +250,62 @@ akerr_ErrorContext *test_akgl_tilemap_compute_tileset_offsets(void)
|
||||
* layer's texture was freed at all. Neither pointer was cleared either, so a
|
||||
* second release was a use-after-free on top of that.
|
||||
*/
|
||||
/**
|
||||
* @brief A load/release cycle must give back every pooled string it took.
|
||||
*
|
||||
* Measured before the fix by counting non-zero HEAP_STRING refcounts across
|
||||
* cycles: five per cycle, exactly, and akgl_tilemap_release gave none of them
|
||||
* back. The pool is 256 entries, so the 52nd map load in a process found it
|
||||
* empty -- and until the accessors were fixed, "empty" arrived as a segfault
|
||||
* rather than as AKGL_ERR_HEAP.
|
||||
*
|
||||
* Blast radius is every level transition, and a game that reloads a level on
|
||||
* death hits it sooner.
|
||||
*/
|
||||
akerr_ErrorContext *test_akgl_tilemap_load_releases_strings(void)
|
||||
{
|
||||
akgl_String *pathstr = NULL;
|
||||
PREPARE_ERROR(errctx);
|
||||
int baseline = 0;
|
||||
int afterfirst = 0;
|
||||
int i = 0;
|
||||
|
||||
ATTEMPT {
|
||||
akgl_gamemap = &akgl_default_gamemap;
|
||||
akgl_renderer = &akgl_default_renderer;
|
||||
CATCH(errctx, akgl_heap_next_string(&pathstr));
|
||||
snprintf((char *)&pathstr->data, AKGL_MAX_STRING_LENGTH, "%s%s",
|
||||
SDL_GetBasePath(), "assets/testmap.tmj");
|
||||
|
||||
baseline = test_string_pool_used();
|
||||
|
||||
memset((void *)akgl_gamemap, 0x00, sizeof(akgl_Tilemap));
|
||||
CATCH(errctx, akgl_tilemap_load((char *)&pathstr->data, akgl_gamemap));
|
||||
CATCH(errctx, akgl_tilemap_release(akgl_gamemap));
|
||||
afterfirst = test_string_pool_used();
|
||||
TEST_ASSERT(errctx, afterfirst == baseline,
|
||||
"one load/release cycle left %d pool strings claimed, expected %d",
|
||||
afterfirst, baseline);
|
||||
|
||||
// Then enough cycles that a leak of even one string per load would run
|
||||
// the pool dry, so this cannot pass by rounding.
|
||||
for ( i = 0; i < 64; i++ ) {
|
||||
memset((void *)akgl_gamemap, 0x00, sizeof(akgl_Tilemap));
|
||||
CATCH(errctx, akgl_tilemap_load((char *)&pathstr->data, akgl_gamemap));
|
||||
CATCH(errctx, akgl_tilemap_release(akgl_gamemap));
|
||||
}
|
||||
TEST_ASSERT(errctx, test_string_pool_used() == baseline,
|
||||
"64 load/release cycles left %d pool strings claimed, expected %d",
|
||||
test_string_pool_used(), baseline);
|
||||
} CLEANUP {
|
||||
if ( pathstr != NULL ) {
|
||||
IGNORE(akgl_heap_release_string(pathstr));
|
||||
}
|
||||
} PROCESS(errctx) {
|
||||
} FINISH(errctx, true);
|
||||
SUCCEED_RETURN(errctx);
|
||||
}
|
||||
|
||||
akerr_ErrorContext *test_akgl_tilemap_release_is_idempotent(void)
|
||||
{
|
||||
akgl_String *pathstr = NULL;
|
||||
@@ -668,6 +724,7 @@ int main(void)
|
||||
CATCH(errctx, test_akgl_tilemap_load_tilesets());
|
||||
CATCH(errctx, test_akgl_tilemap_load_bounds_fixed_tables());
|
||||
CATCH(errctx, test_akgl_tilemap_release_is_idempotent());
|
||||
CATCH(errctx, test_akgl_tilemap_load_releases_strings());
|
||||
//CATCH(errctx, test_akgl_tilemap_load());
|
||||
//CATCH(errctx, test_akgl_tilemap_draw_tileset());
|
||||
//CATCH(errctx, test_akgl_tilemap_draw());
|
||||
|
||||
Reference in New Issue
Block a user