diff --git a/CMakeLists.txt b/CMakeLists.txt index 05075d0..632303f 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -2,7 +2,7 @@ cmake_minimum_required(VERSION 3.10) # The single source of truth for the library version. It drives the generated # include/akgl/version.h, the shared library's VERSION and SOVERSION, and the # Version field in akgl.pc. Bump it here and nowhere else. -project(akgl VERSION 0.3.0 LANGUAGES C) +project(akgl VERSION 0.4.0 LANGUAGES C) # Memory checking reuses the suites that already exist -- `ctest -T memcheck` # runs every registered test under valgrind -- rather than adding programs of its diff --git a/TODO.md b/TODO.md index 39c8931..5bacb69 100644 --- a/TODO.md +++ b/TODO.md @@ -1126,8 +1126,15 @@ nothing about this library. ### Defects the memory checker found -Ordered by blast radius. Numbering continues the lists above. Every size below -is measured, not estimated. +**All six are fixed.** They are kept here rather than deleted because the sizes +are measured and the reasoning is worth having next time somebody asks why a +loader ends in a `CLEANUP` block, or why a name field is staged through a zeroed +buffer. `cmake --build build --target memcheck` is clean, and the CI job that +runs it gates: the next one of these fails the build on the push that +introduces it. + +Ordered by blast radius as they were found. Numbering continues the lists above. +Every size below is measured, not estimated. 34. **Every JSON loader leaks its parsed document.** There are four `json_load_file` calls in `src/` and not one `json_decref` anywhere in the @@ -1147,11 +1154,14 @@ is measured, not estimated. death leaks a level's worth of JSON each time, and this is the one item in this list that grows without bound. - Fix: `json_decref(json)` in the `CLEANUP` block of each, which is where the - other resources are already released. It is four lines. What makes it worth - its own commit rather than a footnote is that each one needs a test proving - the document is released, and `akgl_registry_load_properties` has no test at - all yet. + **Fixed.** `json_decref` in the `CLEANUP` block of each, on the success + path as well as the failure one, with the handle nulled after so a second + pass cannot double-release. `akgl_registry_load_properties` needed its loop + moved inside the `ATTEMPT` block first: `props` is a borrowed reference into + the document and was read after the block ended, so every exit from that + loop leaked the document. The test is the memcheck run -- nothing in the + public API can observe a jansson refcount, and inventing a hook to prove it + would be testing the test. 35. **`akgl_get_property` reads up to 4 KiB past the end of the property value.** `src/registry.c:181` copies a fixed `AKGL_MAX_STRING_LENGTH` bytes @@ -1167,11 +1177,14 @@ is measured, not estimated. SDL's heap and returns garbage past the terminator, and on a value that lands at the end of a page it is a segfault in a getter. - Fix: bound the copy by `strlen` of the source, still capped at - `AKGL_MAX_STRING_LENGTH`. Touches `akgl_get_property` only, and the header - note becomes a description of correct behaviour instead of a confession. - Worth a test that stores a short property, reads it back, and asserts the - bytes after the terminator in the destination are untouched. + **Fixed.** The copy is bounded by the value's own length plus its + terminator, and a value too long for an akgl_String is now refused with + AKERR_OUTOFBOUNDS instead of silently truncated. The documented + AKERR_NULLPOINTER for "unset with no default" is unchanged -- it is raised + deliberately now rather than arriving from inside `aksl_memcpy`. + `tests/registry.c` fills the destination with a sentinel, reads a + three-byte property back, and asserts every byte past the terminator still + holds the sentinel; that assertion fails against the old code. 36. **The savegame name tables read past the end of every registry key, and write what they find into the file.** `akgl_game_save_actorname_iterator` @@ -1189,9 +1202,13 @@ is measured, not estimated. registered object — anything that happened to be next to the key. That is a file a player might send someone. - Fix: copy the name into a zeroed fixed-width buffer and write that. It - pairs naturally with **Defects -> Known and still open** item 7, which is - the same tables disagreeing about their widths between writer and reader. + **Fixed.** All four iterators now write through `write_name_field`, which + stages the key into a zeroed fixed-width buffer, so the padding is + deterministic and nothing but the name leaves the process. A + negative-array-size typedef fails the build if any of the four widths ever + outgrows the staging buffer. Still open and unrelated to the overread: + **Defects -> Known and still open** item 7, the same tables disagreeing + about their widths between writer and reader. 37. **`akgl_controller_list_keyboards` leaks the array SDL gives it.** `src/controller.c:188` calls `SDL_GetKeyboards`, which allocates, and never @@ -1199,10 +1216,10 @@ is measured, not estimated. — one keyboard id — but it is per call, and the function is shaped like something a game calls when a device is hotplugged. - Fix: one `SDL_free`, in a `CLEANUP` block so the early-return path is - covered too. The same question should be asked of every SDL enumeration in - `src/controller.c`; `SDL_GetGamepads` has the same contract and the dummy - driver reports no gamepads, so no test reaches it. + **Fixed.** One `SDL_free`, in a `CLEANUP` block so a failure inside the + loop cannot take the array with it. Still worth asking the same question of + every SDL enumeration in `src/controller.c`: `SDL_GetGamepads` has the same + contract, and the dummy driver reports no gamepads, so no test reaches it. 38. **A font, once loaded, is never freed and cannot be.** `akgl_text_loadfont` (`src/text.c:20`) opens a `TTF_Font`, puts the pointer @@ -1212,19 +1229,36 @@ is measured, not estimated. them SDL_ttf's, the rest FreeType's. This is bounded by how many fonts a game loads, so it is not the runaway - that item 34 is. It is still a gap in the API rather than only a leak: a - game that switches fonts between scenes, or a tool like `charviewer` that - reloads one while the user picks a size, has no way to give the old one - back. Fix: `akgl_text_unloadfont(char *name)` that clears the registry entry - and calls `TTF_CloseFont`, and a matching sweep at shutdown. + that item 34 is. It was a gap in the API rather than only a leak: a game + that switches fonts between scenes, or a tool like `charviewer` that reloads + one while the user picks a size, had no way to give the old one back. + + **Fixed**, and this one is a new public symbol rather than a repair: + `akgl_text_unloadfont(char *name)` clears the registry entry and closes the + font. `akgl_text_loadfont` calls it when it replaces a live name, which + closes the second leak the header used to document as intended behaviour -- + but only after the new font has opened, so a failed reload leaves the caller + with the font they already had. The version goes to 0.4.0 with it: an 0.3 + consumer cannot be handed this library and told it is the same ABI. + + Still open: nothing closes the registry's remaining fonts at shutdown. A + game that exits without unloading leaks them exactly once, which valgrind + reports against the process rather than against a loop, and which + `akgl_game_shutdown` would be the natural home for if it existed. 39. **Vendored `deps/semver`'s own unit test leaks 188 bytes** across 16 blocks, from the `calloc`s in `test_strcut_first` and `test_strcut_second` (`deps/semver/semver_unit.c:8` and `:21`). Not libakgl's code and not libakgl's to fix; recorded so that nobody re-diagnoses it, and because `semver_unit` is registered as one of our CTest tests and so shows up in our - memcheck run. If it becomes noise, the answer is a suppression naming those - two functions, not a local edit to a vendored file. + memcheck run. + + **Suppressed**, which is what this list said the answer would be if it ever + became noise, and gating the job made it noise. The two entries in + `scripts/valgrind.supp` name the functions rather than the file, so a + rewrite of those cases stops being suppressed and comes back as a finding. + They are the only entries there that hide a real leak in a program this + build runs; the alternative was a fork of a vendored dependency. ### Not defects, and why diff --git a/include/akgl/registry.h b/include/akgl/registry.h index 71f0869..3452618 100644 --- a/include/akgl/registry.h +++ b/include/akgl/registry.h @@ -204,13 +204,14 @@ akerr_ErrorContext AKERR_NOIGNORE *akgl_set_property(char *name, char *value); * @return `NULL` on success, otherwise an error context owned by the caller. * @throws AKERR_NULLPOINTER If @p name or @p dest is `NULL`, or if the property * is unset and @p def is `NULL`. + * @throws AKERR_OUTOFBOUNDS If the stored value is #AKGL_MAX_STRING_LENGTH bytes + * or longer, which will not fit an akgl_String with its terminator. * @throws AKERR_VALUE If the value and the destination overlap in memory. * @throws AKGL_ERR_HEAP If `*dest` was `NULL` and the string pool is exhausted. * - * @note The copy is a fixed #AKGL_MAX_STRING_LENGTH bytes rather than the length - * of the value, so a short property value is read past its end. It works - * because the destination is a full-sized pool string; it is still an - * overread of the source. + * @note Only the value and its terminator are copied. The rest of @p dest keeps + * whatever the previous holder left there, so read the result as a C + * string rather than as #AKGL_MAX_STRING_LENGTH bytes. */ akerr_ErrorContext AKERR_NOIGNORE *akgl_get_property(char *name, akgl_String **dest, char *def); diff --git a/include/akgl/text.h b/include/akgl/text.h index 1242924..7849a7d 100644 --- a/include/akgl/text.h +++ b/include/akgl/text.h @@ -29,7 +29,9 @@ * * @param name Registry key to publish the font under. Required. An existing * entry with the same name is replaced, and the font it - * displaced is leaked rather than closed. + * displaced is closed through akgl_text_unloadfont -- but only + * after the new one has opened, so a failed load leaves the + * caller with the font they already had. * @param filepath Path to a `.ttf`/`.otf` file. Required. Used verbatim -- not * resolved against `SDL_GetBasePath()`. * @param size Point size to rasterize at. Passed straight to SDL_ttf, which @@ -43,6 +45,30 @@ * in practice, because akgl_registry_init has not run. */ akerr_ErrorContext AKERR_NOIGNORE *akgl_text_loadfont(char *name, char *filepath, int size); +/** + * @brief Close a loaded font and take it out of the font registry. + * + * The other half of akgl_text_loadfont, and for a while the half that did not + * exist: a font could be opened and published but never handed back, so a game + * that changed fonts between scenes had no way to reclaim the one it had + * finished with. A `TTF_Font` is about ten kilobytes once FreeType's own + * structures are counted. + * + * The registry entry is cleared before the font is closed, so a font is never + * reachable through #AKGL_REGISTRY_FONT after it has gone back to SDL_ttf. + * + * @param name Registry key the font was published under. Required. + * @return `NULL` on success, otherwise an error context owned by the caller. + * @throws AKERR_NULLPOINTER If @p name is `NULL`. + * @throws AKERR_KEY If no font is registered under @p name -- including the + * case where it was already unloaded, which makes a double unload an + * error rather than a double close. + * + * @warning Anything still holding the `TTF_Font *` -- a caller that fetched it + * from the registry earlier, a pending akgl_text_rendertextat -- is + * left with a dangling pointer. Fonts are not reference counted. + */ +akerr_ErrorContext AKERR_NOIGNORE *akgl_text_unloadfont(char *name); /** * @brief Rasterize a string and blit it at a screen position, in one call. * diff --git a/scripts/valgrind.supp b/scripts/valgrind.supp index f31a7a2..5a23946 100644 --- a/scripts/valgrind.supp +++ b/scripts/valgrind.supp @@ -54,3 +54,32 @@ ... fun:FT_Init_FreeType } + +# deps/semver's own unit test leaks the buffers it cuts strings into -- +# semver_unit.c:8 and :21, one calloc each per case. Vendored code, and its test +# program at that: not ours to edit, and not worth carrying a local patch for. +# Named by function rather than by file so that a rewrite of those cases stops +# being suppressed and comes back as a finding. +# +# This is the only entry here that hides a real leak in a program this build +# runs. It is bounded, it is in a test, and the alternative is either a fork of +# a vendored dependency or a memcheck run that is always red -- see TODO.md, +# "Memory checking" item 39. +{ + vendored-semver-unit-test-strcut-first + Memcheck:Leak + match-leak-kinds: definite + fun:calloc + fun:test_strcut_first + fun:test_strcut + fun:main +} +{ + vendored-semver-unit-test-strcut-second + Memcheck:Leak + match-leak-kinds: definite + fun:calloc + fun:test_strcut_second + fun:test_strcut + fun:main +} diff --git a/src/character.c b/src/character.c index 549effc..115b6a6 100644 --- a/src/character.c +++ b/src/character.c @@ -218,7 +218,7 @@ static akerr_ErrorContext *akgl_character_load_json_inner(json_t *json, akgl_Cha akerr_ErrorContext *akgl_character_load_json(char *filename) { PREPARE_ERROR(errctx); - json_t *json; + json_t *json = NULL; json_error_t error; akgl_Character *obj = NULL; //akgl_String *tmpstr = NULL; @@ -245,6 +245,13 @@ akerr_ErrorContext *akgl_character_load_json(char *filename) CATCH(errctx, akgl_get_json_number_value(json, "acceleration_y", &obj->ay)); } CLEANUP { //IGNORE(akgl_heap_release_string(tmpstr)); + // The character keeps nothing that points into the document -- every + // field above is copied out of it -- so the whole parsed tree goes back + // here, on the success path as well as the failure one. + if ( json != NULL ) { + json_decref(json); + json = NULL; + } if ( errctx != NULL ) { if ( errctx->status != 0 ) { IGNORE(akgl_heap_release_character(obj)); diff --git a/src/controller.c b/src/controller.c index 9485482..b4e9e67 100644 --- a/src/controller.c +++ b/src/controller.c @@ -190,10 +190,19 @@ akerr_ErrorContext AKERR_NOIGNORE *akgl_controller_list_keyboards(void) FAIL_ZERO_RETURN(e, keyboards, AKERR_NULLPOINTER, "%s", SDL_GetError()); - for (int i = 0; i < count; i++) { - const char *name = SDL_GetKeyboardNameForID(keyboards[i]); - SDL_Log("Keyboard %d: ID %u, Name: %s\n", i, keyboards[i], name); - } + // The array is SDL's to allocate and ours to free -- that is the contract on + // every SDL_Get*s() enumeration. Released in CLEANUP so the loop can fail + // without taking the array with it. + ATTEMPT { + for (int i = 0; i < count; i++) { + const char *name = SDL_GetKeyboardNameForID(keyboards[i]); + SDL_Log("Keyboard %d: ID %u, Name: %s\n", i, keyboards[i], name); + } + } CLEANUP { + SDL_free(keyboards); + keyboards = NULL; + } PROCESS(e) { + } FINISH(e, true); SUCCEED_RETURN(e); } diff --git a/src/game.c b/src/game.c index cf3418c..de5a1ad 100644 --- a/src/game.c +++ b/src/game.c @@ -50,6 +50,72 @@ static akerr_ErrorContext *write_exact(const void *ptr, size_t size, size_t nmem return aksl_fwrite(ptr, size, nmemb, fp, &transferred); } +/** + * @brief Widest name field any of the save tables writes. + * + * 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_SPRITE_SHEET_MAX_FILENAME_LENGTH + +/** + * @brief Fail the build if a table ever declares a field wider than the staging buffer. + * + * A negative array size is a compile error, so widening one of these constants + * without widening #AKGL_GAME_SAVE_MAX_NAME_FIELD cannot get past the compiler + * and turn back into the overread this buffer exists to prevent. + */ +typedef char akgl_game_save_name_field_fits[ + ((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_SPRITE_MAX_CHARACTER_NAME_LENGTH <= AKGL_GAME_SAVE_MAX_NAME_FIELD)) ? 1 : -1]; + +/** + * @brief Write a registry key as a fixed-width, zero-padded field. + * + * The name tables are read back without a length prefix, so every entry has to + * occupy exactly @p width bytes. Writing @p width bytes *from the key itself* + * is what the four iterators used to do, and the key is an SDL allocation sized + * to the name -- 40 bytes for `benchactor0`. That read past the end of it on + * every entry, and put whatever was next to it in the heap into the save file: + * up to half a kilobyte of this process's memory per registered object, in a + * file a player might send to somebody else. + * + * Staging through a zeroed buffer costs one `memset` per entry and makes the + * padding deterministic, which the reader wanted anyway. + * + * @param name The registry key. Required, and NUL-terminated -- it comes from + * SDL's property table, which stores C strings. + * @param width Field width in bytes, including the padding. Required to be at + * most #AKGL_GAME_SAVE_MAX_NAME_FIELD; the compile-time check + * above is what keeps that true for the four call sites. + * @param fp Open output stream. Required. + * @return `NULL` on success, otherwise an error context owned by the caller. + * @throws AKERR_NULLPOINTER If @p name or @p fp is `NULL`. + * @throws AKERR_OUTOFBOUNDS If @p width exceeds the staging buffer. + */ +static akerr_ErrorContext *write_name_field(const char *name, size_t width, FILE *fp) +{ + char field[AKGL_GAME_SAVE_MAX_NAME_FIELD]; + + PREPARE_ERROR(e); + FAIL_ZERO_RETURN(e, name, AKERR_NULLPOINTER, "NULL name"); + FAIL_ZERO_RETURN(e, fp, AKERR_NULLPOINTER, "NULL file pointer"); + FAIL_NONZERO_RETURN( + e, + (width > sizeof(field)), + AKERR_OUTOFBOUNDS, + "Name field width %zu exceeds the staging buffer (%zu)", + width, + sizeof(field)); + + memset(&field, 0x00, sizeof(field)); + strncpy((char *)&field, name, (width - 1)); + PASS(e, write_exact((char *)&field, 1, width, fp)); + SUCCEED_RETURN(e); +} + static akerr_ErrorContext *read_exact(void *ptr, size_t size, size_t nmemb, FILE *fp) { size_t transferred; @@ -216,7 +282,7 @@ void akgl_game_save_actorname_iterator(void *userdata, SDL_PropertiesID props, c PREPARE_ERROR(e); ATTEMPT { FAIL_ZERO_BREAK(e, fp, AKERR_NULLPOINTER, "NULL file pointer"); - CATCH(e, write_exact((char *)name, 1, AKGL_ACTOR_MAX_NAME_LENGTH, fp)); + CATCH(e, write_name_field(name, AKGL_ACTOR_MAX_NAME_LENGTH, fp)); actor = SDL_GetPointerProperty(props, name, NULL); CATCH(e, write_exact(&actor, 1, sizeof(akgl_Actor *), fp)); } CLEANUP { @@ -245,7 +311,7 @@ void akgl_game_save_spritename_iterator(void *userdata, SDL_PropertiesID props, ATTEMPT { FAIL_ZERO_BREAK(e, fp, AKERR_NULLPOINTER, "NULL file pointer"); sprite = SDL_GetPointerProperty(props, name, NULL); - CATCH(e, write_exact((char *)name, 1, AKGL_SPRITE_MAX_NAME_LENGTH, fp)); + CATCH(e, write_name_field(name, AKGL_SPRITE_MAX_NAME_LENGTH, fp)); CATCH(e, write_exact(&sprite, 1, sizeof(akgl_Sprite *), fp)); } CLEANUP { } PROCESS(e) { @@ -277,7 +343,7 @@ void akgl_game_save_spritesheetname_iterator(void *userdata, SDL_PropertiesID pr ATTEMPT { FAIL_ZERO_BREAK(e, fp, AKERR_NULLPOINTER, "NULL file pointer"); spritesheet = SDL_GetPointerProperty(props, name, NULL); - CATCH(e, write_exact((char *)name, 1, AKGL_SPRITE_SHEET_MAX_FILENAME_LENGTH, fp)); + CATCH(e, write_name_field(name, AKGL_SPRITE_SHEET_MAX_FILENAME_LENGTH, fp)); CATCH(e, write_exact(&spritesheet, 1, sizeof(akgl_SpriteSheet *), fp)); } CLEANUP { } PROCESS(e) { @@ -305,7 +371,7 @@ void akgl_game_save_charactername_iterator(void *userdata, SDL_PropertiesID prop ATTEMPT { FAIL_ZERO_BREAK(e, fp, AKERR_NULLPOINTER, "NULL file pointer"); character = SDL_GetPointerProperty(props, name, NULL); - CATCH(e, write_exact((char *)name, 1, AKGL_SPRITE_MAX_CHARACTER_NAME_LENGTH, fp)); + CATCH(e, write_name_field(name, AKGL_SPRITE_MAX_CHARACTER_NAME_LENGTH, fp)); CATCH(e, write_exact(&character, 1, sizeof(akgl_Character *), fp)); } CLEANUP { } PROCESS(e) { diff --git a/src/registry.c b/src/registry.c index 1607b01..10bbefc 100644 --- a/src/registry.c +++ b/src/registry.c @@ -4,6 +4,7 @@ */ #include +#include #include #include @@ -129,6 +130,13 @@ akerr_ErrorContext AKERR_NOIGNORE *akgl_registry_load_properties(char *fname) PREPARE_ERROR(errctx); FAIL_ZERO_RETURN(errctx, fname, AKERR_NULLPOINTER, "null filename"); + // One ATTEMPT around the whole function, rather than one for the parse and + // a bare loop after it. `props` is a borrowed reference into `json` and is + // read by the loop, so the document cannot be released until the loop is + // done -- and with the loop outside the block, every path out of it leaked + // the document. The per-property ATTEMPT stays nested inside the loop, + // which is what AGENTS.md requires: a CATCH written directly in a loop + // breaks the loop rather than leaving the function. ATTEMPT { SDL_Log("Loading from %s", fname); json = json_load_file(fname, 0, &error); @@ -141,21 +149,28 @@ akerr_ErrorContext AKERR_NOIGNORE *akgl_registry_load_properties(char *fname) error.line, error.text); CATCH(errctx, akgl_get_json_object_value(json, "properties", &props)); + + json_object_foreach(props, pkey, pvalue) { + ATTEMPT { + CATCH(errctx, akgl_heap_next_string(&tmpstr)); + CATCH(errctx, akgl_get_json_string_value(props, (char *)pkey, &tmpstr)); + SDL_SetStringProperty(AKGL_REGISTRY_PROPERTIES, pkey, tmpstr->data); + SDL_Log("Set property %s = %s", pkey, tmpstr->data); + CATCH(errctx, akgl_heap_release_string(tmpstr)); + } CLEANUP { + } PROCESS(errctx) { + } FINISH(errctx, true); + } } CLEANUP { + // Every value was copied into the property store on the way past, so + // nothing outlives the document. + if ( json != NULL ) { + json_decref(json); + json = NULL; + } } PROCESS(errctx) { } FINISH(errctx, true); - json_object_foreach(props, pkey, pvalue) { - ATTEMPT { - CATCH(errctx, akgl_heap_next_string(&tmpstr)); - CATCH(errctx, akgl_get_json_string_value(props, (char *)pkey, &tmpstr)); - SDL_SetStringProperty(AKGL_REGISTRY_PROPERTIES, pkey, tmpstr->data); - SDL_Log("Set property %s = %s", pkey, tmpstr->data); - CATCH(errctx, akgl_heap_release_string(tmpstr)); - } CLEANUP { - } PROCESS(errctx) { - } FINISH(errctx, true); - } SDL_Log("Properties loaded"); SUCCEED_RETURN(errctx); } @@ -171,6 +186,9 @@ akerr_ErrorContext AKERR_NOIGNORE *akgl_set_property(char *name, char *src) akerr_ErrorContext AKERR_NOIGNORE *akgl_get_property(char *name, akgl_String **dest, char *def) { + const char *value = NULL; + size_t valuelen = 0; + PREPARE_ERROR(e); FAIL_ZERO_RETURN(e, name, AKERR_NULLPOINTER, "NULL char *"); FAIL_ZERO_RETURN(e, dest, AKERR_NULLPOINTER, "NULL akgl_String *"); @@ -178,12 +196,32 @@ akerr_ErrorContext AKERR_NOIGNORE *akgl_get_property(char *name, akgl_String **d if ( *dest == NULL ) { CATCH(e, akgl_heap_next_string(dest)); } - CATCH(e, - aksl_memcpy( - (*dest)->data, - (void *)SDL_GetStringProperty(AKGL_REGISTRY_PROPERTIES, (const char *)name, (const char *)def), - AKGL_MAX_STRING_LENGTH) - ); + // Copy the value's own length, not the destination's capacity. What + // SDL hands back is its strdup of the value -- four bytes for "0.0" -- + // so copying a fixed AKGL_MAX_STRING_LENGTH read up to 4 KiB past the + // end of somebody else's allocation on every call. It returned garbage + // past the terminator, and it would have faulted outright on a value + // that happened to sit at the end of a page. + value = SDL_GetStringProperty(AKGL_REGISTRY_PROPERTIES, (const char *)name, (const char *)def); + // An unset property with a NULL default still reports AKERR_NULLPOINTER, + // which is what the header has always promised. It used to arrive from + // inside aksl_memcpy; now it is raised here, before anything is measured. + FAIL_ZERO_BREAK( + e, + value, + AKERR_NULLPOINTER, + "Property %s is not set and no default was given", + name); + valuelen = strlen(value); + FAIL_NONZERO_BREAK( + e, + (valuelen >= AKGL_MAX_STRING_LENGTH), + AKERR_OUTOFBOUNDS, + "Property %s is %zu bytes, which does not fit an akgl_String (%d)", + name, + valuelen, + AKGL_MAX_STRING_LENGTH); + CATCH(e, aksl_memcpy((*dest)->data, (void *)value, (valuelen + 1))); } CLEANUP { } PROCESS(e) { } FINISH(e, true); diff --git a/src/sprite.c b/src/sprite.c index 1dea358..778b942 100644 --- a/src/sprite.c +++ b/src/sprite.c @@ -167,6 +167,13 @@ akerr_ErrorContext *akgl_sprite_load_json(char *filename) CATCH(errctx, akgl_get_json_array_index_integer((json_t *)frames, i, (uint32_t *)&obj->frameids[i])); } } CLEANUP { + // The sprite copies every field it wants out of the document and the + // sheet is found by resolved path, so nothing here points into the + // parsed tree once this block runs. Released on both paths. + if ( json != NULL ) { + json_decref(json); + json = NULL; + } if ( errctx != NULL && errctx->status != 0 ) { IGNORE(akgl_heap_release_sprite(obj)); IGNORE(akgl_heap_release_spritesheet(sheet)); diff --git a/src/text.c b/src/text.c index 4e97441..e54d6e3 100644 --- a/src/text.c +++ b/src/text.c @@ -19,6 +19,13 @@ akerr_ErrorContext AKERR_NOIGNORE *akgl_text_loadfont(char *name, char *filepath FAIL_ZERO_RETURN(errctx, filepath, AKERR_NULLPOINTER, "Null filepath"); font = TTF_OpenFont(filepath, size); FAIL_ZERO_RETURN(errctx, font, AKGL_ERR_SDL, "%s", SDL_GetError()); + // Loading over an existing name used to abandon the font it displaced -- + // there was nothing in the library that could close one. The old font goes + // back first now, and only once the new one has opened, so a failed load + // leaves the caller with the font they already had. + if ( SDL_GetPointerProperty(AKGL_REGISTRY_FONT, name, NULL) != NULL ) { + PASS(errctx, akgl_text_unloadfont(name)); + } FAIL_ZERO_RETURN( errctx, SDL_SetPointerProperty(AKGL_REGISTRY_FONT, name, (void *)font), @@ -30,6 +37,23 @@ akerr_ErrorContext AKERR_NOIGNORE *akgl_text_loadfont(char *name, char *filepath SUCCEED_RETURN(errctx); } +akerr_ErrorContext AKERR_NOIGNORE *akgl_text_unloadfont(char *name) +{ + TTF_Font *font = NULL; + + PREPARE_ERROR(errctx); + FAIL_ZERO_RETURN(errctx, name, AKERR_NULLPOINTER, "Null font name"); + + font = (TTF_Font *)SDL_GetPointerProperty(AKGL_REGISTRY_FONT, name, NULL); + FAIL_ZERO_RETURN(errctx, font, AKERR_KEY, "No font named %s in the registry", name); + + // Cleared before the close, so a font is never reachable through the + // registry after it has been handed back to SDL_ttf. + SDL_ClearProperty(AKGL_REGISTRY_FONT, name); + TTF_CloseFont(font); + SUCCEED_RETURN(errctx); +} + akerr_ErrorContext AKERR_NOIGNORE *akgl_text_rendertextat(TTF_Font *font, char *text, SDL_Color color, int wraplength, int x, int y) { SDL_Surface *textsurf = NULL; diff --git a/src/tilemap.c b/src/tilemap.c index e4018f3..2e9de09 100644 --- a/src/tilemap.c +++ b/src/tilemap.c @@ -729,6 +729,14 @@ akerr_ErrorContext *akgl_tilemap_load(char *fname, akgl_Tilemap *dest) } } CLEANUP { //IGNORE(akgl_heap_release_string(tmpstr)); + // The map is built entirely out of copies -- layer data, tileset + // geometry, object names -- so the document is dead the moment the + // loaders above return, whether they succeeded or not. It is also the + // largest of the four: a map's JSON is the size of its layer data. + if ( json != NULL ) { + json_decref(json); + json = NULL; + } } PROCESS(errctx) { } FINISH(errctx, true); SUCCEED_RETURN(errctx); diff --git a/tests/registry.c b/tests/registry.c index 0849630..22ad336 100644 --- a/tests/registry.c +++ b/tests/registry.c @@ -1,7 +1,13 @@ #include #include +#include #include +#include +#include #include +#include + +#include "testutil.h" typedef akerr_ErrorContext *(*RegistryFuncPtr)(void); @@ -83,12 +89,82 @@ akerr_ErrorContext *test_akgl_registry_init_creation_failures(void) 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); +} + int main(void) { PREPARE_ERROR(errctx); ATTEMPT { CATCH(errctx, akgl_error_init()); CATCH(errctx, test_akgl_registry_init_creation_failures()); + CATCH(errctx, test_akgl_get_property_copies_only_the_value()); } CLEANUP { } PROCESS(errctx) { } FINISH_NORETURN(errctx); diff --git a/tests/text.c b/tests/text.c index e6c6852..19d743a 100644 --- a/tests/text.c +++ b/tests/text.c @@ -68,6 +68,31 @@ akerr_ErrorContext *test_text_loadfont(void) TEST_EXPECT_STATUS(errctx, AKGL_ERR_SDL, akgl_text_loadfont("missing", "assets/no_such_font.ttf", TEST_FONT_SIZE), "loading a font that does not exist"); + + // Loading over a live name closes the font it displaces. Nothing here + // can observe the close directly -- the proof is the memcheck run -- + // but the replacement itself is observable, and it is the path that + // used to abandon ten kilobytes per call. + TEST_EXPECT_OK( + errctx, + akgl_text_loadfont("testfont", TEST_FONT_PATH, TEST_FONT_SIZE), + "loading a second font over a registered name"); + TEST_ASSERT(errctx, + SDL_GetPointerProperty(AKGL_REGISTRY_FONT, "testfont", NULL) != registered, + "reloading a font under a live name left the displaced font registered"); + + // And the font goes back. Unloading is what makes the suite's own + // bookkeeping honest, so this is not only an API test. + TEST_EXPECT_OK(errctx, akgl_text_unloadfont("testfont"), "unloading a registered font"); + TEST_ASSERT(errctx, + SDL_GetPointerProperty(AKGL_REGISTRY_FONT, "testfont", NULL) == NULL, + "akgl_text_unloadfont left the font in AKGL_REGISTRY_FONT"); + TEST_EXPECT_STATUS(errctx, AKERR_KEY, akgl_text_unloadfont("testfont"), + "unloading a font that is already gone"); + TEST_EXPECT_STATUS(errctx, AKERR_KEY, akgl_text_unloadfont("never_loaded"), + "unloading a name that was never registered"); + TEST_EXPECT_STATUS(errctx, AKERR_NULLPOINTER, akgl_text_unloadfont(NULL), + "unloading a NULL name"); } CLEANUP { } PROCESS(errctx) { } FINISH(errctx, true);