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) <noreply@anthropic.com>
302 lines
10 KiB
C
302 lines
10 KiB
C
#include <SDL3/SDL.h>
|
|
#include <stdlib.h>
|
|
#include <string.h>
|
|
#include <akerror.h>
|
|
#include <akgl/error.h>
|
|
#include <akgl/heap.h>
|
|
#include <akgl/registry.h>
|
|
#include <akgl/actor.h>
|
|
#include <akgl/staticstring.h>
|
|
|
|
#include "testutil.h"
|
|
|
|
typedef akerr_ErrorContext *(*RegistryFuncPtr)(void);
|
|
|
|
void *sdl_calloc_always_fails(size_t a, size_t b)
|
|
{
|
|
// This forces SDL to simulate an out of memory condition
|
|
return NULL;
|
|
}
|
|
|
|
akerr_ErrorContext *test_akgl_registry_init(RegistryFuncPtr funcptr)
|
|
{
|
|
SDL_malloc_func malloc_func;
|
|
SDL_calloc_func calloc_func;
|
|
SDL_realloc_func realloc_func;
|
|
SDL_free_func free_func;
|
|
|
|
SDL_GetMemoryFunctions(
|
|
&malloc_func,
|
|
&calloc_func,
|
|
&realloc_func,
|
|
&free_func
|
|
);
|
|
PREPARE_ERROR(errctx);
|
|
ATTEMPT {
|
|
SDL_SetMemoryFunctions(
|
|
malloc_func,
|
|
(SDL_calloc_func)&sdl_calloc_always_fails,
|
|
realloc_func,
|
|
free_func
|
|
);
|
|
CATCH(errctx, funcptr());
|
|
} CLEANUP {
|
|
SDL_SetMemoryFunctions(
|
|
malloc_func,
|
|
calloc_func,
|
|
realloc_func,
|
|
free_func
|
|
);
|
|
} PROCESS(errctx) {
|
|
} FINISH(errctx, true);
|
|
|
|
FAIL_RETURN(errctx, AKGL_ERR_BEHAVIOR, "SDL memory allocator fails but registry reports successful property creation");
|
|
}
|
|
|
|
akerr_ErrorContext *test_akgl_registry_init_creation_failures(void)
|
|
{
|
|
PREPARE_ERROR(errctx);
|
|
ATTEMPT {
|
|
CATCH(errctx, test_akgl_registry_init(&akgl_registry_init_actor));
|
|
} CLEANUP {
|
|
} PROCESS(errctx) {
|
|
} HANDLE(errctx, AKERR_NULLPOINTER) {
|
|
printf("Sucess\n");
|
|
} FINISH(errctx, true);
|
|
|
|
ATTEMPT {
|
|
CATCH(errctx, test_akgl_registry_init(&akgl_registry_init_sprite));
|
|
} CLEANUP {
|
|
} PROCESS(errctx) {
|
|
} HANDLE(errctx, AKERR_NULLPOINTER) {
|
|
printf("Sucess\n");
|
|
} FINISH(errctx, true);
|
|
|
|
ATTEMPT {
|
|
CATCH(errctx, test_akgl_registry_init(&akgl_registry_init_spritesheet));
|
|
} CLEANUP {
|
|
} PROCESS(errctx) {
|
|
} HANDLE(errctx, AKERR_NULLPOINTER) {
|
|
printf("Sucess\n");
|
|
} FINISH(errctx, true);
|
|
|
|
ATTEMPT {
|
|
CATCH(errctx, test_akgl_registry_init(&akgl_registry_init_character));
|
|
} CLEANUP {
|
|
} PROCESS(errctx) {
|
|
} HANDLE(errctx, AKERR_NULLPOINTER) {
|
|
printf("Sucess\n");
|
|
} FINISH(errctx, true);
|
|
SUCCEED_RETURN(errctx);
|
|
}
|
|
|
|
/**
|
|
* @brief akgl_get_property must copy the value, and not a byte more.
|
|
*
|
|
* It used to copy a fixed AKGL_MAX_STRING_LENGTH bytes out of whatever SDL
|
|
* returned, and what SDL returns is its own strdup of the value -- four bytes
|
|
* for "0.0". That read up to 4 KiB past the end of somebody else's allocation on
|
|
* every call, which valgrind reported twelve times over in tests/physics.c
|
|
* alone.
|
|
*
|
|
* The destination is filled with a sentinel first, so the assertion is not
|
|
* "the value arrived" -- that would pass either way -- but "the bytes past the
|
|
* terminator were left alone", which is only true if the copy was bounded by
|
|
* the source.
|
|
*/
|
|
akerr_ErrorContext *test_akgl_get_property_copies_only_the_value(void)
|
|
{
|
|
PREPARE_ERROR(errctx);
|
|
akgl_String *value = NULL;
|
|
char oversized[AKGL_MAX_STRING_LENGTH + 8];
|
|
size_t valuelen = 0;
|
|
size_t i = 0;
|
|
bool untouched = true;
|
|
|
|
ATTEMPT {
|
|
CATCH(errctx, akgl_heap_init());
|
|
CATCH(errctx, akgl_registry_init_properties());
|
|
CATCH(errctx, akgl_heap_next_string(&value));
|
|
|
|
memset(&value->data, 0x5a, AKGL_MAX_STRING_LENGTH);
|
|
CATCH(errctx, akgl_set_property("registry.short", "0.0"));
|
|
CATCH(errctx, akgl_get_property("registry.short", &value, "unset"));
|
|
|
|
valuelen = strlen("0.0");
|
|
TEST_ASSERT(errctx, strcmp(value->data, "0.0") == 0,
|
|
"akgl_get_property returned \"%s\", expected \"0.0\"", value->data);
|
|
for ( i = (valuelen + 1); i < AKGL_MAX_STRING_LENGTH; i++ ) {
|
|
TEST_ASSERT_FLAG(untouched, (value->data[i] == 0x5a));
|
|
}
|
|
TEST_ASSERT(errctx, untouched,
|
|
"akgl_get_property wrote past the end of a %zu byte value", valuelen);
|
|
|
|
// The default takes the same path as a stored value.
|
|
CATCH(errctx, akgl_get_property("registry.absent", &value, "fallback"));
|
|
TEST_ASSERT(errctx, strcmp(value->data, "fallback") == 0,
|
|
"akgl_get_property returned \"%s\" for an unset property, expected the default",
|
|
value->data);
|
|
|
|
// A value too long for an akgl_String is refused rather than truncated
|
|
// into an unterminated buffer.
|
|
memset(&oversized, 'x', sizeof(oversized));
|
|
oversized[AKGL_MAX_STRING_LENGTH + 7] = '\0';
|
|
CATCH(errctx, akgl_set_property("registry.oversized", (char *)&oversized));
|
|
TEST_EXPECT_STATUS(errctx, AKERR_OUTOFBOUNDS,
|
|
akgl_get_property("registry.oversized", &value, "unset"),
|
|
"reading a property longer than an akgl_String");
|
|
|
|
// And the documented refusal survives: unset, with no default.
|
|
TEST_EXPECT_STATUS(errctx, AKERR_NULLPOINTER,
|
|
akgl_get_property("registry.absent", &value, NULL),
|
|
"reading an unset property with no default");
|
|
} CLEANUP {
|
|
if ( value != NULL ) {
|
|
IGNORE(akgl_heap_release_string(value));
|
|
}
|
|
} PROCESS(errctx) {
|
|
} FINISH(errctx, true);
|
|
SUCCEED_RETURN(errctx);
|
|
}
|
|
|
|
/**
|
|
* @brief Every actor-state bit must be nameable, and every name must map to its own bit.
|
|
*
|
|
* akgl_registry_init_actor_state_strings walks AKGL_ACTOR_STATE_STRING_NAMES
|
|
* and registers entry `i` under the value `1 << i`. Character JSON resolves
|
|
* state names through that registry, so an entry that disagrees with the
|
|
* `#define` in actor.h is a state no character can ever bind a sprite to. Bits
|
|
* 11 and 12 were `UNDEFINED_11` and `UNDEFINED_12` here while actor.h called
|
|
* them MOVING_IN and MOVING_OUT, so both were unreachable by name.
|
|
*
|
|
* The loop also pins the array's length: it reads exactly
|
|
* AKGL_ACTOR_MAX_STATES entries, which is what the header declares and what
|
|
* the definition is now sized by. It was declared one longer than defined.
|
|
*/
|
|
akerr_ErrorContext *test_actor_state_string_names(void)
|
|
{
|
|
PREPARE_ERROR(errctx);
|
|
int i = 0;
|
|
int j = 0;
|
|
Sint64 registered = 0;
|
|
|
|
ATTEMPT {
|
|
CATCH(errctx, akgl_registry_init_actor_state_strings());
|
|
|
|
for ( i = 0; i < AKGL_ACTOR_MAX_STATES; i++ ) {
|
|
TEST_ASSERT(errctx, AKGL_ACTOR_STATE_STRING_NAMES[i] != NULL,
|
|
"actor state bit %d has no name", i);
|
|
|
|
registered = SDL_GetNumberProperty(
|
|
AKGL_REGISTRY_ACTOR_STATE_STRINGS,
|
|
AKGL_ACTOR_STATE_STRING_NAMES[i],
|
|
0);
|
|
TEST_ASSERT(errctx, registered == (Sint64)(1 << i),
|
|
"state name %s resolves to %lld, expected %d",
|
|
AKGL_ACTOR_STATE_STRING_NAMES[i],
|
|
(long long)registered,
|
|
(1 << i));
|
|
|
|
// No two entries may share a name, or the later one silently
|
|
// overwrites the earlier in the registry and one bit becomes
|
|
// unreachable without anything looking wrong.
|
|
for ( j = 0; j < i; j++ ) {
|
|
TEST_ASSERT(errctx,
|
|
strcmp(AKGL_ACTOR_STATE_STRING_NAMES[i],
|
|
AKGL_ACTOR_STATE_STRING_NAMES[j]) != 0,
|
|
"actor state bits %d and %d share the name %s",
|
|
j, i, AKGL_ACTOR_STATE_STRING_NAMES[i]);
|
|
}
|
|
}
|
|
|
|
// The two that were unreachable, named explicitly so a regression reads
|
|
// as what it is rather than as an index.
|
|
TEST_ASSERT(errctx,
|
|
SDL_GetNumberProperty(AKGL_REGISTRY_ACTOR_STATE_STRINGS,
|
|
"AKGL_ACTOR_STATE_MOVING_IN", 0)
|
|
== (Sint64)AKGL_ACTOR_STATE_MOVING_IN,
|
|
"AKGL_ACTOR_STATE_MOVING_IN is not resolvable by name");
|
|
TEST_ASSERT(errctx,
|
|
SDL_GetNumberProperty(AKGL_REGISTRY_ACTOR_STATE_STRINGS,
|
|
"AKGL_ACTOR_STATE_MOVING_OUT", 0)
|
|
== (Sint64)AKGL_ACTOR_STATE_MOVING_OUT,
|
|
"AKGL_ACTOR_STATE_MOVING_OUT is not resolvable by name");
|
|
} CLEANUP {
|
|
} PROCESS(errctx) {
|
|
} FINISH(errctx, true);
|
|
SUCCEED_RETURN(errctx);
|
|
}
|
|
|
|
/**
|
|
* @brief Re-initializing a registry must replace the old set, not abandon it.
|
|
*
|
|
* Only akgl_registry_init_actor destroyed the set it was replacing; the other
|
|
* seven leaked an SDL_PropertiesID on every call after the first. A game that
|
|
* resets between levels calls these repeatedly.
|
|
*
|
|
* Also checks that akgl_registry_init() brings up the properties registry.
|
|
* It did not until 0.5.0, so akgl_set_property was a silent no-op for any
|
|
* caller that had not also gone through akgl_game_init.
|
|
*/
|
|
akerr_ErrorContext *test_akgl_registry_init_is_repeatable(void)
|
|
{
|
|
PREPARE_ERROR(errctx);
|
|
akgl_String *readback = NULL;
|
|
|
|
ATTEMPT {
|
|
CATCH(errctx, akgl_registry_init());
|
|
TEST_ASSERT(errctx, AKGL_REGISTRY_PROPERTIES != 0,
|
|
"akgl_registry_init did not create the properties registry");
|
|
|
|
// Whether the old set was *destroyed* is not observable from here --
|
|
// SDL hands out no liveness query, and it reuses ids -- so that half is
|
|
// 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.
|
|
SDL_SetNumberProperty(AKGL_REGISTRY_SPRITE, "akgl_test_sentinel", 1);
|
|
TEST_ASSERT(errctx,
|
|
SDL_GetNumberProperty(AKGL_REGISTRY_SPRITE, "akgl_test_sentinel", 0) == 1,
|
|
"could not write a sentinel into the sprite registry");
|
|
CATCH(errctx, akgl_registry_init_sprite());
|
|
TEST_ASSERT(errctx, AKGL_REGISTRY_SPRITE != 0,
|
|
"re-initializing the sprite registry left it unset");
|
|
TEST_ASSERT(errctx,
|
|
SDL_GetNumberProperty(AKGL_REGISTRY_SPRITE, "akgl_test_sentinel", 0) == 0,
|
|
"re-initializing the sprite registry kept the old contents");
|
|
|
|
// Configuration set through the registry has to survive and read back,
|
|
// which is the behaviour the missing initializer was breaking.
|
|
CATCH(errctx, akgl_registry_init());
|
|
TEST_EXPECT_OK(errctx, akgl_set_property("test.property", "1234"),
|
|
"setting a property after akgl_registry_init");
|
|
TEST_EXPECT_OK(errctx, akgl_get_property("test.property", &readback, NULL),
|
|
"reading a property back after akgl_registry_init");
|
|
TEST_ASSERT(errctx, strcmp((char *)&readback->data, "1234") == 0,
|
|
"property read back as '%s', expected '1234'",
|
|
(char *)&readback->data);
|
|
} CLEANUP {
|
|
if ( readback != NULL ) {
|
|
IGNORE(akgl_heap_release_string(readback));
|
|
}
|
|
} PROCESS(errctx) {
|
|
} FINISH(errctx, true);
|
|
SUCCEED_RETURN(errctx);
|
|
}
|
|
|
|
int main(void)
|
|
{
|
|
PREPARE_ERROR(errctx);
|
|
ATTEMPT {
|
|
CATCH(errctx, akgl_error_init());
|
|
TEST_TRAP_UNHANDLED_ERRORS();
|
|
CATCH(errctx, test_akgl_registry_init_creation_failures());
|
|
CATCH(errctx, test_akgl_get_property_copies_only_the_value());
|
|
CATCH(errctx, test_actor_state_string_names());
|
|
CATCH(errctx, test_akgl_registry_init_is_repeatable());
|
|
} CLEANUP {
|
|
} PROCESS(errctx) {
|
|
} FINISH_NORETURN(errctx);
|
|
|
|
|
|
}
|