andrew/libakgl
1
0
Fork 0
Code Issues Pull Requests 1 Actions 1 Packages Projects Releases Wiki Activity

Report the failures libakstdlib's wrappers were already catching

Browse Source 
Updates deps/libakstdlib to 669b2b3. That commit is a TODO entry rather
than new code, so the interesting part is what libakgl was not yet
calling: it links libakstdlib and uses ten of its wrappers while calling
the raw libc function at a good many more sites. Four of those were
carrying a real defect.

akgl_game_save checked every write and threw away the close. Every write
goes through aksl_fwrite, and for a record this size all of them land in
stdio's buffer -- nothing reaches the device until the close flushes it,
so a full disk, an exceeded quota or an NFS server going away is
reported only there. The function returned success over a savegame that
was never written. aksl_fclose surfaces it. tests/game.c reaches the
path through /dev/full, which accepts writes and fails at the flush;
against the unfixed code the test reports "expected a failure, got
success" with ENOSPC.

The close has to drop the FILE * before the status is examined, because
fclose(3) disassociates the stream either way and CLEANUP would close it
again. Written the other way round it aborted in glibc with "double free
detected in tcache" the first time a close actually failed, which is how
that ordering was found.

akgl_game_load's end-of-file check used fgetc(3), which spells "end of
file" and "the read failed" with the same EOF -- a disk error there read
as a clean end and passed the check it was meant to fail. aksl_fgetc
tells them apart. Extracted to require_at_eof, because inverting the
sense of a call inline needs a nested ATTEMPT whose break binds to the
wrong switch.

Two snprintf sites were truncating under a silenced
-Wformat-truncation. The compiler was right about both. The tileset one
handed the shortened path to IMG_LoadTexture, so a length error was
reported as a missing file, with SDL naming a path the caller never
wrote. Both use aksl_snprintf now and the suppression is gone; nothing
in the tree uses those macros any more. tests/tilemap.c covers the join,
and against the unfixed code it gets AKGL_ERR_SDL where it wants
AKERR_OUTOFBOUNDS.

The two src/game.c strncpy sites the fixed-width copy sweep missed --
the savegame name field and the libversion stamp -- use aksl_strncpy and
sizeof rather than a repeated 32.

Also makes tests/physics_sim.c deterministic. It placed gravity_time at
"now - dt" and let the real clock supply the step, so a machine busy
enough to deschedule the process between that store and the
SDL_GetTicksNS() inside simulate got a longer step. It went red once,
under a parallel ctest. It now sets max_timestep to the step it wants
and gravity_time to zero, so the bound supplies the step and the
scheduler cannot reach it.

TODO.md records what is deliberately not adopted: the pure-arithmetic
wrappers, which can only fail on NULL and which the tree already spells
two ways, and the collections, which the registries do not want because
SDL owns the property-set lifetime. AGENTS.md has the rule and the
fclose ordering trap.

26/26 ctest, memcheck clean, warning-clean at -Wall -Werror.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01B8T5FAYXE8HEJqFLCYwNNc
This commit is contained in:
Andrew Kesterson
2026-08-01 12:36:36 -04:00
parent 147ab7dc78
commit 1b90f2ebd5
12 changed files with 347 additions and 51 deletions
Download Patch File Download Diff File Expand all files Collapse all files

97
src/game.c
View File

@@ -126,7 +126,7 @@ static akerr_ErrorContext *write_name_field(const char *name, size_t width, FILE
sizeof(field));
memset(&field, 0x00, sizeof(field));
strncpy((char *)&field, name, (width - 1));
PASS(errctx, aksl_strncpy((char *)&field, sizeof(field), name, (width - 1)));
PASS(errctx, write_exact((char *)&field, 1, width, fp));
SUCCEED_RETURN(errctx);
}
@@ -138,6 +138,48 @@ static akerr_ErrorContext *read_exact(void *ptr, size_t size, size_t nmemb, FILE
return aksl_fread(ptr, size, nmemb, fp, &transferred);
}
/**
* @brief Require that @p fp has nothing left in it.
*
* A helper rather than three lines at the call site, because "read a byte and
* succeed only if that fails in one specific way" inverts the sense of every
* other call in this file, and doing it inline needs a nested `ATTEMPT` whose
* `break` binds to the wrong `switch`.
*
* `fgetc(3)` is not used here. It spells "end of file" and "the read failed"
* with the same `EOF`, so a disk error at this point read as a clean end and
* passed the check it was supposed to fail. `aksl_fgetc` tells the two apart:
* #AKERR_EOF is the outcome this wants and is the only one swallowed.
*
* @param fp Open input stream. Required.
* @return `NULL` at end of file, otherwise an error context owned by the caller.
* @throws AKERR_NULLPOINTER If @p fp is `NULL`.
* @throws AKERR_IO If a byte was read, naming it.
* @throws Whatever `aksl_fgetc` raises on a read error.
*/
static akerr_ErrorContext *require_at_eof(FILE *fp)
{
int trailing = 0;
PREPARE_ERROR(errctx);
FAIL_ZERO_RETURN(errctx, fp, AKERR_NULLPOINTER, "NULL file pointer");
ATTEMPT {
CATCH(errctx, aksl_fgetc(fp, &trailing));
FAIL_BREAK(
errctx,
AKERR_IO,
"Savegame has data left after its name tables (first trailing byte 0x%02x); "
"a name field width does not match the writer's",
(trailing & 0xFF));
} CLEANUP {
} PROCESS(errctx) {
} HANDLE(errctx, AKERR_EOF) {
// The whole point: nothing left to read is what this asks for.
} FINISH(errctx, true);
SUCCEED_RETURN(errctx);
}
void akgl_game_lowfps(void)
{
SDL_Log("Low FPS! %d", akgl_game.fps);
@@ -152,7 +194,11 @@ akerr_ErrorContext *akgl_game_init(void)
// AKGL_ERR_* codes, and a code raised before its name is registered prints
// as "Unknown Error" in the stack trace the caller is left holding.
PASS(errctx, akgl_error_init());
strncpy((char *)&akgl_game.libversion, AKGL_VERSION, 32);
PASS(errctx, aksl_strncpy(
(char *)&akgl_game.libversion,
sizeof(akgl_game.libversion),
AKGL_VERSION,
sizeof(akgl_game.libversion) - 1));
akgl_game.gameStartTime = SDL_GetTicksNS();
akgl_game.lastIterTime = akgl_game.gameStartTime;
akgl_game.lastFPSTime = akgl_game.gameStartTime;
@@ -463,6 +509,7 @@ akerr_ErrorContext *akgl_game_save_actors(FILE *fp)
akerr_ErrorContext *akgl_game_save(char *fpath)
{
FILE *fp = NULL;
FILE *tmpfp = NULL;
PREPARE_ERROR(errctx);
ATTEMPT {
@@ -470,13 +517,34 @@ akerr_ErrorContext *akgl_game_save(char *fpath)
CATCH(errctx, aksl_fopen(fpath, "wb", &fp));
CATCH(errctx, write_exact(&akgl_game, 1, sizeof(akgl_Game), fp));
CATCH(errctx, akgl_game_save_actors(fp));
// Closed here rather than in CLEANUP, because on this path the close is
// part of the save. Every write above goes through aksl_fwrite and is
// checked; the flush that those writes were buffered for happens in the
// close, so a full disk, an exceeded quota or an NFS server going away
// is reported *only* here. Ignoring it means akgl_game_save returns
// success over a truncated savegame.
//
// The order matters: fclose(3) disassociates the stream whether or not
// it succeeds, so `fp` is dead either way and has to be dropped
// *before* the status is examined. Closing it again in CLEANUP is
// undefined behaviour that no wrapper can detect -- written the other
// way round, this aborted in glibc with "double free detected in
// tcache" the first time a close actually failed.
tmpfp = fp;
fp = NULL;
CATCH(errctx, aksl_fclose(tmpfp));
} CLEANUP {
// CLEANUP must precede PROCESS: with the two transposed, the fclose
// lands inside the PROCESS switch and only runs when an error context
// exists and reports success, so an ordinary save never flushed or
// closed its stream.
if ( fp != NULL )
fclose(fp);
//
// Only the failure path reaches this with a stream still open, and it
// is already carrying an error the caller needs more than a close
// status, so the descriptor is released without asking how it went.
if ( fp != NULL ) {
IGNORE(aksl_fclose(fp));
}
} PROCESS(errctx) {
} FINISH(errctx, true);
SUCCEED_RETURN(errctx); // SUCCEED_NORETURN if in main().
@@ -522,6 +590,7 @@ static akerr_ErrorContext *load_objectnamemap(FILE *fp, SDL_PropertiesID map, in
{
void *ptr = NULL;
char ptrstring[32];
int ptrstringlen = 0;
char objname[namelength];
bool done = false;
@@ -548,8 +617,13 @@ static akerr_ErrorContext *load_objectnamemap(FILE *fp, SDL_PropertiesID map, in
// SDL_Properties objects can only use string keys, so we can't use the
// old pointer as a key without first converting it to a string.
CATCH(errctx, aksl_memset((void *)&ptrstring, 0x00, 32));
snprintf((char *)&ptrstring, 32, "%p", ptr);
CATCH(errctx, aksl_memset((void *)&ptrstring, 0x00, sizeof(ptrstring)));
CATCH(errctx, aksl_snprintf(
&ptrstringlen,
(char *)&ptrstring,
sizeof(ptrstring),
"%p",
ptr));
SDL_SetPointerProperty(
map,
ptrstring,
@@ -655,16 +729,15 @@ akerr_ErrorContext *akgl_game_load(char *fpath)
//
// It also has to move once the objects themselves are written; the
// comment below is the marker for that.
FAIL_NONZERO_BREAK(
errctx,
(fgetc(fp) != EOF),
AKERR_IO,
"Savegame has data left after its name tables; a name field width does not match the writer's");
CATCH(errctx, require_at_eof(fp));
// Now that we have all of our pointer maps built, we can load the actual binary objects and reset their pointers
} CLEANUP {
// Nothing was written, so unlike akgl_game_save there is no buffered
// data a failed close could lose. Releasing the descriptor is all this
// has to do.
if ( fp != NULL ) {
fclose(fp);
IGNORE(aksl_fclose(fp));
}
} PROCESS(errctx) {
} FINISH(errctx, true);

26
src/tilemap.c
View File

@@ -424,6 +424,7 @@ akerr_ErrorContext *akgl_tilemap_load_layer_image(akgl_Tilemap *dest, json_t *ro
PREPARE_ERROR(errctx);
akgl_String *tmpstr;
akgl_String *fpath;
int fpathlen = 0;
FAIL_ZERO_RETURN(errctx, dest, AKERR_NULLPOINTER, "NULL destination tilemap reference");
FAIL_ZERO_RETURN(errctx, root, AKERR_NULLPOINTER, "NULL tilemap root reference");
@@ -434,16 +435,23 @@ akerr_ErrorContext *akgl_tilemap_load_layer_image(akgl_Tilemap *dest, json_t *ro
CATCH(errctx, akgl_heap_next_string(&fpath));
CATCH(errctx, akgl_get_json_string_value(root, "image", &tmpstr));
DISABLE_GCC_WARNING_FORMAT_TRUNCATION
snprintf((char *)&fpath->data,
AKGL_TILEMAP_MAX_TILESET_FILENAME_SIZE,
"%s/%s",
dirname->data,
tmpstr->data
);
RESTORE_GCC_WARNINGS
// aksl_snprintf rather than snprintf(3) under a silenced
// -Wformat-truncation: the compiler was right that "%s/%s" of two
// AKGL_MAX_STRING_LENGTH strings does not fit, and silencing it left
// the truncation happening and unreported. What followed was
// IMG_LoadTexture on a path that was almost the right one, failing with
// SDL's "couldn't open" against a filename the caller never wrote.
// aksl_snprintf raises AKERR_OUTOFBOUNDS naming both lengths instead.
CATCH(errctx, aksl_snprintf(
&fpathlen,
(char *)&fpath->data,
AKGL_TILEMAP_MAX_TILESET_FILENAME_SIZE,
"%s/%s",
dirname->data,
tmpstr->data
));
dest->layers[layerid].texture = IMG_LoadTexture(akgl_renderer->sdl_renderer, (char *)fpath->data);
dest->layers[layerid].texture = IMG_LoadTexture(akgl_renderer->sdl_renderer, (char *)fpath->data);
FAIL_ZERO_BREAK(errctx, dest->layers[layerid].texture, AKGL_ERR_SDL, "%s", SDL_GetError());
dest->layers[layerid].width = dest->layers[layerid].texture->w;
dest->layers[layerid].height = dest->layers[layerid].texture->h;

29
src/util.c
View File

@@ -74,17 +74,15 @@ static akerr_ErrorContext *path_relative_root(char *root, char *path, akgl_Strin
if ( (rootlen + pathlen) >= AKGL_MAX_STRING_LENGTH ) {
FAIL_BREAK(errctx, 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(errctx, aksl_snprintf(
&count,
(char *)&pathbuf->data,
sizeof(pathbuf->data),
"%s/%s",
root,
path
));
RESTORE_GCC_WARNINGS
CATCH(errctx, aksl_realpath((char *)&pathbuf->data, (char *)&strbuf->data, sizeof(strbuf->data)));
CATCH(errctx, aksl_snprintf(
&count,
(char *)&pathbuf->data,
sizeof(pathbuf->data),
"%s/%s",
root,
path
));
CATCH(errctx, aksl_realpath((char *)&pathbuf->data, (char *)&strbuf->data, sizeof(strbuf->data)));
CATCH(errctx, akgl_string_copy(strbuf, dest, 0));
} CLEANUP {
IGNORE(akgl_heap_release_string(strbuf));
@@ -260,6 +258,7 @@ akerr_ErrorContext *akgl_render_and_compare(SDL_Texture *t1, SDL_Texture *t2, in
SDL_FRect dest = {.x = x, .y = y, .w = w, .h = h};
SDL_Rect read = {.x = x, .y = y, .w = w, .h = h};
akgl_String *tmpstring = NULL;
int count = 0;
PREPARE_ERROR(errctx);
ATTEMPT {
@@ -273,7 +272,13 @@ akerr_ErrorContext *akgl_render_and_compare(SDL_Texture *t1, SDL_Texture *t2, in
FAIL_ZERO_BREAK(errctx, s1, AKGL_ERR_SDL, "Failed to read pixels from renderer");
if ( writeout != NULL ) {
snprintf((char *)&tmpstring->data, AKGL_MAX_STRING_LENGTH, "%s%s", SDL_GetBasePath(), writeout);
CATCH(errctx, aksl_snprintf(
&count,
(char *)&tmpstring->data,
sizeof(tmpstring->data),
"%s%s",
SDL_GetBasePath(),
writeout));
FAIL_ZERO_BREAK(
errctx,
IMG_SavePNG(s1, (char *)&tmpstring->data),