diff --git a/TODO.md b/TODO.md index dde29d2..8171084 100644 --- a/TODO.md +++ b/TODO.md @@ -904,39 +904,33 @@ without coming here first. Ordered by blast radius. does not cover a binding replaced while the character is alive. 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. + **Fixed in 0.5.0** with internal-consistency item 16, which swept every + such site. It is `FAIL_BREAK`, and `path_relative_root` is `static` now. + `scripts/check_error_protocol.py` fails the build if one comes back. - Fix: `FAIL_BREAK`. Touches `src/util.c:75`. +23. **Three smaller leaks on failure paths.** **All three fixed in 0.5.0**, each + by moving the release into a `CLEANUP` block. -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. + - `akgl_render_2d_init` released its two pooled strings only after both + `aksl_atoi` calls succeeded, so a non-numeric `game.screenwidth` leaked + two of the pool's 256 entries. `tests/renderer.c` runs 512 failing + initializations against each of the two properties and asserts the pool is + unchanged; against the old code the first loop claims every slot. + - `akgl_controller_open_gamepads` freed the enumeration array only after the + loop completed, so a gamepad that failed to open took the array with it. + The open failure is recorded and reported after the loop, because a + `FAIL_ZERO_BREAK` inside it would have broken the loop rather than the + block. It also frees the array on the "no gamepads enumerated" path, which + SDL still allocates for. + - `akgl_text_rendertextat` destroyed the surface and texture only on the + success path, so a failed upload leaked the surface and a failed draw + leaked both -- once per frame on a HUD line. 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`. + **Already fixed**, as **Defects the memory checker found** item 35 -- the + two entries are the same defect found twice, from reading the code and from + running valgrind. Recorded here only so the duplicate does not read as + outstanding. 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` @@ -1807,14 +1801,14 @@ this library and told it is compatible. each reference, destroys `state_sprites`, and then clears the character, and `akgl_character_state_sprites_iterate` is covered by that path. - **Still open: removal and replacement.** There is no API to unbind one state, - and `akgl_character_sprite_add` still overwrites an existing entry without - releasing the sprite it displaces (**Defects** item 20). A character that - rebinds a state while alive still leaks one reference per rebind; teardown - only gives back whatever the map happens to hold at the end. Wants - `akgl_character_sprite_del`, a release of the displaced entry inside - `akgl_character_sprite_add`, and tests for removal, replacement, and - duplicate sprite bindings across multiple states. + **Replacement is done too**, as **Defects** item 20: + `akgl_character_sprite_add` releases the sprite it displaces, and + `tests/character.c` covers binding, rebinding, and 200 alternating rebinds. + + **Still open: removal.** There is no API to unbind one state without binding + something else over it -- `akgl_character_sprite_del` -- and no test for + duplicate sprite bindings across several states. Neither leaks anything + today; they are a gap in the surface rather than a defect. 2. **An actor cannot be scaled per axis.** `akgl_Actor::scale` is one `float32_t` applied to both `dest.w` and `dest.h`, so there is no way to express "twice as wide, the same diff --git a/include/akgl/text.h b/include/akgl/text.h index d6cff2a..a7b3e87 100644 --- a/include/akgl/text.h +++ b/include/akgl/text.h @@ -69,6 +69,24 @@ akerr_ErrorContext AKERR_NOIGNORE *akgl_text_loadfont(char *name, char *filepath * left with a dangling pointer. Fonts are not reference counted. */ akerr_ErrorContext AKERR_NOIGNORE *akgl_text_unloadfont(char *name); +/** + * @brief Close every font in #AKGL_REGISTRY_FONT and destroy the registry. + * + * A font, once loaded, 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 to them with it. That is + * bounded by how many fonts a game loads rather than unbounded, but there was + * no way to do anything about it at all. + * + * Call this during shutdown, **before** `TTF_Quit` or `SDL_Quit`. Afterwards + * #AKGL_REGISTRY_FONT is 0 and akgl_text_loadfont needs + * akgl_registry_init_font() again before it can register anything. + * + * @return `NULL`. Closing a font reports nothing, and an uninitialized registry + * is success rather than an error -- shutdown paths run after partial + * startups. + */ +akerr_ErrorContext AKERR_NOIGNORE *akgl_text_unloadallfonts(void); /** * @brief Rasterize a string and blit it at a screen position, in one call. * diff --git a/src/character.c b/src/character.c index c0c0238..06ba1b1 100644 --- a/src/character.c +++ b/src/character.c @@ -44,13 +44,37 @@ akerr_ErrorContext *akgl_character_sprite_add(akgl_Character *basechar, akgl_Spr { PREPARE_ERROR(errctx); char stateval[32]; + akgl_Sprite *displaced = NULL; FAIL_ZERO_RETURN(errctx, basechar, AKERR_NULLPOINTER, "NULL character reference"); FAIL_ZERO_RETURN(errctx, ref, AKERR_NULLPOINTER, "NULL sprite reference"); memset(&stateval, 0x00, 32); SDL_itoa(state, (char *)&stateval, 10); - SDL_SetPointerProperty(basechar->state_sprites, (char *)&stateval, ref); - SDL_Log("Added sprite %s to character %s for state %b", (char *)&ref->name, (char *)&basechar->name, state); + + // Whatever this state was bound to is losing its binding, and the reference + // taken for that binding with it. Without this a character that rebinds a + // state while alive leaks one sprite slot per rebind -- releasing at + // teardown only gives back whatever the map happens to hold at the end. + displaced = (akgl_Sprite *)SDL_GetPointerProperty(basechar->state_sprites, (char *)&stateval, NULL); + + // Take the new reference before anything can fail, so a failed write cannot + // leave the sprite bound with no reference behind it. ref->refcount += 1; + if ( SDL_SetPointerProperty(basechar->state_sprites, (char *)&stateval, ref) == false ) { + ref->refcount -= 1; + FAIL_RETURN( + errctx, + AKERR_KEY, + "Unable to bind sprite %s to character %s for state %d: %s", + (char *)&ref->name, + (char *)&basechar->name, + state, + SDL_GetError()); + } + + if ( (displaced != NULL) && (displaced != ref) ) { + PASS(errctx, akgl_heap_release_sprite(displaced)); + } + SDL_Log("Added sprite %s to character %s for state %b", (char *)&ref->name, (char *)&basechar->name, state); SUCCEED_RETURN(errctx); } diff --git a/src/controller.c b/src/controller.c index 3fef438..3b9096a 100644 --- a/src/controller.c +++ b/src/controller.c @@ -212,19 +212,41 @@ akerr_ErrorContext *akgl_controller_open_gamepads(void) int i = 0; SDL_JoystickID *gamepads = NULL; SDL_Gamepad *gamepad = NULL; + bool openfailed = false; PREPARE_ERROR(errctx); if ( SDL_HasGamepad() ) { gamepads = SDL_GetGamepads(&count); if ( count > 0 ) { - FAIL_ZERO_RETURN(errctx, gamepads, AKERR_NULLPOINTER, "%s", SDL_GetError()); - for ( i = 0; i < count ; i++ ) { - gamepad = SDL_OpenGamepad(gamepads[i]); - FAIL_ZERO_RETURN(errctx, gamepad, AKERR_NULLPOINTER, "%s", SDL_GetError()); - SDL_Log("Gamepad %d is %s", i, SDL_GetGamepadNameForID(gamepads[i])); - } - SDL_free(gamepads); + // SDL_free in CLEANUP, not after the loop. A gamepad that failed to + // open used to return straight out of here and take the enumeration + // array with it. + ATTEMPT { + FAIL_ZERO_BREAK(errctx, gamepads, AKERR_NULLPOINTER, "%s", SDL_GetError()); + for ( i = 0; i < count ; i++ ) { + gamepad = SDL_OpenGamepad(gamepads[i]); + if ( gamepad == NULL ) { + openfailed = true; + break; + } + SDL_Log("Gamepad %d is %s", i, SDL_GetGamepadNameForID(gamepads[i])); + } + // Reported after the loop rather than inside it: a + // FAIL_ZERO_BREAK in there would break the loop, not the block. + FAIL_NONZERO_BREAK(errctx, openfailed, AKERR_NULLPOINTER, "%s", SDL_GetError()); + } CLEANUP { + if ( gamepads != NULL ) { + SDL_free(gamepads); + gamepads = NULL; + } + } PROCESS(errctx) { + } FINISH(errctx, true); } else { + // SDL_GetGamepads allocates even when it reports none. + if ( gamepads != NULL ) { + SDL_free(gamepads); + gamepads = NULL; + } SDL_Log("No gamepads enumerated"); } } else { diff --git a/src/renderer.c b/src/renderer.c index bd59005..9ec8081 100644 --- a/src/renderer.c +++ b/src/renderer.c @@ -23,13 +23,26 @@ akerr_ErrorContext *akgl_render_2d_init(akgl_RenderBackend *self) PREPARE_ERROR(errctx); FAIL_ZERO_RETURN(errctx, self, AKERR_NULLPOINTER, "self"); - PASS(errctx, akgl_get_property("game.screenwidth", &width, "0")); - PASS(errctx, akgl_get_property("game.screenheight", &height, "0")); - PASS(errctx, aksl_atoi(width->data, &screenwidth)); - PASS(errctx, aksl_atoi(height->data, &screenheight)); - SDL_Log("Initializing screen (%sx%s = %dx%d)", width->data, height->data, screenwidth, screenheight); - PASS(errctx, akgl_heap_release_string(width)); - PASS(errctx, akgl_heap_release_string(height)); + // The two pooled strings come back in CLEANUP. Released after both parses, + // as they used to be, a non-numeric game.screenwidth returned past them and + // leaked two of the pool's 256 entries. + ATTEMPT { + CATCH(errctx, akgl_get_property("game.screenwidth", &width, "0")); + CATCH(errctx, akgl_get_property("game.screenheight", &height, "0")); + CATCH(errctx, aksl_atoi(width->data, &screenwidth)); + CATCH(errctx, aksl_atoi(height->data, &screenheight)); + SDL_Log("Initializing screen (%sx%s = %dx%d)", width->data, height->data, screenwidth, screenheight); + } CLEANUP { + if ( width != NULL ) { + IGNORE(akgl_heap_release_string(width)); + width = NULL; + } + if ( height != NULL ) { + IGNORE(akgl_heap_release_string(height)); + height = NULL; + } + } PROCESS(errctx) { + } FINISH(errctx, true); FAIL_ZERO_RETURN( errctx, diff --git a/src/text.c b/src/text.c index 2e269a6..6a78d44 100644 --- a/src/text.c +++ b/src/text.c @@ -54,6 +54,38 @@ akerr_ErrorContext *akgl_text_unloadfont(char *name) SUCCEED_RETURN(errctx); } +/** + * @brief SDL_EnumerateProperties callback: close one registered font. + * + * The properties set cannot be modified while it is being enumerated, so this + * only closes; akgl_text_unloadallfonts clears the whole set afterwards. + */ +static void unload_font_iterate(void *userdata, SDL_PropertiesID props, const char *name) +{ + TTF_Font *font = NULL; + + if ( name == NULL ) { + return; + } + font = (TTF_Font *)SDL_GetPointerProperty(props, name, NULL); + if ( font != NULL ) { + TTF_CloseFont(font); + } +} + +akerr_ErrorContext *akgl_text_unloadallfonts(void) +{ + PREPARE_ERROR(errctx); + + if ( AKGL_REGISTRY_FONT == 0 ) { + SUCCEED_RETURN(errctx); + } + SDL_EnumerateProperties(AKGL_REGISTRY_FONT, &unload_font_iterate, NULL); + SDL_DestroyProperties(AKGL_REGISTRY_FONT); + AKGL_REGISTRY_FONT = 0; + SUCCEED_RETURN(errctx); +} + akerr_ErrorContext *akgl_text_rendertextat(TTF_Font *font, char *text, SDL_Color color, int wraplength, int x, int y) { SDL_Surface *textsurf = NULL; @@ -85,14 +117,26 @@ akerr_ErrorContext *akgl_text_rendertextat(TTF_Font *font, char *text, SDL_Color color); } FAIL_ZERO_RETURN(errctx, textsurf, AKERR_NULLPOINTER, "%s", SDL_GetError()); - texture = SDL_CreateTextureFromSurface(akgl_renderer->sdl_renderer, textsurf); - FAIL_ZERO_RETURN(errctx, texture, AKERR_NULLPOINTER, "%s", SDL_GetError()); - dest.x = x; - dest.y = y; - SDL_GetTextureSize(texture, &dest.w, &dest.h); - PASS(errctx, akgl_renderer->draw_texture(akgl_renderer, texture, NULL, &dest, 0, NULL, SDL_FLIP_NONE)); - SDL_DestroyTexture(texture); - SDL_DestroySurface(textsurf); + + // Both were destroyed only after the draw succeeded, so a failed texture + // upload leaked the surface and a failed draw leaked both -- on a HUD line + // redrawn every frame, that is a leak per frame. + ATTEMPT { + texture = SDL_CreateTextureFromSurface(akgl_renderer->sdl_renderer, textsurf); + FAIL_ZERO_BREAK(errctx, texture, AKERR_NULLPOINTER, "%s", SDL_GetError()); + dest.x = x; + dest.y = y; + SDL_GetTextureSize(texture, &dest.w, &dest.h); + CATCH(errctx, akgl_renderer->draw_texture(akgl_renderer, texture, NULL, &dest, 0, NULL, SDL_FLIP_NONE)); + } CLEANUP { + if ( texture != NULL ) { + SDL_DestroyTexture(texture); + texture = NULL; + } + SDL_DestroySurface(textsurf); + textsurf = NULL; + } PROCESS(errctx) { + } FINISH(errctx, true); SUCCEED_RETURN(errctx); } diff --git a/src/tilemap.c b/src/tilemap.c index e0af3e2..fa622b4 100644 --- a/src/tilemap.c +++ b/src/tilemap.c @@ -322,41 +322,58 @@ akerr_ErrorContext *akgl_tilemap_load_layer_objects(akgl_Tilemap *dest, json_t * PASS(errctx, akgl_get_json_array_value(root, "objects", &layerdata)); len = json_array_size((json_t *)layerdata); curlayer = &dest->layers[layerid]; - for ( j = 0; j < len; j++ ) { - FAIL_NONZERO_RETURN( - errctx, - (j >= AKGL_TILEMAP_MAX_OBJECTS_PER_LAYER), - AKERR_OUTOFBOUNDS, - "Object layer %d has more than %d objects", - layerid, - AKGL_TILEMAP_MAX_OBJECTS_PER_LAYER - ); - PASS(errctx, akgl_get_json_array_index_object((json_t *)layerdata, j, &layerdatavalue)); - curobj = &curlayer->objects[j]; - PASS(errctx, akgl_get_json_string_value((json_t *)layerdatavalue, "name", &tmpstr)); - strncpy((char *)curobj->name, tmpstr->data, AKGL_ACTOR_MAX_NAME_LENGTH); - PASS(errctx, akgl_heap_release_string(tmpstr)); - PASS(errctx, akgl_get_json_number_value((json_t *)layerdatavalue, "x", &curobj->x)); - PASS(errctx, akgl_get_json_number_value((json_t *)layerdatavalue, "y", &curobj->y)); - PASS(errctx, akgl_get_json_boolean_value((json_t *)layerdatavalue, "visible", &curobj->visible)); - PASS(errctx, akgl_get_json_string_value((json_t *)layerdatavalue, "type", &tmpstr)); - if ( strcmp(tmpstr->data, "actor") == 0 ) { - PASS(errctx, akgl_tilemap_load_layer_object_actor(curobj, layerdatavalue, layerid, dirname)); - } else if ( strcmp(tmpstr->data, "perspective") == 0 ) { - curobj->visible = false; - if ( strcmp((char *)curobj->name, "p_foreground") == 0 ) { - dest->p_foreground_y = curobj->y; - PASS(errctx, akgl_get_json_integer_value((json_t *)layerdatavalue, "height", &dest->p_foreground_h)); - PASS(errctx, akgl_get_json_properties_float((json_t *)layerdatavalue, "scale", &dest->p_foreground_scale)); - } else if ( strcmp((char *)curobj->name, "p_vanishing") == 0 ) { - dest->p_vanishing_y = curobj->y; - PASS(errctx, akgl_get_json_integer_value((json_t *)layerdatavalue, "height", &dest->p_vanishing_h)); - PASS(errctx, akgl_get_json_properties_float((json_t *)layerdatavalue, "scale", &dest->p_vanishing_scale)); + // The loop is the whole ATTEMPT body on purpose. A CATCH inside a loop + // breaks the loop rather than the block, which is only safe because there + // is nothing after it -- CLEANUP, PROCESS and FINISH still run and still + // propagate. Anything added after this loop has to account for that. + ATTEMPT { + for ( j = 0; j < len; j++ ) { + FAIL_NONZERO_BREAK( + errctx, + (j >= AKGL_TILEMAP_MAX_OBJECTS_PER_LAYER), + AKERR_OUTOFBOUNDS, + "Object layer %d has more than %d objects", + layerid, + AKGL_TILEMAP_MAX_OBJECTS_PER_LAYER + ); + CATCH(errctx, akgl_get_json_array_index_object((json_t *)layerdata, j, &layerdatavalue)); + curobj = &curlayer->objects[j]; + // One scratch string for the whole walk, released once in CLEANUP. + // Releasing it after the name read -- which is what this used to do + // -- dropped its refcount to zero while the very next line went on + // using the same slot, because akgl_get_json_string_value reuses a + // non-NULL destination without taking another reference. Any other + // claim in between would have been handed the same string. + CATCH(errctx, akgl_get_json_string_value((json_t *)layerdatavalue, "name", &tmpstr)); + strncpy((char *)curobj->name, tmpstr->data, AKGL_ACTOR_MAX_NAME_LENGTH); + CATCH(errctx, akgl_get_json_number_value((json_t *)layerdatavalue, "x", &curobj->x)); + CATCH(errctx, akgl_get_json_number_value((json_t *)layerdatavalue, "y", &curobj->y)); + CATCH(errctx, akgl_get_json_boolean_value((json_t *)layerdatavalue, "visible", &curobj->visible)); + + CATCH(errctx, akgl_get_json_string_value((json_t *)layerdatavalue, "type", &tmpstr)); + if ( strcmp(tmpstr->data, "actor") == 0 ) { + CATCH(errctx, akgl_tilemap_load_layer_object_actor(curobj, layerdatavalue, layerid, dirname)); + } else if ( strcmp(tmpstr->data, "perspective") == 0 ) { + curobj->visible = false; + if ( strcmp((char *)curobj->name, "p_foreground") == 0 ) { + dest->p_foreground_y = curobj->y; + CATCH(errctx, akgl_get_json_integer_value((json_t *)layerdatavalue, "height", &dest->p_foreground_h)); + CATCH(errctx, akgl_get_json_properties_float((json_t *)layerdatavalue, "scale", &dest->p_foreground_scale)); + } else if ( strcmp((char *)curobj->name, "p_vanishing") == 0 ) { + dest->p_vanishing_y = curobj->y; + CATCH(errctx, akgl_get_json_integer_value((json_t *)layerdatavalue, "height", &dest->p_vanishing_h)); + CATCH(errctx, akgl_get_json_properties_float((json_t *)layerdatavalue, "scale", &dest->p_vanishing_scale)); + } } + layerdatavalue = NULL; } - layerdatavalue = NULL; - } + } CLEANUP { + if ( tmpstr != NULL ) { + IGNORE(akgl_heap_release_string(tmpstr)); + } + } PROCESS(errctx) { + } FINISH(errctx, true); SUCCEED_RETURN(errctx); } @@ -471,6 +488,12 @@ akerr_ErrorContext *akgl_tilemap_load_layers(akgl_Tilemap *dest, json_t *root, a layerid += 1; } } CLEANUP { + // One scratch string for every layer's type, given back once. It was + // claimed by the first accessor call and never released, which is one + // of the two pool strings a map load used to keep for good. + if ( tmpstr != NULL ) { + IGNORE(akgl_heap_release_string(tmpstr)); + } } PROCESS(errctx) { } FINISH(errctx, true); SUCCEED_RETURN(errctx); @@ -618,7 +641,6 @@ akerr_ErrorContext *akgl_tilemap_load(char *fname, akgl_Tilemap *dest) SDL_Log("Map perspective rate is %f", dest->p_rate); } } CLEANUP { - //IGNORE(akgl_heap_release_string(tmpstr)); // The map is built entirely out of copies -- layer data, tileset // geometry, object names -- so the document is dead the moment the // loaders above return, whether they succeeded or not. It is also the @@ -627,6 +649,13 @@ akerr_ErrorContext *akgl_tilemap_load(char *fname, akgl_Tilemap *dest) json_decref(json); json = NULL; } + // And the directory the map's relative paths resolve against, which + // was claimed above and never released -- the other of the two pool + // strings a map load kept. + if ( dirnamestr != NULL ) { + IGNORE(akgl_heap_release_string(dirnamestr)); + dirnamestr = NULL; + } } PROCESS(errctx) { } FINISH(errctx, true); SUCCEED_RETURN(errctx); diff --git a/tests/character.c b/tests/character.c index a5b18fb..40911e1 100644 --- a/tests/character.c +++ b/tests/character.c @@ -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); diff --git a/tests/renderer.c b/tests/renderer.c index df62bfc..72586e6 100644 --- a/tests/renderer.c +++ b/tests/renderer.c @@ -20,6 +20,7 @@ #include #include #include +#include #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) { diff --git a/tests/text.c b/tests/text.c index 536cd0c..b9abf08 100644 --- a/tests/text.c +++ b/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); diff --git a/tests/tilemap.c b/tests/tilemap.c index fdb39d8..5cf67ae 100644 --- a/tests/tilemap.c +++ b/tests/tilemap.c @@ -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());