diff --git a/TODO.md b/TODO.md index 983559e..0aa58a5 100644 --- a/TODO.md +++ b/TODO.md @@ -718,11 +718,10 @@ Each was found by a test written to assert correct behavior. header documented that as behaviour. It is `AKERR_OUTOFBOUNDS` now, and a negative count is refused too. 7. **Savegame name lengths disagree between writer and reader.** **Fixed in - 0.5.0.** Four `AKGL_GAME_SAVE_*_NAME_WIDTH` constants now drive both sides, - so the two cannot drift again. The writer used each object's own maximum name - length -- 512 for a spritesheet, which is a filename -- and the reader used - `AKGL_ACTOR_MAX_NAME_LENGTH` for all four. The other three are 128 as well, so - only the spritesheet table was wrong. + 0.5.0.** The reader names the same constant the writer does, per table -- + `AKGL_SPRITE_SHEET_MAX_FILENAME_LENGTH` for spritesheets and so on. It used + `AKGL_ACTOR_MAX_NAME_LENGTH` for all four; the other three are 128 as well, + so only the spritesheet table was wrong, and that was enough. **The failure was worse than "cannot be read back", which is what made the test interesting.** The tables carry no length prefix and end at a zeroed @@ -731,16 +730,26 @@ Each was found by a test written to assert correct behavior. success with silently wrong maps. A test that only asserted the load succeeded passed against the broken reader. - So `akgl_game_load` now checks that the stream is at EOF once the four tables - are read. That turns a width disagreement into `AKERR_IO` instead of a - corruption, and it is the assertion `tests/game.c` actually hangs the test - on. The new roundtrip test registers a name in each of the four registries, - with a full-length one in the spritesheet registry, and against a mismatched - reader it fails with `AKERR_IO`. + So `akgl_game_load` checks the stream is at EOF once the four tables are + read. That is what turns a width disagreement into `AKERR_IO` instead of a + corruption, and it is the assertion `tests/game.c` hangs on. The new + roundtrip test registers a name in each of the four registries, with a + full-length one in the spritesheet registry, and fails with `AKERR_IO` + against a mismatched reader. That EOF check has to move when the objects themselves start being written; there is a comment at the site saying so. + A first pass at this introduced four `AKGL_GAME_SAVE_*_NAME_WIDTH` aliases, + one per table, on the reasoning that the on-disk format's widths are a + separate concern from the object model's. They were removed. Each expanded to + exactly one existing constant and had exactly one use per side, so they were + indirection with no second consumer -- and the divergence they anticipated + cannot happen quietly anyway: raising one of those lengths is an ABI change, + which bumps the version, and `akgl_game_load` refuses a save whose + `libversion` does not match before it reads a single table. What actually + guards the widths is the EOF check, which works however they are spelled. + 8. **Heap acquire functions are asymmetric.** `akgl_heap_next_string` increments `refcount`; `next_actor`, `next_sprite`, `next_spritesheet`, and `next_character` do not. `tests/heap.c` pins the current behavior and says so; diff --git a/src/game.c b/src/game.c index e30e08f..ffbfd3d 100644 --- a/src/game.c +++ b/src/game.c @@ -51,34 +51,27 @@ static akerr_ErrorContext *write_exact(const void *ptr, size_t size, size_t nmem return aksl_fwrite(ptr, size, nmemb, fp, &transferred); } -/** - * @brief Field widths of the four savegame name tables. +/* + * The four savegame name tables are written and read at each object's own + * maximum name length -- AKGL_ACTOR_MAX_NAME_LENGTH for actors, + * AKGL_SPRITE_SHEET_MAX_FILENAME_LENGTH for spritesheets, and so on. * - * The writer and the reader have to agree exactly. The tables carry no length - * prefix, so the reader finds each next entry by stepping this many bytes; get - * it wrong and every entry after the first is read out of the middle of its - * neighbour. - * - * They disagreed until 0.5.0. The writer used each object's own maximum name - * length -- 512 for a spritesheet, which is a filename -- and the reader used - * `AKGL_ACTOR_MAX_NAME_LENGTH` for all four. The other three happen to be 128 - * as well, so only the spritesheet table was wrong, and that was enough: a save - * containing any registered spritesheet could not be read back. The roundtrip - * test passed because empty registries write nothing but the zeroed sentinel. - * - * One constant per table, used on both sides, so the two cannot drift again. + * **The writer and the reader have to name the same constant per table.** The + * tables carry no length prefix, so the reader finds each next entry by + * stepping that many bytes. They disagreed until 0.5.0: the writer used each + * object's own length and the reader used AKGL_ACTOR_MAX_NAME_LENGTH for all + * four. The other three are 128 as well, so only the spritesheet table was + * wrong -- and that was enough. See the EOF check at the end of akgl_game_load, + * which is what turns a disagreement into an error instead of a corruption. */ -#define AKGL_GAME_SAVE_ACTOR_NAME_WIDTH AKGL_ACTOR_MAX_NAME_LENGTH -#define AKGL_GAME_SAVE_SPRITE_NAME_WIDTH AKGL_SPRITE_MAX_NAME_LENGTH -#define AKGL_GAME_SAVE_SPRITESHEET_NAME_WIDTH AKGL_SPRITE_SHEET_MAX_FILENAME_LENGTH -#define AKGL_GAME_SAVE_CHARACTER_NAME_WIDTH AKGL_CHARACTER_MAX_NAME_LENGTH /** * @brief Widest name field any of the save tables writes. * - * The spritesheet table is the widest; the other three are half that or less. + * The spritesheet table is the widest at #AKGL_SPRITE_SHEET_MAX_FILENAME_LENGTH; + * the other three are name lengths half that or less. */ -#define AKGL_GAME_SAVE_MAX_NAME_FIELD AKGL_GAME_SAVE_SPRITESHEET_NAME_WIDTH +#define AKGL_GAME_SAVE_MAX_NAME_FIELD AKGL_SPRITE_SHEET_MAX_FILENAME_LENGTH /** * @brief Fail the build if a table ever declares a field wider than the staging buffer. @@ -88,10 +81,10 @@ static akerr_ErrorContext *write_exact(const void *ptr, size_t size, size_t nmem * and turn back into the overread this buffer exists to prevent. */ typedef char akgl_game_save_name_field_fits[ - ((AKGL_GAME_SAVE_ACTOR_NAME_WIDTH <= AKGL_GAME_SAVE_MAX_NAME_FIELD) && - (AKGL_GAME_SAVE_SPRITE_NAME_WIDTH <= AKGL_GAME_SAVE_MAX_NAME_FIELD) && - (AKGL_GAME_SAVE_SPRITESHEET_NAME_WIDTH <= AKGL_GAME_SAVE_MAX_NAME_FIELD) && - (AKGL_GAME_SAVE_CHARACTER_NAME_WIDTH <= AKGL_GAME_SAVE_MAX_NAME_FIELD)) ? 1 : -1]; + ((AKGL_ACTOR_MAX_NAME_LENGTH <= AKGL_GAME_SAVE_MAX_NAME_FIELD) && + (AKGL_SPRITE_MAX_NAME_LENGTH <= AKGL_GAME_SAVE_MAX_NAME_FIELD) && + (AKGL_SPRITE_SHEET_MAX_FILENAME_LENGTH <= AKGL_GAME_SAVE_MAX_NAME_FIELD) && + (AKGL_CHARACTER_MAX_NAME_LENGTH <= AKGL_GAME_SAVE_MAX_NAME_FIELD)) ? 1 : -1]; /** * @brief Write a registry key as a fixed-width, zero-padded field. @@ -321,7 +314,7 @@ static void save_actorname_iterator(void *userdata, SDL_PropertiesID props, cons PREPARE_ERROR(errctx); ATTEMPT { FAIL_ZERO_BREAK(errctx, fp, AKERR_NULLPOINTER, "NULL file pointer"); - CATCH(errctx, write_name_field(name, AKGL_GAME_SAVE_ACTOR_NAME_WIDTH, fp)); + CATCH(errctx, write_name_field(name, AKGL_ACTOR_MAX_NAME_LENGTH, fp)); actor = SDL_GetPointerProperty(props, name, NULL); CATCH(errctx, write_exact(&actor, 1, sizeof(akgl_Actor *), fp)); } CLEANUP { @@ -350,7 +343,7 @@ static void save_spritename_iterator(void *userdata, SDL_PropertiesID props, con ATTEMPT { FAIL_ZERO_BREAK(errctx, fp, AKERR_NULLPOINTER, "NULL file pointer"); sprite = SDL_GetPointerProperty(props, name, NULL); - CATCH(errctx, write_name_field(name, AKGL_GAME_SAVE_SPRITE_NAME_WIDTH, fp)); + CATCH(errctx, write_name_field(name, AKGL_SPRITE_MAX_NAME_LENGTH, fp)); CATCH(errctx, write_exact(&sprite, 1, sizeof(akgl_Sprite *), fp)); } CLEANUP { } PROCESS(errctx) { @@ -382,7 +375,7 @@ static void save_spritesheetname_iterator(void *userdata, SDL_PropertiesID props ATTEMPT { FAIL_ZERO_BREAK(errctx, fp, AKERR_NULLPOINTER, "NULL file pointer"); spritesheet = SDL_GetPointerProperty(props, name, NULL); - CATCH(errctx, write_name_field(name, AKGL_GAME_SAVE_SPRITESHEET_NAME_WIDTH, fp)); + CATCH(errctx, write_name_field(name, AKGL_SPRITE_SHEET_MAX_FILENAME_LENGTH, fp)); CATCH(errctx, write_exact(&spritesheet, 1, sizeof(akgl_SpriteSheet *), fp)); } CLEANUP { } PROCESS(errctx) { @@ -410,7 +403,7 @@ static void save_charactername_iterator(void *userdata, SDL_PropertiesID props, ATTEMPT { FAIL_ZERO_BREAK(errctx, fp, AKERR_NULLPOINTER, "NULL file pointer"); character = SDL_GetPointerProperty(props, name, NULL); - CATCH(errctx, write_name_field(name, AKGL_GAME_SAVE_CHARACTER_NAME_WIDTH, fp)); + CATCH(errctx, write_name_field(name, AKGL_CHARACTER_MAX_NAME_LENGTH, fp)); CATCH(errctx, write_exact(&character, 1, sizeof(akgl_Character *), fp)); } CLEANUP { } PROCESS(errctx) { @@ -641,16 +634,16 @@ akerr_ErrorContext *akgl_game_load(char *fpath) memcpy((void *)&akgl_game, (void *)&savegame, sizeof(akgl_Game)); // Load actor name map actormap = SDL_CreateProperties(); - CATCH(errctx, load_objectnamemap(fp, actormap, AKGL_GAME_SAVE_ACTOR_NAME_WIDTH, sizeof(akgl_Actor *), AKGL_REGISTRY_ACTOR)); + CATCH(errctx, load_objectnamemap(fp, actormap, AKGL_ACTOR_MAX_NAME_LENGTH, sizeof(akgl_Actor *), AKGL_REGISTRY_ACTOR)); // Load sprite name map spritemap = SDL_CreateProperties(); - CATCH(errctx, load_objectnamemap(fp, spritemap, AKGL_GAME_SAVE_SPRITE_NAME_WIDTH, sizeof(akgl_Sprite *), AKGL_REGISTRY_SPRITE)); + CATCH(errctx, load_objectnamemap(fp, spritemap, AKGL_SPRITE_MAX_NAME_LENGTH, sizeof(akgl_Sprite *), AKGL_REGISTRY_SPRITE)); // Load spritesheet name map spritesheetmap = SDL_CreateProperties(); - CATCH(errctx, load_objectnamemap(fp, spritesheetmap, AKGL_GAME_SAVE_SPRITESHEET_NAME_WIDTH, sizeof(akgl_SpriteSheet *), AKGL_REGISTRY_SPRITESHEET)); + CATCH(errctx, load_objectnamemap(fp, spritesheetmap, AKGL_SPRITE_SHEET_MAX_FILENAME_LENGTH, sizeof(akgl_SpriteSheet *), AKGL_REGISTRY_SPRITESHEET)); // Load character name map charactermap = SDL_CreateProperties(); - CATCH(errctx, load_objectnamemap(fp, charactermap, AKGL_GAME_SAVE_CHARACTER_NAME_WIDTH, sizeof(akgl_Character *), AKGL_REGISTRY_CHARACTER)); + CATCH(errctx, load_objectnamemap(fp, charactermap, AKGL_CHARACTER_MAX_NAME_LENGTH, sizeof(akgl_Character *), AKGL_REGISTRY_CHARACTER)); // The four tables are the whole file, so the stream has to be at its end // now. Nothing else checks: the tables carry no length prefix and end at