andrew/libakgl
1
0
Fork 0
Code Issues Pull Requests 1 Actions 1 Packages Projects Releases Wiki Activity

Report the failures that used to be crashes

Browse Source 
Closes Defects items 30 and 31 and Known-and-still-open items 1, 2, 5, 9 and 11.

Both string accessors in json_helpers.c ended their ATTEMPT block with
FINISH(errctx, false), which swallows the failure, and then strncpy'd through
the pointer akgl_heap_next_string never set. So the one condition the pool
exists to report -- it is full, which in practice means something is not
releasing -- arrived as a segfault somewhere else entirely. It is
FINISH(errctx, true) now, and tests/json_helpers.c claims every slot and
asserts AKGL_ERR_HEAP comes back out of both. That test segfaults against the
old code, which is also how the tilemap leak test in the previous commit
confirmed this one.

akgl_tilemap_release tested layers[i].texture and destroyed
tilesets[i].texture, so every tileset texture was freed twice on one release
and no image layer's texture was freed at all. Pointers are cleared as they go,
so a second release is safe instead of a use-after-free.

akgl_game_update_fps called game.lowfpsfunc() unguarded, on a path taken on
frame one because fps is 0 for the first second. Only akgl_game_init installs
it, and renderer.h documents the other path deliberately -- a host that owns
its window binds a backend instead. It installs the default when it finds NULL.

akgl_controller_pushmap and akgl_controller_default checked only the upper
bound, so a negative id indexed before akgl_controlmaps.

The two test-harness helpers were quietly worthless. akgl_render_and_compare
drew t1 on both passes, so it always reported a match and every image assertion
built on it asserted nothing; and akgl_compare_sdl_surfaces memcmp'd
s1->pitch * s1->h bytes out of both surfaces without checking that the second
was the same size, so a smaller one was read past its end. Both fixed, both
tested. tests/util.c also now calls the collide-point test it has defined and
never run.

25/25 pass, memcheck clean, reindent --check and check_error_protocol clean.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Andrew Kesterson
2026-08-01 00:32:11 -04:00
parent 230278d303
commit 21d69192e3
12 changed files with 309 additions and 15 deletions
Download Patch File Download Diff File Expand all files Collapse all files

12
tests/controller.c
View File

@@ -170,6 +170,18 @@ akerr_ErrorContext *test_controller_pushmap(void)
TEST_EXPECT_STATUS(e, AKERR_OUTOFBOUNDS,
akgl_controller_pushmap(AKGL_MAX_CONTROL_MAPS + 5, &control),
"pushing into a control map id past the limit");
// Negative ids index *before* akgl_controlmaps. Both entry points
// checked only the upper bound until 0.5.0.
TEST_EXPECT_STATUS(e, AKERR_OUTOFBOUNDS,
akgl_controller_pushmap(-1, &control),
"pushing into a negative control map id");
TEST_EXPECT_STATUS(e, AKERR_OUTOFBOUNDS,
akgl_controller_pushmap(-4096, &control),
"pushing into a far negative control map id");
TEST_EXPECT_STATUS(e, AKERR_OUTOFBOUNDS,
akgl_controller_default(-1, "player", TEST_KBID, TEST_JSID),
"defaulting a negative control map id");
} CLEANUP {
reset_control_maps();
} PROCESS(e) {

41
tests/game.c
View File

@@ -328,6 +328,46 @@ akerr_ErrorContext *test_game_state_lock(void)
SUCCEED_RETURN(e);
}
/**
* @brief akgl_game_update_fps must not call a lowfpsfunc nobody installed.
*
* `game.fps` is 0 for the first second of the process, which is under the
* threshold, so this fires on frame one. Only akgl_game_init installs the
* default -- and renderer.h documents the other path deliberately: a host that
* owns its own window calls akgl_render_2d_bind instead. Such an embedder
* crashed here on its first frame, through a NULL function pointer.
*/
akerr_ErrorContext *test_game_updateFPS_without_a_lowfps_handler(void)
{
PREPARE_ERROR(e);
ATTEMPT {
set_game_identity();
// Exactly the state a host that never called akgl_game_init is in.
akgl_game.lowfpsfunc = NULL;
akgl_game.fps = 0;
akgl_game.framesSinceUpdate = 0;
akgl_game.lastFPSTime = SDL_GetTicksNS();
akgl_game_update_fps();
TEST_ASSERT(e, akgl_game.lowfpsfunc != NULL,
"akgl_game_update_fps left lowfpsfunc NULL");
TEST_ASSERT(e, akgl_game.lowfpsfunc == &akgl_game_lowfps,
"akgl_game_update_fps installed something other than the default");
// And it keeps working on the frames after.
akgl_game_update_fps();
TEST_ASSERT(e, akgl_game.framesSinceUpdate == 2,
"frames counted as %d over two updates, expected 2",
akgl_game.framesSinceUpdate);
} CLEANUP {
akgl_game.lowfpsfunc = &akgl_game_lowfps;
} PROCESS(e) {
} FINISH(e, true);
SUCCEED_RETURN(e);
}
/** @brief Cleared while the helper thread should keep holding the state mutex. */
static SDL_AtomicInt lockholder_release;
@@ -494,6 +534,7 @@ int main(void)
CATCH(errctx, test_game_state_lock());
CATCH(errctx, test_game_state_lock_budget());
CATCH(errctx, test_game_updateFPS());
CATCH(errctx, test_game_updateFPS_without_a_lowfps_handler());
} CLEANUP {
} PROCESS(errctx) {
} FINISH_NORETURN(errctx);

71
tests/json_helpers.c
View File

@@ -56,6 +56,76 @@ static akerr_ErrorContext *load_fixture(void)
* could safely get wrong depended on which typed accessor they happened to
* call -- and the ones that crashed were the ones used most.
*/
/**
* @brief An exhausted string pool must report AKGL_ERR_HEAP, not crash.
*
* Both string accessors ended their ATTEMPT block with `FINISH(errctx, false)`,
* which swallows the failure instead of passing it up, and then ran
* `strncpy(&(*dest)->data, ...)` through the pointer akgl_heap_next_string
* never set. So the one condition the pool is designed to report -- it is full,
* usually because something is not releasing -- arrived as a segfault inside
* strncpy.
*
* This is what TODO.md Performance item 29 looked like from the outside: not
* "the tilemap loader leaks five strings a load", but a crash somewhere else
* entirely, fifty levels later.
*/
akerr_ErrorContext *test_json_string_accessor_reports_pool_exhaustion(void)
{
PREPARE_ERROR(e);
akgl_String *claimed[AKGL_MAX_HEAP_STRING];
akgl_String *dest = NULL;
json_t *strings = NULL;
int held = 0;
int i = 0;
memset(&claimed, 0x00, sizeof(claimed));
ATTEMPT {
CATCH(e, akgl_get_json_array_value(fixture, "strings", &strings));
// Take every slot the pool has. Whatever else is holding one already
// simply means this stops sooner.
while ( held < AKGL_MAX_HEAP_STRING ) {
akerr_ErrorContext *claim = akgl_heap_next_string(&claimed[held]);
if ( claim != NULL ) {
claim->handled = true;
claim = akerr_release_error(claim);
claimed[held] = NULL;
break;
}
held += 1;
}
TEST_ASSERT(e, held > 0, "could not claim any pool strings");
TEST_ASSERT(e, test_string_pool_used() == AKGL_MAX_HEAP_STRING,
"the string pool is not full: %d of %d claimed",
test_string_pool_used(), AKGL_MAX_HEAP_STRING);
// dest is NULL, so both accessors have to claim -- and cannot.
dest = NULL;
TEST_EXPECT_STATUS(e, AKGL_ERR_HEAP,
akgl_get_json_string_value(fixture, "name", &dest),
"reading a string with the pool exhausted");
TEST_ASSERT(e, dest == NULL,
"a refused string accessor wrote to its destination");
dest = NULL;
TEST_EXPECT_STATUS(e, AKGL_ERR_HEAP,
akgl_get_json_array_index_string(strings, 0, &dest),
"reading an array string with the pool exhausted");
TEST_ASSERT(e, dest == NULL,
"a refused array string accessor wrote to its destination");
} CLEANUP {
for ( i = 0; i < AKGL_MAX_HEAP_STRING; i++ ) {
if ( claimed[i] != NULL ) {
IGNORE(akgl_heap_release_string(claimed[i]));
}
}
} PROCESS(e) {
} FINISH(e, true);
SUCCEED_RETURN(e);
}
akerr_ErrorContext *test_json_accessor_null_arguments(void)
{
PREPARE_ERROR(e);
@@ -422,6 +492,7 @@ int main(void)
CATCH(errctx, test_json_array_index_accessors());
CATCH(errctx, test_json_type_and_key_errors());
CATCH(errctx, test_json_with_default());
CATCH(errctx, test_json_string_accessor_reports_pool_exhaustion());
} CLEANUP {
if ( fixture != NULL ) {
json_decref(fixture);

54
tests/tilemap.c
View File

@@ -241,6 +241,59 @@ akerr_ErrorContext *test_akgl_tilemap_compute_tileset_offsets(void)
* akgl_tilemap_load_layers already bounded its own loop and raised
* AKERR_OUTOFBOUNDS, so the shape to copy was in the same file.
*/
/**
* @brief Releasing a map twice must not free a texture twice.
*
* The layers loop tested `dest->layers[i].texture` and then destroyed
* `dest->tilesets[i].texture` -- already destroyed by the loop above it. So
* every tileset texture was freed twice on a single release, and no image
* 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.
*/
akerr_ErrorContext *test_akgl_tilemap_release_is_idempotent(void)
{
akgl_String *pathstr = NULL;
PREPARE_ERROR(errctx);
int i = 0;
ATTEMPT {
akgl_gamemap = &akgl_default_gamemap;
akgl_renderer = &akgl_default_renderer;
memset((void *)akgl_gamemap, 0x00, sizeof(akgl_Tilemap));
CATCH(errctx, akgl_heap_next_string(&pathstr));
snprintf((char *)&pathstr->data, AKGL_MAX_STRING_LENGTH, "%s%s",
SDL_GetBasePath(), "assets/testmap.tmj");
CATCH(errctx, akgl_tilemap_load((char *)&pathstr->data, akgl_gamemap));
TEST_ASSERT(errctx, akgl_gamemap->numtilesets > 0,
"the fixture map loaded no tilesets, so this test proves nothing");
TEST_ASSERT(errctx, akgl_gamemap->tilesets[0].texture != NULL,
"the fixture map's first tileset has no texture");
TEST_EXPECT_OK(errctx, akgl_tilemap_release(akgl_gamemap), "releasing a loaded map");
for ( i = 0; i < AKGL_TILEMAP_MAX_TILESETS; i++ ) {
TEST_ASSERT(errctx, akgl_gamemap->tilesets[i].texture == NULL,
"tileset %d's texture pointer survived release", i);
}
for ( i = 0; i < AKGL_TILEMAP_MAX_LAYERS; i++ ) {
TEST_ASSERT(errctx, akgl_gamemap->layers[i].texture == NULL,
"layer %d's texture pointer survived release", i);
}
// The one that used to be a use-after-free.
TEST_EXPECT_OK(errctx, akgl_tilemap_release(akgl_gamemap), "releasing the same map again");
TEST_EXPECT_OK(errctx, akgl_tilemap_release(akgl_gamemap), "releasing it a third time");
TEST_EXPECT_STATUS(errctx, AKERR_NULLPOINTER, akgl_tilemap_release(NULL),
"releasing a NULL map");
} CLEANUP {
if ( pathstr != NULL ) {
IGNORE(akgl_heap_release_string(pathstr));
}
} PROCESS(errctx) {
} FINISH(errctx, true);
SUCCEED_RETURN(errctx);
}
akerr_ErrorContext *test_akgl_tilemap_load_bounds_fixed_tables(void)
{
akgl_String *pathstr = NULL;
@@ -614,6 +667,7 @@ int main(void)
CATCH(errctx, test_akgl_tilemap_load_layers());
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());
//CATCH(errctx, test_akgl_tilemap_draw_tileset());
//CATCH(errctx, test_akgl_tilemap_draw());

67
tests/util.c
View File

@@ -356,6 +356,69 @@ akerr_ErrorContext *test_akgl_collide_rectangles_logic(void)
* The loop runs well past AKERR_MAX_ARRAY_ERROR on purpose: at the old
* behaviour this test does not fail, it terminates the suite.
*/
/**
* @brief akgl_compare_sdl_surfaces must check geometry before it memcmps.
*
* It compared `s1->pitch * s1->h` bytes out of both surfaces without looking at
* the second one's dimensions, so a smaller s2 was read past its end rather
* than reported as a mismatch. Benign in practice and immediately fatal under a
* memory checker, which is the reason to fix it rather than leave it.
*/
akerr_ErrorContext *test_akgl_compare_sdl_surfaces_checks_geometry(void)
{
PREPARE_ERROR(errctx);
SDL_Surface *big = NULL;
SDL_Surface *small = NULL;
SDL_Surface *twin = NULL;
ATTEMPT {
big = SDL_CreateSurface(32, 32, SDL_PIXELFORMAT_RGBA8888);
twin = SDL_CreateSurface(32, 32, SDL_PIXELFORMAT_RGBA8888);
small = SDL_CreateSurface(8, 8, SDL_PIXELFORMAT_RGBA8888);
FAIL_ZERO_BREAK(errctx, big, AKGL_ERR_SDL, "%s", SDL_GetError());
FAIL_ZERO_BREAK(errctx, twin, AKGL_ERR_SDL, "%s", SDL_GetError());
FAIL_ZERO_BREAK(errctx, small, AKGL_ERR_SDL, "%s", SDL_GetError());
FAIL_ZERO_BREAK(errctx, SDL_FillSurfaceRect(big, NULL, 0), AKGL_ERR_SDL, "%s", SDL_GetError());
FAIL_ZERO_BREAK(errctx, SDL_FillSurfaceRect(twin, NULL, 0), AKGL_ERR_SDL, "%s", SDL_GetError());
FAIL_ZERO_BREAK(errctx, SDL_FillSurfaceRect(small, NULL, 0), AKGL_ERR_SDL, "%s", SDL_GetError());
TEST_EXPECT_OK(errctx, akgl_compare_sdl_surfaces(big, twin),
"comparing two identical surfaces");
// The one that used to read 4 KiB past the end of an 8x8 surface.
TEST_EXPECT_STATUS(errctx, AKERR_VALUE, akgl_compare_sdl_surfaces(big, small),
"comparing a 32x32 surface against an 8x8 one");
TEST_EXPECT_STATUS(errctx, AKERR_VALUE, akgl_compare_sdl_surfaces(small, big),
"comparing an 8x8 surface against a 32x32 one");
TEST_EXPECT_STATUS(errctx, AKERR_NULLPOINTER, akgl_compare_sdl_surfaces(NULL, big),
"comparing a NULL first surface");
TEST_EXPECT_STATUS(errctx, AKERR_NULLPOINTER, akgl_compare_sdl_surfaces(big, NULL),
"comparing a NULL second surface");
// Same dimensions, different format: the pixels are not comparable even
// though the byte count might be.
SDL_DestroySurface(small);
small = SDL_CreateSurface(32, 32, SDL_PIXELFORMAT_RGB24);
FAIL_ZERO_BREAK(errctx, small, AKGL_ERR_SDL, "%s", SDL_GetError());
TEST_EXPECT_STATUS(errctx, AKERR_VALUE, akgl_compare_sdl_surfaces(big, small),
"comparing two surfaces of different pixel formats");
} CLEANUP {
if ( big != NULL ) {
SDL_DestroySurface(big);
}
if ( twin != NULL ) {
SDL_DestroySurface(twin);
}
if ( small != NULL ) {
SDL_DestroySurface(small);
}
} PROCESS(errctx) {
} FINISH(errctx, true);
SUCCEED_RETURN(errctx);
}
akerr_ErrorContext *test_akgl_path_relative_releases_contexts(void)
{
PREPARE_ERROR(errctx);
@@ -398,8 +461,12 @@ int main(void)
CATCH(errctx, test_akgl_rectangle_points_nullpointers());
CATCH(errctx, test_akgl_rectangle_points_math());
CATCH(errctx, test_akgl_collide_point_rectangle_nullpointers());
// Defined since forever and never called until 0.5.0. TODO.md, "Known
// and still open" item 9.
CATCH(errctx, test_akgl_collide_point_rectangle_logic());
CATCH(errctx, test_akgl_collide_rectangles_nullpointers());
CATCH(errctx, test_akgl_collide_rectangles_logic());
CATCH(errctx, test_akgl_compare_sdl_surfaces_checks_geometry());
CATCH(errctx, test_akgl_path_relative_releases_contexts());
} CLEANUP {
} PROCESS(errctx) {