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];