From 4d80664cd906ba6f16e590c5528f989d033a6603 Mon Sep 17 00:00:00 2001 From: Andrew Kesterson Date: Sat, 1 Aug 2026 09:18:52 -0400 Subject: [PATCH] Build clean under -Wall, with -Werror in CI only TODO.md item 37 said the cast sweep buys nothing until the build turns on the warnings those casts suppress. This is that precondition, and the argument turned out to be right with evidence: -Wall found three genuine signedness mismatches in sprite.c, where uint32_t width, height and speed were passed straight to akgl_get_json_integer_value(..., int *). A cast would have silenced all three. Fixing them properly also turned up that speed is scaled by a million into a 32-bit field, so anything past 4294 ms overflowed rather than being held. The other real finding was ten -Wstringop-truncation warnings, every one a fixed-width strncpy. Those leave a name field unterminated when the input fills it -- and those fields are registry keys, handed to strcmp and SDL property calls that do not stop at the field. Same overread class fixed twice already this release. All ten now use aksl_strncpy from akstdlib, which always terminates and never NUL-pads, bounded to size - 1 so an over-long name still truncates rather than being refused. staticstring.h documented the old strncpy semantics explicitly and has been rewritten to match. Two sites in game.c are deliberately left on strncpy: both stage into a pre-zeroed buffer for the savegame's fixed-width fields, where the NUL padding is part of the format. Neither is flagged. sprite.c also had a memcpy of a full 128-byte field from a NUL-terminated string, which reads past the end of any shorter name. Not flagged by anything; found while converting its neighbours. -Werror is an option, default OFF, on only in the cmake_build and performance CI jobs. libakgl is consumed with add_subdirectory -- akbasic does exactly that -- so a target-level -Werror turns every diagnostic a newer compiler invents into a broken build for somebody else. The warning set is not even stable across build types here: an -O2 build reports ten warnings that -O0 and -fsyntax-only do not, because they need the middle end. The two jobs that set it are one of each. -Wextra is deliberately not adopted. It adds 22 findings, 17 inherent to the design: 13 -Wunused-parameter from backend vtables and SDL callbacks that must match a signature, and 4 -Wimplicit-fallthrough from libakerror's own PROCESS/HANDLE/HANDLE_GROUP, which fall through by design. Filed upstream as deps/libakerror TODO item 8; the submodule bump carries it. deps/semver/semver.c is listed directly in add_library, so the target's PRIVATE options reach it. Exempted with -w so a future semver update cannot fail this build -- verified by forcing -Wextra -Werror and watching our files fail while semver compiled clean. Verified: RelWithDebInfo, Debug and coverage builds all clean under -Werror; -Werror actually fails on a planted unused variable; an embedded consumer with AKGL_WERROR unset still configures and builds. 25/25 pass, memcheck clean, reindent --check, check_api_surface and check_error_protocol clean. Co-Authored-By: Claude Opus 5 (1M context) --- .gitea/workflows/ci.yaml | 6 +++-- AGENTS.md | 42 +++++++++++++++++++++++++++++ CMakeLists.txt | 35 ++++++++++++++++++++++++ TODO.md | 54 +++++++++++++++++++++++++++---------- deps/libakerror | 2 +- include/akgl/staticstring.h | 39 ++++++++++++++------------- src/actor.c | 7 ++++- src/character.c | 5 +++- src/game.c | 1 - src/json_helpers.c | 13 +++++++-- src/physics.c | 1 - src/sprite.c | 52 ++++++++++++++++++++++++++++++----- src/staticstring.c | 8 +++--- src/tilemap.c | 24 ++++++++++++----- tests/registry.c | 2 -- util/charviewer.c | 1 - 16 files changed, 230 insertions(+), 62 deletions(-) diff --git a/.gitea/workflows/ci.yaml b/.gitea/workflows/ci.yaml index 8da6a11..f7b24e8 100644 --- a/.gitea/workflows/ci.yaml +++ b/.gitea/workflows/ci.yaml @@ -24,7 +24,8 @@ jobs: run: | cmake -S . -B build \ -DCMAKE_BUILD_TYPE=Debug \ - -DAKGL_COVERAGE=ON + -DAKGL_COVERAGE=ON \ + -DAKGL_WERROR=ON cmake --build build --parallel - name: Build API documentation run: doxygen Doxyfile @@ -90,7 +91,8 @@ jobs: - name: Configure and build run: | cmake -S . -B build \ - -DCMAKE_BUILD_TYPE=RelWithDebInfo + -DCMAKE_BUILD_TYPE=RelWithDebInfo \ + -DAKGL_WERROR=ON cmake --build build --parallel # --verbose rather than --output-on-failure: a passing benchmark's output # is the entire point of running it, and --output-on-failure prints diff --git a/AGENTS.md b/AGENTS.md index 88884f5..2d352b1 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -243,6 +243,48 @@ akerr_ErrorContext *akgl_actor_update(akgl_Actor *obj) ); ``` +### Compiler warnings + +**`-Wall` is on for every target this project owns** -- the library, the test +suites and `charviewer` -- and the tree builds clean under it at every +optimization level. + +**`-Werror` is not on by default.** It is `option(AKGL_WERROR ... OFF)`, turned +on only by the `cmake_build` and `performance` CI jobs. Two reasons, and both +are measured rather than assumed: + +- libakgl is consumed with `add_subdirectory()`; `akbasic` does exactly that. A + target-level `-Werror` makes every diagnostic a newer compiler invents into a + broken build for somebody else's project. +- The warning set is not stable across build types. An `-O2` build reports ten + `-Wstringop-truncation` warnings that `-O0` and `-fsyntax-only` do not, + because they need the middle end. The two CI jobs that set `AKGL_WERROR` are + deliberately one of each: coverage at `-O0`, performance at `-O2`. + +If you are checking this by hand, **compile for real at the optimization level +being shipped.** `-fsyntax-only` reported 6 of the 16 findings in `src/`. + +**`-Wextra` is deliberately not adopted.** It adds 22 findings, 17 of which are +inherent to the design rather than defects: 13 `-Wunused-parameter`, because a +backend vtable entry or an SDL callback must match a signature whether it reads +every argument or not, and 4 `-Wimplicit-fallthrough` from libakerror's own +`PROCESS`/`HANDLE`/`HANDLE_GROUP`, which fall through between `case` labels by +design. Adopting it today would mean disabling both permanently to gain 5 +`-Wsign-compare`. The fallthrough half is filed upstream as +`deps/libakerror/TODO.md` item 8; revisit when that lands. + +**Vendored code is exempt, not fixed.** `deps/semver/semver.c` is listed +directly in `add_library(akgl ...)`, so the target's `PRIVATE` options reach it; +`set_source_files_properties(... COMPILE_OPTIONS "-w")` keeps a future `semver` +update from failing this build. The other vendored projects are separate targets +and are unaffected either way. + +**Never silence a warning with a cast.** Three `-Wpointer-sign` findings in +`src/sprite.c` were real signedness mismatches -- `uint32_t *` passed where an +`int *` was expected -- and a cast would have hidden every one. That is the +whole argument of `TODO.md` item 37. Read into a correctly typed local, check +the range, and assign. + ### No repository-wide formatter There is no `clang-format` or linter wired into the build, and `cc-mode`'s diff --git a/CMakeLists.txt b/CMakeLists.txt index 0afa568..c3d0c3a 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -38,6 +38,32 @@ endif() include(CTest) option(AKGL_COVERAGE "Instrument libakgl and generate coverage reports with CTest" OFF) +# -Wall is always on for code this project owns. -Werror is not, and the +# distinction is deliberate. +# +# libakgl is consumed with add_subdirectory() -- akbasic does exactly that -- so +# a target-level -Werror makes every new compiler diagnostic a broken build for +# somebody else's project. The warning set is not even stable across build types +# here: an -O2 build reports ten -Wstringop-truncation warnings that -O0 and +# -fsyntax-only do not, because they need the middle end. A newer GCC or a +# switch to clang moves the line again. +# +# So the errors live in CI, where the compiler is pinned and a new warning is a +# task for the maintainer rather than an outage for a consumer. +option(AKGL_WERROR "Treat compiler warnings as errors. For CI; leave OFF when embedding." OFF) +set(AKGL_WARNING_FLAGS -Wall) +if(AKGL_WERROR) + list(APPEND AKGL_WARNING_FLAGS -Werror) +endif() + +# -Wextra is deliberately not here. It adds 22 findings, 17 of which are +# inherent to the design rather than defects: 13 -Wunused-parameter, because a +# backend vtable entry or an SDL callback has to match a signature whether it +# reads every argument or not, and 4 -Wimplicit-fallthrough from libakerror's +# own PROCESS/HANDLE/HANDLE_GROUP, which fall through between case labels by +# design. Adopting it would mean disabling both permanently to gain 5 +# -Wsign-compare. See deps/libakerror/TODO.md item 8. + if(AKGL_COVERAGE) if(NOT CMAKE_C_COMPILER_ID MATCHES "GNU|Clang") message(FATAL_ERROR "AKGL_COVERAGE requires GCC or Clang") @@ -261,6 +287,13 @@ set_target_properties(akgl PROPERTIES SOVERSION ${AKGL_SOVERSION} ) +target_compile_options(akgl PRIVATE ${AKGL_WARNING_FLAGS}) +# deps/semver/semver.c is vendored and listed directly in add_library(), so the +# target's PRIVATE options reach it. Exempt it: a future semver update must not +# be able to fail this build. (PRIVATE options do not reach SDL, jansson, +# akerror or akstdlib -- those are separate targets.) +set_source_files_properties(deps/semver/semver.c PROPERTIES COMPILE_OPTIONS "-w") + add_library(akgl::akgl ALIAS akgl) add_executable(charviewer util/charviewer.c) @@ -475,9 +508,11 @@ target_link_libraries(akgl foreach(suite IN LISTS AKGL_TEST_SUITES AKGL_PERF_SUITES) target_link_libraries(akgl_test_${suite} PRIVATE akstdlib::akstdlib akerror::akerror akgl SDL3::SDL3 SDL3_ttf::SDL3_ttf SDL3_image::SDL3_image SDL3_mixer::SDL3_mixer jansson::jansson -lm) target_include_directories(akgl_test_${suite} PRIVATE "${CMAKE_CURRENT_SOURCE_DIR}/tests") + target_compile_options(akgl_test_${suite} PRIVATE ${AKGL_WARNING_FLAGS}) endforeach() target_link_libraries(charviewer PRIVATE akstdlib::akstdlib akerror::akerror akgl SDL3::SDL3 SDL3_ttf::SDL3_ttf SDL3_image::SDL3_image SDL3_mixer::SDL3_mixer jansson::jansson -lm) +target_compile_options(charviewer PRIVATE ${AKGL_WARNING_FLAGS}) # When the vendored SDL satellite libraries are built in-tree they land in per- # project subdirectories that are not on the loader's default search path, so a diff --git a/TODO.md b/TODO.md index 3b8b17e..e153b10 100644 --- a/TODO.md +++ b/TODO.md @@ -358,22 +358,25 @@ below.** so `akgl_physics_init_arcade` and `akgl_render_2d_init` quietly ignored their configuration. `tests/registry.c` sets a property and reads it back. -37. **Redundant casts obscure the code.** **Still open, and deliberately so.** - Roughly 180 pointer casts across `src/`, a large majority no-ops -- - `(json_t *)json` where `json` is already `json_t *`, `(char *)&obj->name` on - an array that decays anyway -- densest in `src/tilemap.c` and - `src/character.c`. +37. **Redundant casts obscure the code.** **Still open -- but the precondition + it named is now met.** Roughly 180 pointer casts across `src/`, a large + majority no-ops, densest in `src/tilemap.c` and `src/character.c`. - The argument for fixing it is real: a no-op cast suppresses exactly the - conversion warning that would catch a mismatch. The argument for not doing - it *here* is that it is the largest and least mechanical change on this - list, it touches almost every line of the two files with the most - outstanding functional defects, and the benefit only arrives once the build - actually turns those warnings on -- which it does not today. Doing this - without `-Wall -Wextra` on the library target buys nothing but churn. + This item used to say the benefit only arrives once the build turns on the + warnings those casts suppress, and that doing it first buys nothing but + churn. `-Wall` is on now (see `AGENTS.md`, "Compiler warnings"), so the + sweep can proceed as its own commit. - **Do them together**, in that order: enable the warnings, see what they say, - then remove the casts that were hiding something and the ones that were not. + **The argument turned out to be right, with evidence.** Turning `-Wall` on + found three genuine signedness mismatches in `src/sprite.c`: `obj->width`, + `obj->height` and `obj->speed` are `uint32_t` and were being passed straight + to `akgl_get_json_integer_value(..., int *)`. A cast would have silenced all + three. They are fixed by reading into an `int`, range-checking, and + assigning -- which also turned up that `speed` is scaled by 1,000,000 into a + 32-bit field, so anything past 4294 ms overflowed rather than being held. + + When doing the sweep: remove a cast, rebuild, and read what the compiler + says. A cast that was load-bearing will say so. 38. **`struct`-qualified parameters in two definitions.** `akgl_physics_null_move` and `akgl_physics_arcade_move` use the typedef like the other eight. @@ -1858,6 +1861,29 @@ this library and told it is compatible. outlived its own press, the code-point-boundary truncation, and both NULL arguments. `src/controller.c` holds at 91%. +## Truncated registry keys can collide + +Found while turning `-Wall` on. `akgl_actor_initialize` and +`akgl_character_initialize` document their name fields as "Truncated, not +rejected, if the source name is longer", and that is what they do -- the copy is +`aksl_strncpy` bounded to the field, so it always terminates now. + +Termination was the overread half, and it is fixed. The other half is not: the +truncated name **is the registry key**. Two distinct 200-character names +truncate to the same 127-byte key, and the second `SDL_SetPointerProperty` +silently replaces the first. The objects are different; the registry cannot tell. + +The same applies to `akgl_Sprite::name` (128), `akgl_SpriteSheet::name` (512) +and the tilemap's object and tileset names. + +Whether that matters depends on whether long asset names are realistic, and 127 +bytes is generous for a hand-written name in a JSON file. Recording it rather +than fixing it, because the fix is a contract change -- refuse an over-long name +with `AKERR_OUTOFBOUNDS` instead of truncating -- and every one of those headers +currently promises the opposite. `aksl_strncpy` already reports exactly that +status when the bytes do not fit, so the change is to stop capping `n` at +`size - 1` and let it raise. + ## Carried over 1. **Make character-to-sprite state bindings release their references symmetrically.** diff --git a/deps/libakerror b/deps/libakerror index 5ff8790..7993636 160000 --- a/deps/libakerror +++ b/deps/libakerror @@ -1 +1 @@ -Subproject commit 5ff87908e7b68ab2dc328b55ea89585272411ff9 +Subproject commit 79936362d0773b1e5abfe4e6bdd4c3a1f073ad90 diff --git a/include/akgl/staticstring.h b/include/akgl/staticstring.h index 2953300..8bb2372 100644 --- a/include/akgl/staticstring.h +++ b/include/akgl/staticstring.h @@ -27,16 +27,18 @@ typedef struct /** * @brief Set a pooled string's contents and mark the slot in use. * - * Copies at most #AKGL_MAX_STRING_LENGTH bytes out of @p init, or zeroes the - * buffer when @p init is `NULL`, then sets `refcount` to 1. Callers normally - * reach this through akgl_heap_next_string rather than calling it directly. + * Copies @p init into the buffer, or zeroes it when @p init is `NULL`, then + * sets `refcount` to 1. Callers normally reach this through + * akgl_heap_next_string rather than calling it directly. * * @param obj The pooled string to (re)initialize. Required. Its previous * contents are discarded without inspection. * @param init Initial contents, NUL-terminated. Optional -- `NULL` zero-fills - * the buffer instead. An @p init longer than - * #AKGL_MAX_STRING_LENGTH is truncated *and left unterminated*, - * because this is `strncpy` semantics, not `strlcpy`. + * the buffer instead. An @p init that does not fit is truncated, + * and the result is **always NUL-terminated**. Until 0.5.0 this was + * `strncpy` at exactly the buffer size, which left an over-long + * string unterminated -- and a pooled string is handed to `strcmp`, + * `realpath` and SDL property calls, none of which stop at 4096. * @return `NULL` on success, otherwise an error context owned by the caller. * @throws AKERR_NULLPOINTER If @p obj is `NULL`. * @@ -49,26 +51,27 @@ akerr_ErrorContext AKERR_NOIGNORE *akgl_string_initialize(akgl_String *obj, char /** * @brief Copy the contents of one pooled string into another. * - * A bounded `strncpy` between two already-claimed pool slots. It copies bytes - * only: `refcount` is left alone, so @p dest keeps whatever pool state it had. + * A bounded copy between two already-claimed pool slots. It copies bytes only: + * `refcount` is left alone, so @p dest keeps whatever pool state it had. * * @param src Source string. Required. Read up to @p count bytes. - * @param dest Destination string. Required. Overwritten in place; the pool + * @param dest Destination string. Required. Overwritten in place; the pool * slot must already have been claimed. * @param count Maximum bytes to copy. 0 selects #AKGL_MAX_STRING_LENGTH, the - * whole buffer. A @p count shorter than the source truncates - * without writing a terminator; a @p count longer than the source - * zero-pads the remainder, per `strncpy`. A negative @p count, or - * one above #AKGL_MAX_STRING_LENGTH, is refused -- both buffers - * are exactly that long, so a larger count walked off the end of - * two pool slots at once. + * whole buffer. A @p count shorter than the source truncates, and + * the result is **always NUL-terminated**. The remainder is not + * zero-padded, so this no longer writes the full buffer for a + * short copy. A negative @p count, or one above + * #AKGL_MAX_STRING_LENGTH, is refused -- both buffers are exactly + * that long, so a larger count walked off the end of two pool + * slots at once. * @return `NULL` on success, otherwise an error context owned by the caller. * @throws AKERR_NULLPOINTER If @p src or @p dest is `NULL`. * @throws AKERR_OUTOFBOUNDS If @p count is negative or above * #AKGL_MAX_STRING_LENGTH. - * @throws errno Whatever `errno` holds if `strncpy` returns something other - * than @p dest. In practice `strncpy` always returns its destination, so - * this path is unreachable rather than merely rare. + * @throws AKERR_VALUE, AKERR_OUTOFBOUNDS Whatever `aksl_strncpy` reports. The + * unreachable `errno` path this used to document went with the + * `strncpy` call it described. */ akerr_ErrorContext AKERR_NOIGNORE *akgl_string_copy(akgl_String *src, akgl_String *dest, int count); #endif //_AKGL_STATICSTRING_H_ diff --git a/src/actor.c b/src/actor.c index 4463800..411c850 100644 --- a/src/actor.c +++ b/src/actor.c @@ -7,6 +7,7 @@ #include #include #include +#include #include #include @@ -24,7 +25,11 @@ akerr_ErrorContext *akgl_actor_initialize(akgl_Actor *obj, char *name) FAIL_ZERO_RETURN(errctx, name, AKERR_NULLPOINTER, "akgl_actor_initialize received null name string pointer"); memset(obj, 0x00, sizeof(akgl_Actor)); - strncpy((char *)obj->name, name, AKGL_ACTOR_MAX_NAME_LENGTH); + // aksl_strncpy always terminates; strncpy at exactly the field width does + // not, and this field is a registry key. n is one less than the field so an + // over-long name still truncates rather than being refused, which is the + // documented contract. + PASS(errctx, aksl_strncpy((char *)obj->name, sizeof(obj->name), name, sizeof(obj->name) - 1)); obj->curSpriteReversing = false; obj->scale = 1.0; obj->movement_controls_face = true; diff --git a/src/character.c b/src/character.c index fda9c8f..62d3d78 100644 --- a/src/character.c +++ b/src/character.c @@ -6,6 +6,7 @@ #include #include #include +#include #include #include @@ -24,7 +25,9 @@ akerr_ErrorContext *akgl_character_initialize(akgl_Character *obj, char *name) FAIL_ZERO_RETURN(errctx, obj, AKERR_NULLPOINTER, "NULL akgl_Character reference"); FAIL_ZERO_RETURN(errctx, name, AKERR_NULLPOINTER, "NULL name string pointer"); memset(obj, 0x00, sizeof(akgl_Character)); - strncpy(obj->name, name, AKGL_CHARACTER_MAX_NAME_LENGTH); + // Always terminated: this is a registry key, and strncpy at exactly the + // field width leaves an over-long name unterminated. + PASS(errctx, aksl_strncpy(obj->name, sizeof(obj->name), name, sizeof(obj->name) - 1)); obj->state_sprites = SDL_CreateProperties(); FAIL_ZERO_RETURN(errctx, obj->state_sprites, AKERR_NULLPOINTER, "Unable to initialize SDL_PropertiesID for character state map"); diff --git a/src/game.c b/src/game.c index ffbfd3d..4458745 100644 --- a/src/game.c +++ b/src/game.c @@ -523,7 +523,6 @@ static akerr_ErrorContext *load_objectnamemap(FILE *fp, SDL_PropertiesID map, in void *ptr = NULL; char ptrstring[32]; char objname[namelength]; - int retval = 0; bool done = false; PREPARE_ERROR(errctx); diff --git a/src/json_helpers.c b/src/json_helpers.c index ef9ef09..14d9d6f 100644 --- a/src/json_helpers.c +++ b/src/json_helpers.c @@ -6,6 +6,7 @@ #include #include #include +#include #include #include @@ -102,7 +103,11 @@ akerr_ErrorContext *akgl_get_json_string_value(json_t *obj, char *key, akgl_Stri } PROCESS(errctx) { } FINISH(errctx, true); - strncpy((char *)&(*dest)->data, json_string_value(value), AKGL_MAX_STRING_LENGTH); + PASS(errctx, aksl_strncpy( + (char *)&(*dest)->data, + sizeof((*dest)->data), + json_string_value(value), + sizeof((*dest)->data) - 1)); SUCCEED_RETURN(errctx); } @@ -164,7 +169,11 @@ akerr_ErrorContext *akgl_get_json_array_index_string(json_t *array, int index, a } PROCESS(errctx) { } FINISH(errctx, true); - strncpy((char *)&(*dest)->data, json_string_value(value), AKGL_MAX_STRING_LENGTH); + PASS(errctx, aksl_strncpy( + (char *)&(*dest)->data, + sizeof((*dest)->data), + json_string_value(value), + sizeof((*dest)->data) - 1)); SUCCEED_RETURN(errctx); } diff --git a/src/physics.c b/src/physics.c index 834945f..d8a3fd2 100644 --- a/src/physics.c +++ b/src/physics.c @@ -233,7 +233,6 @@ akerr_ErrorContext *akgl_physics_simulate(akgl_PhysicsBackend *self, akgl_Iterat akerr_ErrorContext *akgl_physics_factory(akgl_PhysicsBackend *self, akgl_String *type) { - uint32_t hashval; PREPARE_ERROR(errctx); FAIL_ZERO_RETURN(errctx, self, AKERR_NULLPOINTER, "self"); FAIL_ZERO_RETURN(errctx, type, AKERR_NULLPOINTER, "type"); diff --git a/src/sprite.c b/src/sprite.c index 0929058..c344e95 100644 --- a/src/sprite.c +++ b/src/sprite.c @@ -8,6 +8,7 @@ #include #include #include +#include #include #include @@ -121,6 +122,7 @@ akerr_ErrorContext *akgl_sprite_load_json(char *filename) int i = 0; int framecount = 0; int frameid = 0; + int jsonint = 0; FAIL_ZERO_RETURN(errctx, filename, AKERR_NULLPOINTER, "Received null filename"); ATTEMPT { @@ -128,7 +130,7 @@ akerr_ErrorContext *akgl_sprite_load_json(char *filename) CATCH(errctx, akgl_heap_next_string(&filename_copy)); FAIL_NONZERO_BREAK( errctx, - strlen(filename) >= AKGL_MAX_STRING_LENGTH, + (strlen(filename) >= AKGL_MAX_STRING_LENGTH), AKERR_OUTOFBOUNDS, "Sprite filename exceeds temporary string capacity" ); @@ -155,10 +157,43 @@ akerr_ErrorContext *akgl_sprite_load_json(char *filename) (akgl_SpriteSheet *)sheet) ); - CATCH(errctx, akgl_get_json_integer_value((json_t *)json, "width", &obj->width)); - CATCH(errctx, akgl_get_json_integer_value((json_t *)json, "height", &obj->height)); - CATCH(errctx, akgl_get_json_integer_value((json_t *)json, "speed", &obj->speed)); - obj->speed = obj->speed * AKGL_TIME_ONEMS_NS; + // Read into an int and narrow deliberately. These three fields are + // uint32_t and the accessor takes an int *, so passing them directly was + // a signedness mismatch -- the kind -Wpointer-sign exists to catch, and + // the kind a cast would have hidden. + CATCH(errctx, akgl_get_json_integer_value((json_t *)json, "width", &jsonint)); + FAIL_NONZERO_BREAK( + errctx, + (jsonint <= 0), + AKERR_VALUE, + "Sprite %s has width %d; a sprite must be at least one pixel wide", + (char *)&obj->name, + jsonint); + obj->width = (uint32_t)jsonint; + + CATCH(errctx, akgl_get_json_integer_value((json_t *)json, "height", &jsonint)); + FAIL_NONZERO_BREAK( + errctx, + (jsonint <= 0), + AKERR_VALUE, + "Sprite %s has height %d; a sprite must be at least one pixel high", + (char *)&obj->name, + jsonint); + obj->height = (uint32_t)jsonint; + + // speed is milliseconds in the file and nanoseconds in the struct, and + // the struct field is 32 bits, so anything past this overflows on the + // multiply below rather than being held. + CATCH(errctx, akgl_get_json_integer_value((json_t *)json, "speed", &jsonint)); + FAIL_NONZERO_BREAK( + errctx, + ((jsonint < 0) || ((uint32_t)jsonint > (UINT32_MAX / AKGL_TIME_ONEMS_NS))), + AKERR_OUTOFBOUNDS, + "Sprite %s has a frame speed of %d ms; the range is 0 to %u", + (char *)&obj->name, + jsonint, + (UINT32_MAX / AKGL_TIME_ONEMS_NS)); + obj->speed = ((uint32_t)jsonint) * AKGL_TIME_ONEMS_NS; CATCH(errctx, akgl_get_json_boolean_value((json_t *)json, "loop", &obj->loop)); CATCH(errctx, akgl_get_json_boolean_value((json_t *)json, "loopReverse", &obj->loopReverse)); @@ -226,7 +261,10 @@ akerr_ErrorContext *akgl_sprite_initialize(akgl_Sprite *spr, char *name, akgl_Sp FAIL_ZERO_RETURN(errctx, sheet, AKERR_NULLPOINTER, "Null spritesheet reference"); memset(spr, 0x00, sizeof(akgl_Sprite)); - memcpy(spr->name, name, AKGL_SPRITE_MAX_NAME_LENGTH); + // Was a memcpy of the full field width from a NUL-terminated string, which + // reads past the end of any name shorter than the field. Copies only what + // is there now, and terminates. + PASS(errctx, aksl_strncpy(spr->name, sizeof(spr->name), name, sizeof(spr->name) - 1)); spr->sheet = sheet; FAIL_ZERO_RETURN( errctx, @@ -251,7 +289,7 @@ akerr_ErrorContext *akgl_spritesheet_initialize(akgl_SpriteSheet *sheet, int spr //CATCH(errctx, akgl_heap_next_string(&tmpstr)); //CATCH(errctx, akgl_string_initialize(tmpstr, NULL)); - strncpy((char *)&sheet->name, filename, AKGL_SPRITE_SHEET_MAX_FILENAME_LENGTH); + CATCH(errctx, aksl_strncpy((char *)&sheet->name, sizeof(sheet->name), filename, sizeof(sheet->name) - 1)); sheet->texture = IMG_LoadTexture(akgl_renderer->sdl_renderer, filename); FAIL_ZERO_BREAK(errctx, sheet->texture, AKGL_ERR_SDL, "Failed loading asset %s : %s", filename, SDL_GetError()); diff --git a/src/staticstring.c b/src/staticstring.c index 357ef51..23a540e 100644 --- a/src/staticstring.c +++ b/src/staticstring.c @@ -4,15 +4,15 @@ */ #include +#include #include -#include akerr_ErrorContext *akgl_string_initialize(akgl_String *obj, char *init) { PREPARE_ERROR(errctx); FAIL_ZERO_RETURN(errctx, obj, AKERR_NULLPOINTER, "Attempted to initialize NULL string reference"); if ( init != NULL ) { - strncpy((char *)&obj->data, init, AKGL_MAX_STRING_LENGTH); + PASS(errctx, aksl_strncpy((char *)&obj->data, sizeof(obj->data), init, sizeof(obj->data) - 1)); } else { // sizeof(obj->data), not sizeof(akgl_String). `data` starts after the // `refcount` in front of it, so zeroing the size of the whole struct @@ -43,8 +43,6 @@ akerr_ErrorContext *akgl_string_copy(akgl_String *src, akgl_String *dest, int co "Copy count %d is outside 0..%d", count, AKGL_MAX_STRING_LENGTH); - if ( (char *)dest->data != strncpy((char *)&dest->data, (char *)&src->data, count) ) { - FAIL_RETURN(errctx, errno, "strncpy"); - } + PASS(errctx, aksl_strncpy((char *)&dest->data, sizeof(dest->data), (char *)&src->data, count)); SUCCEED_RETURN(errctx); } diff --git a/src/tilemap.c b/src/tilemap.c index fa622b4..5a346fb 100644 --- a/src/tilemap.c +++ b/src/tilemap.c @@ -149,14 +149,21 @@ akerr_ErrorContext *akgl_tilemap_load_tilesets_each(json_t *tileset, akgl_Tilema PASS(errctx, akgl_get_json_string_value((json_t *)tileset, "name", &tmpstr)); PASS(errctx, akgl_heap_next_string(&tmppath)); ATTEMPT { - strncpy((char *)&dest->tilesets[tsidx].name, - (char *)&tmpstr->data, - AKGL_TILEMAP_MAX_TILESET_NAME_SIZE - ); + CATCH(errctx, aksl_strncpy( + (char *)&dest->tilesets[tsidx].name, + sizeof(dest->tilesets[tsidx].name), + (char *)&tmpstr->data, + sizeof(dest->tilesets[tsidx].name) - 1 + )); CATCH(errctx, akgl_get_json_string_value((json_t *)tileset, "image", &tmpstr)); CATCH(errctx, akgl_path_relative((char *)&dirname->data, (char *)&tmpstr->data, tmppath)); - strncpy((char *)&dest->tilesets[tsidx].imagefilename, tmppath->data, AKGL_MAX_STRING_LENGTH); + CATCH(errctx, aksl_strncpy( + (char *)&dest->tilesets[tsidx].imagefilename, + sizeof(dest->tilesets[tsidx].imagefilename), + tmppath->data, + sizeof(dest->tilesets[tsidx].imagefilename) - 1 + )); } CLEANUP { IGNORE(akgl_heap_release_string(tmpstr)); IGNORE(akgl_heap_release_string(tmppath)); @@ -346,7 +353,12 @@ akerr_ErrorContext *akgl_tilemap_load_layer_objects(akgl_Tilemap *dest, json_t * // 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, aksl_strncpy( + (char *)curobj->name, + sizeof(curobj->name), + tmpstr->data, + sizeof(curobj->name) - 1 + )); 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)); diff --git a/tests/registry.c b/tests/registry.c index 15a57c1..8093129 100644 --- a/tests/registry.c +++ b/tests/registry.c @@ -241,7 +241,6 @@ akerr_ErrorContext *test_actor_state_string_names(void) akerr_ErrorContext *test_akgl_registry_init_is_repeatable(void) { PREPARE_ERROR(errctx); - SDL_PropertiesID first = 0; akgl_String *readback = NULL; ATTEMPT { @@ -254,7 +253,6 @@ akerr_ErrorContext *test_akgl_registry_init_is_repeatable(void) // the memcheck run's job, and `cmake --build build --target memcheck` // gates on it. What is observable is that re-initializing really does // replace: a key written before the call must be gone after it. - first = AKGL_REGISTRY_SPRITE; SDL_SetNumberProperty(AKGL_REGISTRY_SPRITE, "akgl_test_sentinel", 1); TEST_ASSERT(errctx, SDL_GetNumberProperty(AKGL_REGISTRY_SPRITE, "akgl_test_sentinel", 0) == 1, diff --git a/util/charviewer.c b/util/charviewer.c index 997e8c8..6c1fa45 100644 --- a/util/charviewer.c +++ b/util/charviewer.c @@ -24,7 +24,6 @@ SDL_AppResult SDL_AppInit(void **appstate, int argc, char *argv[]) { PREPARE_ERROR(errctx); akgl_Actor *actorptr = NULL; - int i = 0; int gamepadids[32]; char *characterjson = NULL; char pathbuf[4096];