From 21d69192e3c698a636ce727885327217b64363cb Mon Sep 17 00:00:00 2001 From: Andrew Kesterson Date: Sat, 1 Aug 2026 00:32:11 -0400 Subject: [PATCH] Report the failures that used to be crashes 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) --- include/akgl/game.h | 2 +- include/akgl/util.h | 19 +++++++----- src/controller.c | 4 +-- src/game.c | 8 +++++ src/json_helpers.c | 12 ++++++-- src/tilemap.c | 8 ++++- src/util.c | 26 +++++++++++++++- tests/controller.c | 12 ++++++++ tests/game.c | 41 +++++++++++++++++++++++++ tests/json_helpers.c | 71 ++++++++++++++++++++++++++++++++++++++++++++ tests/tilemap.c | 54 +++++++++++++++++++++++++++++++++ tests/util.c | 67 +++++++++++++++++++++++++++++++++++++++++ 12 files changed, 309 insertions(+), 15 deletions(-) diff --git a/include/akgl/game.h b/include/akgl/game.h index c694234..c0aa210 100644 --- a/include/akgl/game.h +++ b/include/akgl/game.h @@ -92,7 +92,7 @@ typedef struct { SDL_Time lastIterTime; /**< Timestamp of the most recent akgl_game_update_fps call. */ SDL_Time lastFPSTime; /**< When `fps` was last recomputed. */ int16_t framesSinceUpdate; /**< Frames counted so far in the current second. */ - void (*lowfpsfunc)(void); /**< Called every frame while `fps` is under 30. Defaults to akgl_game_lowfps; replace it to do something more useful than log. */ + void (*lowfpsfunc)(void); /**< Called every frame while `fps` is under 30. akgl_game_init installs akgl_game_lowfps, and akgl_game_update_fps installs it too if it finds this NULL -- an embedder that binds its own renderer rather than calling akgl_game_init used to crash here on frame one. Replace it to do something more useful than log. */ } akgl_Game; /** @brief The SDL window, created by akgl_render_2d_init. `NULL` until then. */ diff --git a/include/akgl/util.h b/include/akgl/util.h index d11ff68..22e85d8 100644 --- a/include/akgl/util.h +++ b/include/akgl/util.h @@ -142,12 +142,15 @@ akerr_ErrorContext AKERR_NOIGNORE *akgl_path_relative(char *root, char *path, ak * @param s2 Second surface. Required. * @return `NULL` when the pixels match, otherwise an error context owned by the * caller. "Not equal" is reported as an error, not as an out-param. - * @throws AKERR_NULLPOINTER If @p s1 or @p s2 is `NULL`. - * @throws AKERR_VALUE If the pixels differ. + * @throws AKERR_NULLPOINTER If @p s1 or @p s2 is `NULL`, or either has no + * pixel buffer. + * @throws AKERR_VALUE If the surfaces differ in size, pitch, or pixel format, + * or if their pixels differ. * - * @warning The surfaces' dimensions, pitch, and format are not compared, so a - * smaller @p s2 is read past its end rather than reported as a - * mismatch. TODO.md, "Known and still open" item 5. + * Dimensions, pitch and pixel format are compared first, and a difference in + * any of them is reported as a mismatch. Until 0.5.0 they were not, so a + * smaller @p s2 was read past its end instead -- benign in practice and + * immediately fatal under a memory checker. */ akerr_ErrorContext AKERR_NOIGNORE *akgl_compare_sdl_surfaces(SDL_Surface *s1, SDL_Surface *s2); /** @@ -177,9 +180,9 @@ akerr_ErrorContext AKERR_NOIGNORE *akgl_compare_sdl_surfaces(SDL_Surface *s1, SD * @throws AKERR_VALUE If the two renders differ. * @throws AKGL_ERR_HEAP If the string pool is exhausted. * - * @warning Known defect: both passes draw @p t1 -- @p t2 is never rendered -- so - * this currently always reports a match. TODO.md, "Known and still - * open" item 1. + * @note Until 0.5.0 both passes drew @p t1, so this always reported a match and + * every image assertion built on it asserted nothing. It draws @p t2 on + * the second pass now. */ akerr_ErrorContext AKERR_NOIGNORE *akgl_render_and_compare(SDL_Texture *t1, SDL_Texture *t2, int x, int y, int w, int h, char *writeout); diff --git a/src/controller.c b/src/controller.c index 5418903..3fef438 100644 --- a/src/controller.c +++ b/src/controller.c @@ -440,7 +440,7 @@ akerr_ErrorContext *akgl_controller_pushmap(int controlmapid, akgl_Control *cont PREPARE_ERROR(errctx); ATTEMPT { FAIL_ZERO_BREAK(errctx, control, AKERR_NULLPOINTER, "NULL Control"); - FAIL_NONZERO_BREAK(errctx, (controlmapid >= AKGL_MAX_CONTROL_MAPS), AKERR_OUTOFBOUNDS, "ID %d exceeds maximum %d", controlmapid, AKGL_MAX_CONTROL_MAPS); + FAIL_NONZERO_BREAK(errctx, ((controlmapid < 0) || (controlmapid >= AKGL_MAX_CONTROL_MAPS)), AKERR_OUTOFBOUNDS, "Control map id %d is outside 0..%d", controlmapid, (AKGL_MAX_CONTROL_MAPS - 1)); newmapid = akgl_controlmaps[controlmapid].nextMap; FAIL_ZERO_BREAK(errctx, (AKGL_MAX_CONTROLS - newmapid), AKERR_OUTOFBOUNDS, "Control map ID %d is full", controlmapid); memcpy((void *)&akgl_controlmaps[controlmapid].controls[newmapid], control, sizeof(akgl_Control)); @@ -459,7 +459,7 @@ akerr_ErrorContext *akgl_controller_default(int controlmapid, char *actorname, i PREPARE_ERROR(errctx); ATTEMPT { // set up the control map - FAIL_NONZERO_BREAK(errctx, (controlmapid >= AKGL_MAX_CONTROL_MAPS), AKERR_OUTOFBOUNDS, "ID %d exceeds maximum %d", controlmapid, AKGL_MAX_CONTROL_MAPS); + FAIL_NONZERO_BREAK(errctx, ((controlmapid < 0) || (controlmapid >= AKGL_MAX_CONTROL_MAPS)), AKERR_OUTOFBOUNDS, "Control map id %d is outside 0..%d", controlmapid, (AKGL_MAX_CONTROL_MAPS - 1)); memset((void *)&control, 0x00, sizeof(akgl_Control)); controlmap = &akgl_controlmaps[controlmapid]; controlmap->kbid = kbid; diff --git a/src/game.c b/src/game.c index c0c7158..b47803d 100644 --- a/src/game.c +++ b/src/game.c @@ -254,6 +254,14 @@ void akgl_game_update_fps(void) akgl_game.lastFPSTime = curTime; } if ( akgl_game.fps < 30 ) { + // Install the default if nobody has. Only akgl_game_init sets this, and + // renderer.h documents the other path on purpose -- a host that owns + // its own window calls akgl_render_2d_bind instead. Such an embedder + // used to crash on frame one, because fps is 0 for the first second and + // 0 is under the threshold. + if ( akgl_game.lowfpsfunc == NULL ) { + akgl_game.lowfpsfunc = &akgl_game_lowfps; + } akgl_game.lowfpsfunc(); } akgl_game.framesSinceUpdate += 1; diff --git a/src/json_helpers.c b/src/json_helpers.c index 9628c96..af47d18 100644 --- a/src/json_helpers.c +++ b/src/json_helpers.c @@ -94,9 +94,13 @@ akerr_ErrorContext *akgl_get_json_string_value(json_t *obj, char *key, akgl_Stri CATCH(errctx, akgl_heap_next_string(dest)); CATCH(errctx, akgl_string_initialize(*dest, NULL)); } + // FINISH(errctx, true), not false. With `false` a failed + // akgl_heap_next_string was swallowed and the strncpy below ran through + // the pointer it never set, so an exhausted string pool arrived as a + // segfault inside strncpy rather than as AKGL_ERR_HEAP. } CLEANUP { } PROCESS(errctx) { - } FINISH(errctx, false); + } FINISH(errctx, true); strncpy((char *)&(*dest)->data, json_string_value(value), AKGL_MAX_STRING_LENGTH); SUCCEED_RETURN(errctx); @@ -152,9 +156,13 @@ akerr_ErrorContext *akgl_get_json_array_index_string(json_t *array, int index, a CATCH(errctx, akgl_heap_next_string(dest)); CATCH(errctx, akgl_string_initialize(*dest, NULL)); } + // FINISH(errctx, true), not false. With `false` a failed + // akgl_heap_next_string was swallowed and the strncpy below ran through + // the pointer it never set, so an exhausted string pool arrived as a + // segfault inside strncpy rather than as AKGL_ERR_HEAP. } CLEANUP { } PROCESS(errctx) { - } FINISH(errctx, false); + } FINISH(errctx, true); strncpy((char *)&(*dest)->data, json_string_value(value), AKGL_MAX_STRING_LENGTH); SUCCEED_RETURN(errctx); diff --git a/src/tilemap.c b/src/tilemap.c index be48f85..e0af3e2 100644 --- a/src/tilemap.c +++ b/src/tilemap.c @@ -828,14 +828,20 @@ akerr_ErrorContext *akgl_tilemap_release(akgl_Tilemap *dest) PREPARE_ERROR(errctx); FAIL_ZERO_RETURN(errctx, dest, AKERR_NULLPOINTER, "NULL map"); int i = 0; + // Each pointer is cleared as it goes. Without that a second release + // destroys textures SDL has already freed, and the second loop used to + // destroy `tilesets[i]` while testing `layers[i]` -- so every tileset + // texture was freed twice and no image layer's texture was freed at all. for ( i = 0; i < AKGL_TILEMAP_MAX_TILESETS; i++ ) { if ( dest->tilesets[i].texture != NULL ) { SDL_DestroyTexture(dest->tilesets[i].texture); + dest->tilesets[i].texture = NULL; } } for ( i = 0; i < AKGL_TILEMAP_MAX_LAYERS; i++ ) { if ( dest->layers[i].texture != NULL ) { - SDL_DestroyTexture(dest->tilesets[i].texture); + SDL_DestroyTexture(dest->layers[i].texture); + dest->layers[i].texture = NULL; } } SUCCEED_RETURN(errctx); diff --git a/src/util.c b/src/util.c index aa32564..c63ee99 100644 --- a/src/util.c +++ b/src/util.c @@ -224,6 +224,30 @@ akerr_ErrorContext *akgl_compare_sdl_surfaces(SDL_Surface *s1, SDL_Surface *s2) PREPARE_ERROR(errctx); FAIL_ZERO_RETURN(errctx, s1, AKERR_NULLPOINTER, "NULL Surface pointer"); FAIL_ZERO_RETURN(errctx, s2, AKERR_NULLPOINTER, "NULL Surface pointer"); + // Geometry first. The memcmp reads s1->pitch * s1->h bytes out of *both*, + // so a smaller s2 was read past its end rather than reported as a + // mismatch -- and a differing pitch or format made the comparison + // meaningless even when it did not run off. + FAIL_NONZERO_RETURN( + errctx, + ((s1->w != s2->w) || (s1->h != s2->h)), + AKERR_VALUE, + "Comparison surfaces differ in size: %dx%d against %dx%d", + s1->w, s1->h, s2->w, s2->h); + FAIL_NONZERO_RETURN( + errctx, + (s1->pitch != s2->pitch), + AKERR_VALUE, + "Comparison surfaces differ in pitch: %d against %d", + s1->pitch, s2->pitch); + FAIL_NONZERO_RETURN( + errctx, + (s1->format != s2->format), + AKERR_VALUE, + "Comparison surfaces differ in pixel format: %s against %s", + SDL_GetPixelFormatName(s1->format), SDL_GetPixelFormatName(s2->format)); + FAIL_ZERO_RETURN(errctx, s1->pixels, AKERR_NULLPOINTER, "First surface has no pixels"); + FAIL_ZERO_RETURN(errctx, s2->pixels, AKERR_NULLPOINTER, "Second surface has no pixels"); FAIL_NONZERO_RETURN(errctx, memcmp(s1->pixels, s2->pixels, (s1->pitch * s1->h)), AKERR_VALUE, "Comparison surfaces are not equal"); SUCCEED_RETURN(errctx); } @@ -261,7 +285,7 @@ akerr_ErrorContext *akgl_render_and_compare(SDL_Texture *t1, SDL_Texture *t2, in SDL_RenderClear(akgl_renderer->sdl_renderer); - CATCH(errctx, akgl_renderer->draw_texture(akgl_renderer, t1, &src, &dest, 0, NULL, SDL_FLIP_NONE)); + CATCH(errctx, akgl_renderer->draw_texture(akgl_renderer, t2, &src, &dest, 0, NULL, SDL_FLIP_NONE)); s2 = SDL_RenderReadPixels(akgl_renderer->sdl_renderer, &read); FAIL_ZERO_BREAK(errctx, s2, AKGL_ERR_SDL, "Failed to read pixels from renderer"); diff --git a/tests/controller.c b/tests/controller.c index 047ad4f..b787472 100644 --- a/tests/controller.c +++ b/tests/controller.c @@ -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) { diff --git a/tests/game.c b/tests/game.c index 4832e1b..60a6003 100644 --- a/tests/game.c +++ b/tests/game.c @@ -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); diff --git a/tests/json_helpers.c b/tests/json_helpers.c index 8836fc5..9f35a4b 100644 --- a/tests/json_helpers.c +++ b/tests/json_helpers.c @@ -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); diff --git a/tests/tilemap.c b/tests/tilemap.c index a0ea1a2..fdb39d8 100644 --- a/tests/tilemap.c +++ b/tests/tilemap.c @@ -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()); diff --git a/tests/util.c b/tests/util.c index f16bf4a..7203295 100644 --- a/tests/util.c +++ b/tests/util.c @@ -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) {