diff --git a/AGENTS.md b/AGENTS.md index 55e575e..6888d6d 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -464,6 +464,47 @@ before it. `-Wstringop-truncation` catches the first form at `-O2`. Nothing catches the `memcpy` form; that one was found by reading its neighbours. +## Reaching For libakstdlib + +`deps/libakstdlib` wraps most of libc so a failure arrives as an +`akerr_ErrorContext *` with context instead of a return value the caller has to +remember to check. Use the wrapper **wherever the underlying call can actually +fail**, which in practice means anything touching a file, a format string, or a +fixed-width destination: + +| Instead of | Use | Because | +|---|---|---| +| `fclose` | `aksl_fclose` | The flush happens here, so a full disk or an exceeded quota is reported *only* here | +| `fgetc` | `aksl_fgetc` | `fgetc(3)` returns `EOF` for both end-of-file and a read error; this raises `AKERR_EOF` and `AKERR_IO` separately | +| `snprintf` | `aksl_snprintf` | Truncation becomes `AKERR_OUTOFBOUNDS` naming both lengths, instead of a silently short string | +| `strncpy` into a fixed field | `aksl_strncpy` | See **Copying Into Fixed-Width Fields** | +| `fopen`/`fread`/`fwrite`/`realpath`/`atof`/`atoi` | the `aksl_` form | Already the convention in this tree | + +**`fclose(3)` disassociates the stream whether or not it succeeds.** Drop the +`FILE *` before you look at the status, or `CLEANUP` closes it a second time: + +```c + tmpfp = fp; + fp = NULL; + CATCH(errctx, aksl_fclose(tmpfp)); +``` + +Written the other way round this aborts in glibc with `double free detected in +tcache`, and only on the path where a close actually fails -- which is to say, +never during development. `/dev/full` is how `tests/game.c` reaches it: writes +to that device are accepted and the `ENOSPC` surfaces at the flush. + +**Do not convert the pure-arithmetic calls.** `memset`, `memcpy`, `strlen`, +`strcmp` and friends can only fail on a NULL argument, and the tree is +inconsistent about them already; see `TODO.md`, "libakstdlib wrappers not yet +adopted", before starting a sweep. + +**Never silence `-Wformat-truncation`.** `include/akgl/error.h` still exports +`DISABLE_GCC_WARNING_FORMAT_TRUNCATION` and nothing uses it. Both sites that +did were genuinely truncating, and both reported the length problem as +something else -- one as a missing texture file, with SDL naming a path the +caller never wrote. `aksl_snprintf` is the answer. + ## Fixing Defects **Read what the code does before deciding what it should do.** Every rule below diff --git a/TODO.md b/TODO.md index d6c013a..2206986 100644 --- a/TODO.md +++ b/TODO.md @@ -1884,6 +1884,50 @@ 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. +## libakstdlib wrappers not yet adopted + +`libakgl` links `libakstdlib` and uses ten of its wrappers. It calls the raw +libc function at a good many more sites. Adopting the four that were carrying a +real defect is done (see the 0.6.0 commit); this is what is left, and the reason +each is left. + +**Adopted, for the record:** `aksl_fclose` (an unchecked close in +`akgl_game_save` lost buffered data silently), `aksl_fgetc` (`fgetc(3)` spells +EOF and a read error the same way), `aksl_snprintf` at two sites that were +truncating under a silenced `-Wformat-truncation`, and `aksl_strncpy` at the two +`src/game.c` sites the fixed-width copy sweep missed. + +### The pure-arithmetic wrappers are deliberately not adopted + +`memset` (38 sites), `strlen` (14), `strcmp` (9), `strncmp` (9), `memcpy` (6), +`memcmp` (2). Every one of these can only fail on a NULL argument, and at all +but a handful of those sites the argument is a stack object whose address cannot +be NULL. `aksl_memset` and `aksl_memcpy` are used twice each, which is the +inconsistency worth naming: there is no rule distinguishing those two sites from +the other 44. + +Converting them costs an `errctx` check per line and buys a NULL check the +compiler already knows is redundant. Not converting them leaves the file mixing +two spellings of the same operation. **Pick one and write it down** -- the +recommendation is to convert only where the argument is a parameter or a heap +pointer, and say so in `AGENTS.md`, rather than either extreme. + +### `DISABLE_GCC_WARNING_FORMAT_TRUNCATION` is now dead + +`include/akgl/error.h` exports it and `RESTORE_GCC_WARNINGS`, and after the +`aksl_snprintf` conversion nothing in the tree uses either. They are public +macros, so removing them is an API change and is not done here. Both suppressed +sites turned out to be real truncations, which is the argument for deleting +rather than keeping them: the two times this library silenced that warning, the +compiler was right. + +### No wrapper is used for the collections + +`aksl_HashMap`, `aksl_List` and `aksl_TreeNode` exist and libakgl uses SDL +property sets for every registry instead. That is not an oversight -- the +registries hand `SDL_PropertiesID` values to callers and SDL owns the lifetime. +Recorded so nobody re-derives it. + ## Arcade physics feel Found by building `tests/physics_sim.c`, which runs the arcade backend as a game diff --git a/deps/libakstdlib b/deps/libakstdlib index f8425b8..669b2b3 160000 --- a/deps/libakstdlib +++ b/deps/libakstdlib @@ -1 +1 @@ -Subproject commit f8425b872909fa2680842cc5d55c355c63bfa7ff +Subproject commit 669b2b395fda52167502e7326b9634bac7658004 diff --git a/include/akgl/error.h b/include/akgl/error.h index 6ea7178..4f19e5c 100644 --- a/include/akgl/error.h +++ b/include/akgl/error.h @@ -27,10 +27,18 @@ #error "libakgl requires libakerror >= 1.0.0: the akerror.h on the include path predates the status registry. Rebuild and reinstall libakerror." #endif -// This macro is used to silence warnings on string concatenation operations that may fail. -// e.g., combining two element of PATH_MAX into a string buffer of AKGL_STRING_MAX_LENGTH. -// We have to draw a line in the sand somewhere or we will just let our buffers grow forever -// to keep the compiler happy. +// Silences -Wformat-truncation on a string concatenation that genuinely may not +// fit -- e.g. joining two PATH_MAX strings into one AKGL_MAX_STRING_LENGTH +// buffer. The line has to be drawn somewhere or the buffers grow forever to +// keep the compiler happy. +// +// **Nothing in libakgl uses these any more.** Both sites -- the path join in +// akgl_path_relative and the tileset image join in +// akgl_tilemap_load_layer_image -- now go through aksl_snprintf, which raises +// AKERR_OUTOFBOUNDS on truncation instead. That is the better answer: the +// compiler was right both times, and silencing it left the truncation +// happening and unreported. Kept because they are public and a consumer may +// have them; see TODO.md. #define DISABLE_GCC_WARNING_FORMAT_TRUNCATION \ _Pragma("GCC diagnostic push") \ _Pragma("GCC diagnostic ignored \"-Wformat-truncation\"") diff --git a/include/akgl/game.h b/include/akgl/game.h index ea199a2..a0dbb58 100644 --- a/include/akgl/game.h +++ b/include/akgl/game.h @@ -208,6 +208,12 @@ void akgl_game_update_fps(void); * @throws ENOENT, EACCES Or whatever else `fopen(3)` reports, with the path in * the message. * @throws AKERR_IO On a stream error or a short write. + * @throws ENOSPC, EDQUOT Or whatever else the close reports. A record this size + * fits entirely in stdio's buffer, so nothing reaches the device until + * the close flushes it and that is the only place a full disk, an + * exceeded quota or a server going away can be seen. The close is + * checked; success here means the bytes are on the device. + * @throws AKERR_OUTOFBOUNDS If a registered name does not fit its field. * * @note This is a partial implementation: the name tables are written but the * objects themselves are not, so the file is not yet enough to restore a @@ -236,6 +242,11 @@ akerr_ErrorContext AKERR_NOIGNORE *akgl_game_save(char *fpath); * the running game, is not valid semver. * @throws AKERR_API If the save file is from a different library version, game * version, game name, or game URI. + * @throws AKERR_IO If anything remains in the file after the four name tables. + * The tables carry no length prefix and end at a zeroed sentinel, so a + * reader whose field widths disagree with the writer's stops early + * inside an entry and would otherwise report success with silently + * wrong maps. Trailing data is the symptom of that disagreement. * * @note Like akgl_game_save, this is partial: it rebuilds the pointer maps but * does not yet read back any objects. The four `SDL_CreateProperties` diff --git a/include/akgl/tilemap.h b/include/akgl/tilemap.h index 06c8ad0..07ae5e3 100644 --- a/include/akgl/tilemap.h +++ b/include/akgl/tilemap.h @@ -601,10 +601,10 @@ akerr_ErrorContext AKERR_NOIGNORE *akgl_tilemap_load_layer_object_actor(akgl_Til * @throws AKGL_ERR_SDL If the image cannot be loaded. The message carries * `SDL_GetError()`. * @throws AKGL_ERR_HEAP If the string pool is exhausted. - * - * @note The joined path is truncated at - * #AKGL_TILEMAP_MAX_TILESET_FILENAME_SIZE without complaint, so an - * over-long path fails as a missing file rather than as a length error. + * @throws AKERR_OUTOFBOUNDS If `dirname` and the image name do not fit in + * #AKGL_TILEMAP_MAX_TILESET_FILENAME_SIZE when joined, naming both + * lengths. This used to truncate silently, so an over-long path failed + * as a missing file rather than as the length error it is. */ akerr_ErrorContext AKERR_NOIGNORE *akgl_tilemap_load_layer_image(akgl_Tilemap *dest, json_t *root, int layerid, akgl_String *dirname); /** diff --git a/src/game.c b/src/game.c index 4458745..404c237 100644 --- a/src/game.c +++ b/src/game.c @@ -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); diff --git a/src/tilemap.c b/src/tilemap.c index 5a346fb..f9ba7d9 100644 --- a/src/tilemap.c +++ b/src/tilemap.c @@ -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; diff --git a/src/util.c b/src/util.c index c63ee99..548c9bc 100644 --- a/src/util.c +++ b/src/util.c @@ -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), diff --git a/tests/game.c b/tests/game.c index 2c97a27..814202b 100644 --- a/tests/game.c +++ b/tests/game.c @@ -154,6 +154,53 @@ akerr_ErrorContext *test_game_save_roundtrip(void) SUCCEED_RETURN(e); } +/** + * @brief A save whose close fails must not report success. + * + * akgl_game_save checks every write, and every one of those writes lands in + * stdio's buffer -- for a record this size, nothing reaches the device until + * the close flushes it. So a full disk, an exceeded quota or a server going + * away is reported *only* by the close, and while that close was unchecked the + * function returned success over a savegame that was never written. + * + * `/dev/full` reproduces that exactly: writes to it are accepted, and the + * ENOSPC surfaces at the flush. Verified against the unfixed code, which + * reported a clean save. + * + * Linux-specific, so it skips rather than fails where the device is not there. + * That is the right trade for a test whose alternative is filling a real + * filesystem. + */ +akerr_ErrorContext *test_game_save_reports_a_failed_close(void) +{ + PREPARE_ERROR(e); + char fullpath[] = "/dev/full"; + FILE *probe = NULL; + + ATTEMPT { + probe = fopen(fullpath, "wb"); + if ( probe == NULL ) { + printf(" skipping the failed-close case: %s is not writable here\n", fullpath); + SUCCEED_BREAK(e); + } + fclose(probe); + probe = NULL; + + CATCH(e, akgl_registry_init()); + CATCH(e, akgl_heap_init()); + set_game_identity(); + + TEST_EXPECT_ANY_ERROR(e, akgl_game_save((char *)&fullpath), + "saving to a device with no room"); + } CLEANUP { + if ( probe != NULL ) { + fclose(probe); + } + } PROCESS(e) { + } FINISH(e, true); + SUCCEED_RETURN(e); +} + akerr_ErrorContext *test_game_load_rejects_foreign_saves(void) { PREPARE_ERROR(e); @@ -736,6 +783,7 @@ int main(void) CATCH(errctx, test_game_load_versioncmp_mismatched()); CATCH(errctx, test_game_load_versioncmp_releases_semver()); CATCH(errctx, test_game_save_roundtrip()); + CATCH(errctx, test_game_save_reports_a_failed_close()); CATCH(errctx, test_game_load_rejects_foreign_saves()); CATCH(errctx, test_game_save_load_nullpointers()); CATCH(errctx, test_game_load_truncated_table()); diff --git a/tests/physics_sim.c b/tests/physics_sim.c index decd760..ad70c17 100644 --- a/tests/physics_sim.c +++ b/tests/physics_sim.c @@ -21,11 +21,17 @@ * failure says *what the motion did*, not merely that a number was wrong. * * @note akgl_physics_simulate reads SDL_GetTicksNS() itself, so it cannot be - * stepped with a caller-supplied dt. These tests set `gravity_time` to - * `now - dt` immediately before each call, which lands within a few - * microseconds of the intended step -- about 0.02% of a 60 Hz frame. - * Assertions carry tolerances to match. That the engine cannot be stepped - * exactly is itself a finding; see TODO.md. + * handed a dt. It can be *bounded* to one, though: sim_step sets + * `max_timestep` to the step it wants and `gravity_time` to zero, so the + * measured interval is always longer than the bound and every step is + * exactly `max_timestep`. That is deterministic under any load, which + * matters -- the first version placed `gravity_time` at `now - dt` and + * let the real clock supply the step, and a machine busy enough to + * deschedule the process between that store and the SDL_GetTicksNS() + * inside simulate produced a longer step and a red suite. It failed + * exactly once, under a parallel ctest, which is the worst way to find + * out. That the engine cannot be stepped exactly is itself a finding; + * see TODO.md. */ #include @@ -64,15 +70,19 @@ static akgl_Character sim_character; /** * @brief Advance the simulation by exactly one step of @p dt seconds. * - * akgl_physics_simulate measures dt as `SDL_GetTicksNS() - self->gravity_time`, - * so the only way to control it from outside is to place `gravity_time` the - * right distance behind the clock immediately before the call. + * akgl_physics_simulate measures dt as `SDL_GetTicksNS() - self->gravity_time` + * and then bounds it by `max_timestep`. Zeroing `gravity_time` makes the + * measured interval the whole uptime of the process, which is always past the + * bound, so the step the simulation takes is `max_timestep` and nothing else. + * The scheduler cannot get in the way of that, which the earlier `now - dt` + * version could not say. */ static akerr_ErrorContext *sim_step(float32_t dt) { PREPARE_ERROR(errctx); - sim_physics.gravity_time = SDL_GetTicksNS() - (SDL_Time)(dt * (float32_t)AKGL_TIME_ONESEC_NS); + sim_physics.max_timestep = (float64_t)dt; + sim_physics.gravity_time = 0; PASS(errctx, sim_physics.simulate(&sim_physics, NULL)); SUCCEED_RETURN(errctx); } diff --git a/tests/tilemap.c b/tests/tilemap.c index 5cf67ae..4e41119 100644 --- a/tests/tilemap.c +++ b/tests/tilemap.c @@ -9,6 +9,7 @@ #include #include #include +#include #include #include "testutil.h" @@ -505,6 +506,52 @@ akerr_ErrorContext *test_akgl_tilemap_load_layer_tile(void) SUCCEED_RETURN(errctx); } +/** + * @brief A layer image path too long for the field must say so, not load the wrong file. + * + * The join is `"%s/%s"` of a directory and an image name into a + * #AKGL_TILEMAP_MAX_TILESET_FILENAME_SIZE buffer, and both inputs are + * #AKGL_MAX_STRING_LENGTH wide, so it can overflow. It used to be a raw + * `snprintf` under a silenced `-Wformat-truncation`: the path was quietly cut + * short and handed to `IMG_LoadTexture`, which failed with SDL's "couldn't + * open" against a filename the caller never wrote -- the length problem + * reported as a missing-file problem, pointing at the wrong thing. + * + * `aksl_snprintf` treats truncation as the error it is. The directory here is + * filled to capacity so the join cannot fit whatever the image name is. + */ +akerr_ErrorContext *test_akgl_tilemap_layer_image_path_too_long(void) +{ + PREPARE_ERROR(errctx); + akgl_String *dirname = NULL; + json_t *layer = NULL; + json_error_t errdata; + + ATTEMPT { + CATCH(errctx, akgl_heap_next_string(&dirname)); + CATCH(errctx, aksl_memset((void *)&dirname->data, 'd', sizeof(dirname->data))); + dirname->data[sizeof(dirname->data) - 1] = '\0'; + + layer = json_loads("{\"image\": \"tiles.png\"}", 0, &errdata); + FAIL_ZERO_BREAK(errctx, layer, AKERR_VALUE, "Failed to build the test layer: %s", errdata.text); + + TEST_EXPECT_STATUS( + errctx, + AKERR_OUTOFBOUNDS, + akgl_tilemap_load_layer_image(akgl_gamemap, layer, 0, dirname), + "joining an image name onto a directory that fills the field"); + } CLEANUP { + if ( dirname != NULL ) { + IGNORE(akgl_heap_release_string(dirname)); + } + if ( layer != NULL ) { + json_decref(layer); + } + } PROCESS(errctx) { + } FINISH(errctx, true); + SUCCEED_RETURN(errctx); +} + akerr_ErrorContext *test_akgl_tilemap_load_layers(void) { akgl_String *pathstr; @@ -720,6 +767,7 @@ int main(void) CATCH(errctx, test_akgl_tilemap_compute_tileset_offsets()); CATCH(errctx, test_akgl_tilemap_load_layer_objects()); CATCH(errctx, test_akgl_tilemap_load_layer_tile()); + CATCH(errctx, test_akgl_tilemap_layer_image_path_too_long()); CATCH(errctx, test_akgl_tilemap_load_layers()); CATCH(errctx, test_akgl_tilemap_load_tilesets()); CATCH(errctx, test_akgl_tilemap_load_bounds_fixed_tables());