diff --git a/CMakeLists.txt b/CMakeLists.txt index 3a81fba..a56664a 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -316,6 +316,23 @@ add_test( ) set_tests_properties(api_surface PROPERTIES SKIP_RETURN_CODE 2) +# The two akerror control-flow rules whose failure mode is silent: a *_RETURN +# inside an ATTEMPT block, and a return out of a HANDLE block. Neither produces +# a compiler diagnostic and neither fails a test run until the pool it drains is +# empty, which is why both have already shipped. +# +# The mutation target below wants the same interpreter; find_package caches, so +# asking here as well costs nothing and keeps this block self-contained. +find_package(Python3 COMPONENTS Interpreter) +if(Python3_Interpreter_FOUND) + add_test( + NAME error_protocol + COMMAND ${Python3_EXECUTABLE} + ${CMAKE_CURRENT_SOURCE_DIR}/scripts/check_error_protocol.py + ${CMAKE_CURRENT_SOURCE_DIR}/src + ) +endif() + # One translation unit per public header, each including exactly that header and # nothing before it. This is the only shape that proves a header is # self-contained: a second #include in the same file proves nothing about the diff --git a/scripts/check_error_protocol.py b/scripts/check_error_protocol.py new file mode 100755 index 0000000..16e7f43 --- /dev/null +++ b/scripts/check_error_protocol.py @@ -0,0 +1,145 @@ +#!/usr/bin/env python3 +"""Enforce the two akerror control-flow rules whose failure mode is silent. + +AGENTS.md states both, and both have already cost this library a leak: + + 1. Never use a ``*_RETURN`` macro inside an ``ATTEMPT`` block. It returns past + ``CLEANUP``, so every release, ``fclose`` and free in ``CLEANUP`` is + skipped. ``akgl_get_json_tilemap_property`` leaked two pooled strings on + every *successful* lookup this way. + + 2. Never ``return`` from inside a ``HANDLE`` block. ``FINISH`` is what carries + ``RELEASE_ERROR``, so leaving early never gives the handled context back to + ``AKERR_ARRAY_ERROR``. That cost ``akgl_path_relative`` one slot per call + and killed the process on the 129th. + +Neither produces a compiler diagnostic, and neither shows up in a passing test +run until the pool it drains is empty. Hence a linter. + +Usage: check_error_protocol.py [...] + +Exits 0 when clean, 1 on a violation. +""" + +import os +import re +import sys + +# A ``*_RETURN`` macro invocation: SUCCEED_RETURN, FAIL_RETURN, FAIL_ZERO_RETURN, +# FAIL_NONZERO_RETURN, and anything later shaped like them. +RETURN_MACRO = re.compile(r"\b[A-Z][A-Z0-9_]*_RETURN\s*\(") +BARE_RETURN = re.compile(r"\breturn\b") +ATTEMPT_OPEN = re.compile(r"\bATTEMPT\s*\{") +CLEANUP_OPEN = re.compile(r"\}\s*CLEANUP\s*\{") +HANDLE_OPEN = re.compile(r"\}\s*HANDLE(?:_GROUP)?\s*\(") +FINISH_ANY = re.compile(r"\}\s*FINISH(?:_NORETURN)?\s*\(") + + +def strip_comments(text): + """Blank out comments, keeping line numbering intact. + + Comments are removed rather than skipped because several of these keywords + appear in prose -- "which should never return AKERR_KEY" is a sentence, not + a violation. + """ + out = [] + i = 0 + n = len(text) + while i < n: + ch = text[i] + if ch == '"' or ch == "'": + quote = ch + out.append(ch) + i += 1 + while i < n: + if text[i] == "\\" and (i + 1) < n: + out.append(" ") + i += 2 + continue + out.append(text[i]) + if text[i] == quote: + i += 1 + break + i += 1 + continue + if text.startswith("/*", i): + j = text.find("*/", i + 2) + j = n if j == -1 else (j + 2) + out.append("".join(c if c == "\n" else " " for c in text[i:j])) + i = j + continue + if text.startswith("//", i): + j = text.find("\n", i) + j = n if j == -1 else j + out.append(" " * (j - i)) + i = j + continue + out.append(ch) + i += 1 + return "".join(out) + + +def check(path): + findings = [] + lines = strip_comments(open(path, encoding="utf-8", errors="replace").read()).split("\n") + in_attempt = False + in_handle = False + + for number, line in enumerate(lines, 1): + if ATTEMPT_OPEN.search(line): + in_attempt = True + continue + if CLEANUP_OPEN.search(line): + in_attempt = False + if HANDLE_OPEN.search(line): + in_handle = True + continue + if FINISH_ANY.search(line): + in_handle = False + continue + + if in_attempt and RETURN_MACRO.search(line): + findings.append( + (number, "a *_RETURN macro inside an ATTEMPT block returns past CLEANUP; " + "use the matching *_BREAK variant")) + if in_handle and BARE_RETURN.search(line): + findings.append( + (number, "returning from inside a HANDLE block skips the RELEASE_ERROR that " + "FINISH carries; set a flag and act on it after FINISH")) + return findings + + +def sources(targets): + for target in targets: + if os.path.isdir(target): + for root, _dirs, files in os.walk(target): + for name in sorted(files): + if name.endswith(".c") and not name.endswith("~"): + yield os.path.join(root, name) + else: + yield target + + +def main(argv): + if len(argv) < 2: + print(__doc__, file=sys.stderr) + return 1 + + total = 0 + for path in sources(argv[1:]): + for number, message in check(path): + print("%s:%d: %s" % (path, number, message), file=sys.stderr) + total += 1 + + if total: + print("", file=sys.stderr) + print("check_error_protocol: %d violation(s). See the Error-Handling Protocol " + "section of AGENTS.md." % total, file=sys.stderr) + return 1 + + print("check_error_protocol: clean") + return 0 + + +if __name__ == "__main__": + sys.exit(main(sys.argv)) diff --git a/src/controller.c b/src/controller.c index 7e4919c..ddd4434 100644 --- a/src/controller.c +++ b/src/controller.c @@ -446,10 +446,10 @@ akerr_ErrorContext *akgl_controller_pushmap(int controlmapid, akgl_Control *cont int newmapid = 0; PREPARE_ERROR(errctx); ATTEMPT { - FAIL_ZERO_RETURN(errctx, control, AKERR_NULLPOINTER, "NULL Control"); - FAIL_NONZERO_RETURN(errctx, (controlmapid >= AKGL_MAX_CONTROL_MAPS), AKERR_OUTOFBOUNDS, "ID %d exceeds maximum %d", controlmapid, AKGL_MAX_CONTROL_MAPS); + 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); newmapid = akgl_controlmaps[controlmapid].nextMap; - FAIL_ZERO_RETURN(errctx, (AKGL_MAX_CONTROLS - newmapid), AKERR_OUTOFBOUNDS, "Control map ID %d is full", controlmapid); + 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)); akgl_controlmaps[controlmapid].nextMap = newmapid + 1; } CLEANUP { @@ -466,7 +466,7 @@ akerr_ErrorContext *akgl_controller_default(int controlmapid, char *actorname, i PREPARE_ERROR(errctx); ATTEMPT { // set up the control map - FAIL_NONZERO_RETURN(errctx, (controlmapid >= AKGL_MAX_CONTROL_MAPS), AKERR_OUTOFBOUNDS, "ID %d exceeds maximum %d", controlmapid, AKGL_MAX_CONTROL_MAPS); + FAIL_NONZERO_BREAK(errctx, (controlmapid >= AKGL_MAX_CONTROL_MAPS), AKERR_OUTOFBOUNDS, "ID %d exceeds maximum %d", controlmapid, AKGL_MAX_CONTROL_MAPS); memset((void *)&control, 0x00, sizeof(akgl_Control)); controlmap = &akgl_controlmaps[controlmapid]; controlmap->kbid = kbid; @@ -542,12 +542,13 @@ akerr_ErrorContext *akgl_controller_default(int controlmapid, char *actorname, i control.handler_on = &akgl_actor_cmhf_right_on; control.handler_off = &akgl_actor_cmhf_right_off; CATCH(errctx, akgl_controller_pushmap(controlmapid, &control)); - - SUCCEED_RETURN(errctx); - } CLEANUP { } PROCESS(errctx) { } FINISH(errctx, true); + // The SUCCEED_RETURN used to sit at the end of the ATTEMPT block, which + // returned past CLEANUP and left this function with no return statement at + // all on the path that falls out of FINISH. + SUCCEED_RETURN(errctx); } akerr_ErrorContext *akgl_controller_poll_key(int *keycode, bool *available) diff --git a/src/game.c b/src/game.c index a58d8ee..c7684ff 100644 --- a/src/game.c +++ b/src/game.c @@ -600,12 +600,12 @@ akerr_ErrorContext *akgl_game_load(char *fpath) CATCH(e, read_exact((void *)&savegame, 1, sizeof(akgl_Game), fp)); CATCH(e, akgl_game_load_versioncmp("library", (char *)&savegame.libversion, (char *)AKGL_VERSION)); CATCH(e, akgl_game_load_versioncmp("game", (char *)&savegame.version, (char *)&akgl_game.version)); - FAIL_NONZERO_RETURN( + FAIL_NONZERO_BREAK( e, strncmp((char *)&savegame.name, (char *)&akgl_game.name, 256), AKERR_API, "Savegame is not compatible with this game"); - FAIL_NONZERO_RETURN( + FAIL_NONZERO_BREAK( e, strncmp((char *)&savegame.uri, (char *)&akgl_game.uri, 256), AKERR_API, diff --git a/src/json_helpers.c b/src/json_helpers.c index 75f6f89..cb8a442 100644 --- a/src/json_helpers.c +++ b/src/json_helpers.c @@ -17,6 +17,8 @@ akerr_ErrorContext *akgl_get_json_object_value(json_t *obj, char *key, json_t ** { PREPARE_ERROR(errctx); FAIL_ZERO_RETURN(errctx, obj, AKERR_NULLPOINTER, "NULL object pointer"); + FAIL_ZERO_RETURN(errctx, key, AKERR_NULLPOINTER, "NULL key string"); + FAIL_ZERO_RETURN(errctx, dest, AKERR_NULLPOINTER, "NULL destination pointer"); json_t *value = json_object_get(obj, key); FAIL_ZERO_RETURN(errctx, value, AKERR_KEY, "Key %s not found in object", key); FAIL_ZERO_RETURN(errctx, (json_is_object(value)), AKERR_TYPE, "Key %s in object has incorrect type", key); @@ -28,6 +30,8 @@ akerr_ErrorContext *akgl_get_json_boolean_value(json_t *obj, char *key, bool *de { PREPARE_ERROR(errctx); FAIL_ZERO_RETURN(errctx, obj, AKERR_NULLPOINTER, "NULL object pointer"); + FAIL_ZERO_RETURN(errctx, key, AKERR_NULLPOINTER, "NULL key string"); + FAIL_ZERO_RETURN(errctx, dest, AKERR_NULLPOINTER, "NULL destination pointer"); json_t *value = json_object_get(obj, key); FAIL_ZERO_RETURN(errctx, value, AKERR_KEY, "Key %s not found in object", key); FAIL_ZERO_RETURN(errctx, (json_is_boolean(value)), AKERR_TYPE, "Key %s in object has incorrect type", key); @@ -39,6 +43,8 @@ akerr_ErrorContext *akgl_get_json_integer_value(json_t *obj, char *key, int *des { PREPARE_ERROR(errctx); FAIL_ZERO_RETURN(errctx, obj, AKERR_NULLPOINTER, "NULL object pointer"); + FAIL_ZERO_RETURN(errctx, key, AKERR_NULLPOINTER, "NULL key string"); + FAIL_ZERO_RETURN(errctx, dest, AKERR_NULLPOINTER, "NULL destination pointer"); json_t *value = json_object_get(obj, key); FAIL_ZERO_RETURN(errctx, value, AKERR_KEY, "Key %s not found in object", key); FAIL_ZERO_RETURN(errctx, (json_is_integer(value)), AKERR_TYPE, "Key %s in object has incorrect type", key); @@ -50,6 +56,8 @@ akerr_ErrorContext *akgl_get_json_number_value(json_t *obj, char *key, float *de { PREPARE_ERROR(errctx); FAIL_ZERO_RETURN(errctx, obj, AKERR_NULLPOINTER, "NULL pointer reference"); + FAIL_ZERO_RETURN(errctx, key, AKERR_NULLPOINTER, "NULL key string"); + FAIL_ZERO_RETURN(errctx, dest, AKERR_NULLPOINTER, "NULL destination pointer"); json_t *value = json_object_get(obj, key); FAIL_ZERO_RETURN(errctx, value, AKERR_KEY, "Key %s not found in object", key); FAIL_ZERO_RETURN(errctx, (json_is_number(value)), AKERR_TYPE, "Key %s in object has incorrect type", key); @@ -61,6 +69,8 @@ akerr_ErrorContext *akgl_get_json_double_value(json_t *obj, char *key, double *d { PREPARE_ERROR(errctx); FAIL_ZERO_RETURN(errctx, obj, AKERR_NULLPOINTER, "NULL pointer reference"); + FAIL_ZERO_RETURN(errctx, key, AKERR_NULLPOINTER, "NULL key string"); + FAIL_ZERO_RETURN(errctx, dest, AKERR_NULLPOINTER, "NULL destination pointer"); json_t *value = json_object_get(obj, key); FAIL_ZERO_RETURN(errctx, value, AKERR_KEY, "Key %s not found in object", key); FAIL_ZERO_RETURN(errctx, (json_is_number(value)), AKERR_TYPE, "Key %s in object has incorrect type", key); @@ -73,8 +83,8 @@ akerr_ErrorContext *akgl_get_json_string_value(json_t *obj, char *key, akgl_Stri json_t *value = NULL; PREPARE_ERROR(errctx); FAIL_ZERO_RETURN(errctx, obj, AKERR_NULLPOINTER, "NULL pointer reference"); - FAIL_ZERO_RETURN(errctx, key, AKERR_NULLPOINTER, "NULL pointer reference"); - FAIL_ZERO_RETURN(errctx, dest, AKERR_NULLPOINTER, "NULL pointer reference"); + FAIL_ZERO_RETURN(errctx, key, AKERR_NULLPOINTER, "NULL key string"); + FAIL_ZERO_RETURN(errctx, dest, AKERR_NULLPOINTER, "NULL destination pointer"); value = json_object_get(obj, key); FAIL_ZERO_RETURN(errctx, value, AKERR_KEY, "Key %s not found in object", key); @@ -96,6 +106,8 @@ akerr_ErrorContext *akgl_get_json_array_value(json_t *obj, char *key, json_t **d { PREPARE_ERROR(errctx); FAIL_ZERO_RETURN(errctx, obj, AKERR_NULLPOINTER, "NULL pointer reference"); + FAIL_ZERO_RETURN(errctx, key, AKERR_NULLPOINTER, "NULL key string"); + FAIL_ZERO_RETURN(errctx, dest, AKERR_NULLPOINTER, "NULL destination pointer"); json_t *value = json_object_get(obj, key); FAIL_ZERO_RETURN(errctx, value, AKERR_KEY, "Key %s not found in object", key); FAIL_ZERO_RETURN(errctx, (json_is_array(value)), AKERR_TYPE, "Key %s in object has incorrect type", key); @@ -107,6 +119,7 @@ akerr_ErrorContext *akgl_get_json_array_index_object(json_t *array, int index, j { PREPARE_ERROR(errctx); FAIL_ZERO_RETURN(errctx, array, AKERR_NULLPOINTER, "NULL pointer reference"); + FAIL_ZERO_RETURN(errctx, dest, AKERR_NULLPOINTER, "NULL destination pointer"); json_t *value = json_array_get(array, index); FAIL_ZERO_RETURN(errctx, value, AKERR_OUTOFBOUNDS, "Index %d out of bounds for array", index); FAIL_ZERO_RETURN(errctx, (json_is_object(value)), AKERR_TYPE, "Index %d in object has incorrect type", index); @@ -118,6 +131,7 @@ akerr_ErrorContext *akgl_get_json_array_index_integer(json_t *array, int index, { PREPARE_ERROR(errctx); FAIL_ZERO_RETURN(errctx, array, AKERR_NULLPOINTER, "NULL pointer reference"); + FAIL_ZERO_RETURN(errctx, dest, AKERR_NULLPOINTER, "NULL destination pointer"); json_t *value = json_array_get(array, index); FAIL_ZERO_RETURN(errctx, value, AKERR_OUTOFBOUNDS, "Index %d out of bounds for array", index); FAIL_ZERO_RETURN(errctx, (json_is_integer(value)), AKERR_TYPE, "Index %d in object has incorrect type", index); @@ -129,7 +143,7 @@ akerr_ErrorContext *akgl_get_json_array_index_string(json_t *array, int index, a { PREPARE_ERROR(errctx); FAIL_ZERO_RETURN(errctx, array, AKERR_NULLPOINTER, "NULL pointer reference"); - FAIL_ZERO_RETURN(errctx, dest, AKERR_NULLPOINTER, "NULL destination pointer reference"); + FAIL_ZERO_RETURN(errctx, dest, AKERR_NULLPOINTER, "NULL destination pointer"); json_t *value = json_array_get(array, index); FAIL_ZERO_RETURN(errctx, value, AKERR_OUTOFBOUNDS, "Index %d out of bounds for array", index); FAIL_ZERO_RETURN(errctx, (json_is_string(value)), AKERR_TYPE, "Index %d in object has incorrect type", index); diff --git a/src/physics.c b/src/physics.c index 79b472d..fa5abd1 100644 --- a/src/physics.c +++ b/src/physics.c @@ -16,6 +16,7 @@ akerr_ErrorContext *akgl_physics_null_gravity(akgl_PhysicsBackend *self, akgl_Ac { PREPARE_ERROR(e); FAIL_ZERO_RETURN(e, self, AKERR_NULLPOINTER, "self"); + FAIL_ZERO_RETURN(e, actor, AKERR_NULLPOINTER, "actor"); SUCCEED_RETURN(e); } @@ -23,13 +24,16 @@ akerr_ErrorContext *akgl_physics_null_collide(akgl_PhysicsBackend *self, akgl_Ac { PREPARE_ERROR(e); FAIL_ZERO_RETURN(e, self, AKERR_NULLPOINTER, "self"); + FAIL_ZERO_RETURN(e, a1, AKERR_NULLPOINTER, "a1"); + FAIL_ZERO_RETURN(e, a2, AKERR_NULLPOINTER, "a2"); SUCCEED_RETURN(e); } -akerr_ErrorContext *akgl_physics_null_move(struct akgl_PhysicsBackend *self, akgl_Actor *actor, float32_t dt) +akerr_ErrorContext *akgl_physics_null_move(akgl_PhysicsBackend *self, akgl_Actor *actor, float32_t dt) { PREPARE_ERROR(e); FAIL_ZERO_RETURN(e, self, AKERR_NULLPOINTER, "self"); + FAIL_ZERO_RETURN(e, actor, AKERR_NULLPOINTER, "actor"); SUCCEED_RETURN(e); } @@ -77,7 +81,7 @@ akerr_ErrorContext *akgl_physics_arcade_collide(akgl_PhysicsBackend *self, akgl_ SUCCEED_RETURN(e); } -akerr_ErrorContext *akgl_physics_arcade_move(struct akgl_PhysicsBackend *self, akgl_Actor *actor, float32_t dt) +akerr_ErrorContext *akgl_physics_arcade_move(akgl_PhysicsBackend *self, akgl_Actor *actor, float32_t dt) { PREPARE_ERROR(e); FAIL_ZERO_RETURN(e, self, AKERR_NULLPOINTER, "self"); diff --git a/src/renderer.c b/src/renderer.c index b3ec671..a65878c 100644 --- a/src/renderer.c +++ b/src/renderer.c @@ -68,12 +68,14 @@ akerr_ErrorContext *akgl_render_2d_bind(akgl_RenderBackend *self) akerr_ErrorContext *akgl_render_2d_shutdown(akgl_RenderBackend *self) { PREPARE_ERROR(e); + FAIL_ZERO_RETURN(e, self, AKERR_NULLPOINTER, "self"); SUCCEED_RETURN(e); } akerr_ErrorContext *akgl_render_2d_frame_start(akgl_RenderBackend *self) { PREPARE_ERROR(e); + FAIL_ZERO_RETURN(e, self, AKERR_NULLPOINTER, "self"); FAIL_ZERO_RETURN(e, self->sdl_renderer, AKERR_NULLPOINTER, "No valid SDL rendering backend"); SDL_SetRenderDrawColor(self->sdl_renderer, 0, 0, 0, 255); SDL_RenderClear(self->sdl_renderer); @@ -83,6 +85,7 @@ akerr_ErrorContext *akgl_render_2d_frame_start(akgl_RenderBackend *self) akerr_ErrorContext *akgl_render_2d_frame_end(akgl_RenderBackend *self) { PREPARE_ERROR(e); + FAIL_ZERO_RETURN(e, self, AKERR_NULLPOINTER, "self"); FAIL_ZERO_RETURN(e, self->sdl_renderer, AKERR_NULLPOINTER, "No valid SDL rendering backend"); SDL_RenderPresent(self->sdl_renderer); SUCCEED_RETURN(e); diff --git a/src/tilemap.c b/src/tilemap.c index 2a69b2f..24fd886 100644 --- a/src/tilemap.c +++ b/src/tilemap.c @@ -30,6 +30,7 @@ akerr_ErrorContext *akgl_get_json_tilemap_property(json_t *obj, char *key, char json_t *property = NULL; akgl_String *tmpstr = NULL; akgl_String *typestr = NULL; + bool found = false; int i = 0; // This is not a generic JSON helper. It assumes we are receiving an object with a 'properties' key // inside of it. That key is an array of objects, and each object has a name, type, and value. @@ -39,9 +40,13 @@ akerr_ErrorContext *akgl_get_json_tilemap_property(json_t *obj, char *key, char CATCH(errctx, akgl_get_json_array_value(obj, "properties", &properties)); for (i = 0; i < json_array_size(properties); i++) { CATCH(errctx, akgl_get_json_array_index_object(properties, i, &property)); + // One scratch string for the whole scan. akgl_get_json_string_value + // reuses a non-NULL *dest without taking another reference, so + // releasing it per iteration -- which this used to do -- dropped the + // refcount to zero while the slot was still in use, and the next + // claim anywhere could have been handed the same one. CATCH(errctx, akgl_get_json_string_value(property, "name", &tmpstr)); if ( strcmp(tmpstr->data, key) != 0 ) { - CATCH(errctx, akgl_heap_release_string(tmpstr)); continue; } CATCH(errctx, akgl_get_json_string_value(property, "type", &typestr)); @@ -49,7 +54,8 @@ akerr_ErrorContext *akgl_get_json_tilemap_property(json_t *obj, char *key, char FAIL_BREAK(errctx, AKERR_TYPE, "Property %s is present but is incorrect type(expected %s got %s)", key, type, (char *)typestr->data); } *dest = property; - SUCCEED_RETURN(errctx); + found = true; + break; } } CLEANUP { if ( tmpstr != NULL ) { @@ -61,6 +67,13 @@ akerr_ErrorContext *akgl_get_json_tilemap_property(json_t *obj, char *key, char } PROCESS(errctx) { } FINISH(errctx, true); + // The success path used to be a SUCCEED_RETURN from inside the ATTEMPT + // block, which returned past CLEANUP and leaked both scratch strings on + // every successful lookup. The pool is 256 entries and a map load does this + // many times per layer. + if ( found == true ) { + SUCCEED_RETURN(errctx); + } FAIL_RETURN(errctx, AKERR_KEY, "Property not found in properties map"); } @@ -567,7 +580,7 @@ akerr_ErrorContext *akgl_tilemap_load(char *fname, akgl_Tilemap *dest) dest->orientation = 0; if ( (dest->width * dest->height) >= (AKGL_TILEMAP_MAX_WIDTH * AKGL_TILEMAP_MAX_HEIGHT) ) { - FAIL_RETURN(errctx, AKERR_OUTOFBOUNDS, "Map exceeds the maximum size"); + FAIL_BREAK(errctx, AKERR_OUTOFBOUNDS, "Map exceeds the maximum size"); } CATCH(errctx, akgl_tilemap_load_layers((akgl_Tilemap *)dest, (json_t *)json, dirnamestr)); diff --git a/src/util.c b/src/util.c index ad06506..b882236 100644 --- a/src/util.c +++ b/src/util.c @@ -73,7 +73,7 @@ static akerr_ErrorContext *path_relative_root(char *root, char *path, akgl_Strin rootlen = strlen(root); pathlen = strlen(path); if ( (rootlen + pathlen) >= AKGL_MAX_STRING_LENGTH ) { - FAIL_RETURN(e, AKERR_OUTOFBOUNDS, "Total path length (%d) is greater than maximum akgl_String length (%d)", (rootlen + pathlen), AKGL_MAX_STRING_LENGTH); + FAIL_BREAK(e, AKERR_OUTOFBOUNDS, "Total path length (%d) is greater than maximum akgl_String length (%d)", (rootlen + pathlen), AKGL_MAX_STRING_LENGTH); } DISABLE_GCC_WARNING_FORMAT_TRUNCATION CATCH(e, aksl_snprintf( @@ -180,41 +180,43 @@ akerr_ErrorContext *akgl_collide_rectangles(SDL_FRect *r1, SDL_FRect *r2, bool * // is the upper left corner of r1 contacting r2? CATCH(errctx, akgl_collide_point_rectangle(&r1p.topleft, &r2p, collide)); - if ( *collide == true ) { SUCCEED_RETURN(errctx); } + if ( *collide == true ) { break; } // is the upper left corner of r2 contacting r1? CATCH(errctx, akgl_collide_point_rectangle(&r2p.topleft, &r1p, collide)); - if ( *collide == true ) { SUCCEED_RETURN(errctx); } + if ( *collide == true ) { break; } // is the top right corner of r1 contacting r2? CATCH(errctx, akgl_collide_point_rectangle(&r1p.topright, &r2p, collide)); - if ( *collide == true ) { SUCCEED_RETURN(errctx); } + if ( *collide == true ) { break; } // is the top right corner of r2 contacting r1? CATCH(errctx, akgl_collide_point_rectangle(&r2p.topright, &r1p, collide)); - if ( *collide == true ) { SUCCEED_RETURN(errctx); } + if ( *collide == true ) { break; } // is the bottom left corner of r1 contacting r2? CATCH(errctx, akgl_collide_point_rectangle(&r1p.bottomleft, &r2p, collide)); - if ( *collide == true ) { SUCCEED_RETURN(errctx); } + if ( *collide == true ) { break; } // is the bottom left corner of r2 contacting r1? CATCH(errctx, akgl_collide_point_rectangle(&r2p.bottomleft, &r1p, collide)); - if ( *collide == true ) { SUCCEED_RETURN(errctx); } + if ( *collide == true ) { break; } // is the bottom right corner of r1 contacting r2? CATCH(errctx, akgl_collide_point_rectangle(&r1p.bottomright, &r2p, collide)); - if ( *collide == true ) { SUCCEED_RETURN(errctx); } + if ( *collide == true ) { break; } // is the bottom right corner of r2 contacting r1? CATCH(errctx, akgl_collide_point_rectangle(&r2p.bottomright, &r1p, collide)); - if ( *collide == true ) { SUCCEED_RETURN(errctx); } + if ( *collide == true ) { break; } } CLEANUP { } PROCESS(errctx) { } FINISH(errctx, true); - *collide = false; + // No trailing `*collide = false;` here. Each corner test writes the flag + // itself, so the eighth one leaves it false when nothing hit; assigning + // after FINISH would overwrite the hit that broke out of the block early. SUCCEED_RETURN(errctx); } diff --git a/tests/json_helpers.c b/tests/json_helpers.c index 7080f57..8836fc5 100644 --- a/tests/json_helpers.c +++ b/tests/json_helpers.c @@ -48,6 +48,68 @@ static akerr_ErrorContext *load_fixture(void) SUCCEED_RETURN(e); } +/** + * @brief Every accessor must refuse a NULL key or destination, not dereference it. + * + * Only the two string accessors did. The other eight validated the container + * and then wrote through @p dest unconditionally, so which arguments a caller + * could safely get wrong depended on which typed accessor they happened to + * call -- and the ones that crashed were the ones used most. + */ +akerr_ErrorContext *test_json_accessor_null_arguments(void) +{ + PREPARE_ERROR(e); + int intval = 0; + float floatval = 0; + double dblval = 0; + bool boolval = false; + json_t *jsonval = NULL; + akgl_String *strval = NULL; + + ATTEMPT { + TEST_EXPECT_STATUS(e, AKERR_NULLPOINTER, akgl_get_json_object_value(fixture, NULL, &jsonval), + "object accessor with a NULL key"); + TEST_EXPECT_STATUS(e, AKERR_NULLPOINTER, akgl_get_json_object_value(fixture, "nested", NULL), + "object accessor with a NULL destination"); + TEST_EXPECT_STATUS(e, AKERR_NULLPOINTER, akgl_get_json_boolean_value(fixture, NULL, &boolval), + "boolean accessor with a NULL key"); + TEST_EXPECT_STATUS(e, AKERR_NULLPOINTER, akgl_get_json_boolean_value(fixture, "enabled", NULL), + "boolean accessor with a NULL destination"); + TEST_EXPECT_STATUS(e, AKERR_NULLPOINTER, akgl_get_json_integer_value(fixture, NULL, &intval), + "integer accessor with a NULL key"); + TEST_EXPECT_STATUS(e, AKERR_NULLPOINTER, akgl_get_json_integer_value(fixture, "count", NULL), + "integer accessor with a NULL destination"); + TEST_EXPECT_STATUS(e, AKERR_NULLPOINTER, akgl_get_json_number_value(fixture, NULL, &floatval), + "number accessor with a NULL key"); + TEST_EXPECT_STATUS(e, AKERR_NULLPOINTER, akgl_get_json_number_value(fixture, "ratio", NULL), + "number accessor with a NULL destination"); + TEST_EXPECT_STATUS(e, AKERR_NULLPOINTER, akgl_get_json_double_value(fixture, NULL, &dblval), + "double accessor with a NULL key"); + TEST_EXPECT_STATUS(e, AKERR_NULLPOINTER, akgl_get_json_double_value(fixture, "ratio", NULL), + "double accessor with a NULL destination"); + TEST_EXPECT_STATUS(e, AKERR_NULLPOINTER, akgl_get_json_array_value(fixture, NULL, &jsonval), + "array accessor with a NULL key"); + TEST_EXPECT_STATUS(e, AKERR_NULLPOINTER, akgl_get_json_array_value(fixture, "integers", NULL), + "array accessor with a NULL destination"); + + // The index accessors take no key, but they take a destination. + CATCH(e, akgl_get_json_array_value(fixture, "integers", &jsonval)); + TEST_EXPECT_STATUS(e, AKERR_NULLPOINTER, akgl_get_json_array_index_integer(jsonval, 0, NULL), + "array index integer accessor with a NULL destination"); + TEST_EXPECT_STATUS(e, AKERR_NULLPOINTER, akgl_get_json_array_index_object(jsonval, 0, NULL), + "array index object accessor with a NULL destination"); + TEST_EXPECT_STATUS(e, AKERR_NULLPOINTER, akgl_get_json_array_index_string(jsonval, 0, NULL), + "array index string accessor with a NULL destination"); + + // Nothing above should have claimed a pool string on the way to + // refusing, which is the other half of "refuse rather than proceed". + TEST_ASSERT(e, strval == NULL, "a refused accessor wrote to its destination anyway"); + } CLEANUP { + } PROCESS(e) { + } FINISH(e, true); + SUCCEED_RETURN(e); +} + akerr_ErrorContext *test_json_scalar_accessors(void) { PREPARE_ERROR(e); @@ -353,6 +415,7 @@ int main(void) CATCH(errctx, akgl_registry_init()); CATCH(errctx, load_fixture()); + CATCH(errctx, test_json_accessor_null_arguments()); CATCH(errctx, test_json_scalar_accessors()); CATCH(errctx, test_json_double_value()); CATCH(errctx, test_json_string_accessor()); diff --git a/tests/physics.c b/tests/physics.c index 78d338f..3a944b9 100644 --- a/tests/physics.c +++ b/tests/physics.c @@ -145,6 +145,18 @@ akerr_ErrorContext *test_physics_null_backend_is_inert(void) "null move with NULL self"); TEST_EXPECT_STATUS(e, AKERR_NULLPOINTER, akgl_physics_null_collide(NULL, &actor, &reference), "null collide with NULL self"); + + // The actor arguments too. The arcade backend has always checked these; + // the null one checked only `self`, so which pointers a caller could get + // away with passing depended on which backend it happened to have. + TEST_EXPECT_STATUS(e, AKERR_NULLPOINTER, akgl_physics_null_gravity(&backend, NULL, 1.0f), + "null gravity with a NULL actor"); + TEST_EXPECT_STATUS(e, AKERR_NULLPOINTER, akgl_physics_null_move(&backend, NULL, 1.0f), + "null move with a NULL actor"); + TEST_EXPECT_STATUS(e, AKERR_NULLPOINTER, akgl_physics_null_collide(&backend, NULL, &reference), + "null collide with a NULL first actor"); + TEST_EXPECT_STATUS(e, AKERR_NULLPOINTER, akgl_physics_null_collide(&backend, &actor, NULL), + "null collide with a NULL second actor"); } CLEANUP { } PROCESS(e) { } FINISH(e, true); diff --git a/tests/renderer.c b/tests/renderer.c index a61b628..df62bfc 100644 --- a/tests/renderer.c +++ b/tests/renderer.c @@ -92,6 +92,16 @@ akerr_ErrorContext *test_render_backend_without_a_renderer(void) TEST_EXPECT_STATUS(errctx, AKERR_NULLPOINTER, empty.frame_end(&empty), "ending a frame on a backend with no renderer"); + // And a NULL backend outright. frame_start and frame_end used to read + // self->sdl_renderer with no check on self at all, which is a segfault + // rather than an error, while draw_texture beside them checked it. + TEST_EXPECT_STATUS(errctx, AKERR_NULLPOINTER, akgl_render_2d_frame_start(NULL), + "starting a frame on a NULL backend"); + TEST_EXPECT_STATUS(errctx, AKERR_NULLPOINTER, akgl_render_2d_frame_end(NULL), + "ending a frame on a NULL backend"); + TEST_EXPECT_STATUS(errctx, AKERR_NULLPOINTER, akgl_render_2d_shutdown(NULL), + "shutting down a NULL backend"); + // Shutdown is the one that has nothing to release yet, so it succeeds. TEST_EXPECT_OK(errctx, empty.shutdown(&empty), "shutting down an unused backend"); } CLEANUP { diff --git a/tests/testutil.h b/tests/testutil.h index ed6b13c..9896707 100644 --- a/tests/testutil.h +++ b/tests/testutil.h @@ -20,6 +20,28 @@ #include #include #include +#include + +/** + * @brief How many pool string slots are currently claimed. + * + * The string pool is 256 entries and every loader borrows scratch strings from + * it, so "does this function give back what it took" is a question a test can + * answer directly by counting either side of a call. A leak that would take + * fifty map loads to show up in production shows up here on the first one. + */ +static inline int test_string_pool_used(void) +{ + int used = 0; + int i = 0; + + for ( i = 0; i < AKGL_MAX_HEAP_STRING; i++ ) { + if ( akgl_heap_strings[i].refcount != 0 ) { + used += 1; + } + } + return used; +} /** * @brief Exit with a status the shell can actually see. diff --git a/tests/tilemap.c b/tests/tilemap.c index fe3f1a0..9286269 100644 --- a/tests/tilemap.c +++ b/tests/tilemap.c @@ -13,6 +13,100 @@ #include "testutil.h" +/** + * @brief akgl_get_json_tilemap_property must give back its scratch strings on every path. + * + * It claims two pool strings per call -- one for the property name it is + * comparing, one for the declared type -- and used to leave the block through a + * `SUCCEED_RETURN` from inside `ATTEMPT` on the path where it *found* what it + * was asked for. That returns past `CLEANUP`, so the ordinary, successful + * lookup was the leaking one, at two of the pool's 256 entries a time. A map + * load does this several times per layer. + * + * Three paths, run more times than the pool has entries: found, absent, and + * present-but-wrong-type. Against the old code the found loop exhausts the pool + * and comes back AKGL_ERR_HEAP long before it finishes. + */ +akerr_ErrorContext *test_tilemap_property_lookup_releases_strings(void) +{ + PREPARE_ERROR(errctx); + json_t *jsondoc = NULL; + json_error_t jsonerr; + akgl_String *pathstr = NULL; + int baseline = 0; + int propnum = 0; + int i = 0; + + ATTEMPT { + CATCH(errctx, akgl_heap_next_string(&pathstr)); + snprintf( + (char *)&pathstr->data, + AKGL_MAX_STRING_LENGTH, + "%s%s", + SDL_GetBasePath(), + "assets/snippets/test_tilemap_get_json_tilemap_property.json" + ); + jsondoc = json_load_file((char *)&pathstr->data, 0, (json_error_t *)&jsonerr); + FAIL_ZERO_BREAK(errctx, jsondoc, AKERR_NULLPOINTER, "Failure loading json fixture: %s", (char *)jsonerr.text); + + baseline = test_string_pool_used(); + + // The success path. Twice the pool size, so a leak of even one entry + // per call cannot finish. + for ( i = 0; i < (AKGL_MAX_HEAP_STRING * 2); i++ ) { + CATCH(errctx, akgl_get_json_properties_integer(jsondoc, "state", &propnum)); + } + TEST_ASSERT( + errctx, + test_string_pool_used() == baseline, + "%d successful property lookups left %d pool strings claimed, expected %d", + (AKGL_MAX_HEAP_STRING * 2), + test_string_pool_used(), + baseline); + + // The absent path, which reports AKERR_KEY from after FINISH. + for ( i = 0; i < (AKGL_MAX_HEAP_STRING * 2); i++ ) { + TEST_EXPECT_STATUS( + errctx, + AKERR_KEY, + akgl_get_json_properties_integer(jsondoc, "no_such_property", &propnum), + "looking up a property the object does not have"); + } + TEST_ASSERT( + errctx, + test_string_pool_used() == baseline, + "absent-property lookups left %d pool strings claimed, expected %d", + test_string_pool_used(), + baseline); + + // The wrong-type path, which fails from inside the ATTEMPT block with + // both scratch strings already claimed. + for ( i = 0; i < (AKGL_MAX_HEAP_STRING * 2); i++ ) { + TEST_EXPECT_STATUS( + errctx, + AKERR_TYPE, + akgl_get_json_properties_integer(jsondoc, "character", &propnum), + "reading a string property as an int"); + } + TEST_ASSERT( + errctx, + test_string_pool_used() == baseline, + "wrong-type lookups left %d pool strings claimed, expected %d", + test_string_pool_used(), + baseline); + } CLEANUP { + if ( jsondoc != NULL ) { + json_decref(jsondoc); + jsondoc = NULL; + } + if ( pathstr != NULL ) { + IGNORE(akgl_heap_release_string(pathstr)); + } + } PROCESS(errctx) { + } FINISH(errctx, true); + SUCCEED_RETURN(errctx); +} + akerr_ErrorContext *test_tilemap_akgl_get_json_tilemap_property(void) { PREPARE_ERROR(errctx); @@ -438,6 +532,7 @@ int main(void) CATCH(errctx, akgl_character_load_json("assets/testcharacter.json")); CATCH(errctx, test_tilemap_akgl_get_json_tilemap_property()); + CATCH(errctx, test_tilemap_property_lookup_releases_strings()); CATCH(errctx, test_akgl_tilemap_compute_tileset_offsets()); CATCH(errctx, test_akgl_tilemap_load_layer_objects()); CATCH(errctx, test_akgl_tilemap_load_layer_tile());