From e337babce926dea6bc4067b4111e9297e5057e8e Mon Sep 17 00:00:00 2001 From: Arnold de Ruiter <187004+ArnoldDeRuiter@users.noreply.github.com> Date: Fri, 6 Nov 2026 14:16:58 +0100 Subject: [PATCH] core_info: read an info file added after its core was cached, keep served entries in a rewritten cache (#20007) * Fix core info cache ignoring late .info files and dropping cached entries on rewrite * core_info: write the cache straight from the core list The cache writer takes the core list's entries as they are instead of a deep copy of every installed core, and a core read from its info file no longer gets copied into a cache list that is not written. core_info_cache_test also checks that an unchanged core set leaves the cache alone, and runs under ASan and UBSan in Linux-samples-tasks. * core_info: read cached entries in place, take core paths from the listing The cache reader parses each entry straight into the next slot of the cache list instead of a separate allocation moved in afterwards, and a firmware list grows geometrically rather than one entry per realloc. A core's path is taken over from the directory listing, which is freed at the end of the same pass, instead of being copied. core_info_cache_test adds a start served from the cache alone, with more cores than the list starts with and long firmware lists, and a cache cut off inside an entry, which must be discarded without a leak. * string_list: add packed splits, use them for core info lists string_split_packed() splits into a single allocation holding the list, its elements and the tokens, marked by cap 0 with elements present. string_list_free() releases it in one free; growing it and string_list_deinitialize() are refused, so a packed list stays read-only. Every pipe-separated core info field is split this way, from the cache and from info files alike, which takes a warm start over the full libretro-core-info set from 16661 allocations to 8851. The string_split_packed sample checks it against string_split and the refusals, under ASan and UBSan and as a mingw-w64 build under Wine. --------- Co-authored-by: libretroadmin --- .github/workflows/Linux-samples-gfx.yml | 2 +- .github/workflows/Linux-samples-tasks.yml | 16 + core_info.c | 239 +++++------- libretro-common/include/lists/string_list.h | 14 + libretro-common/lists/string_list.c | 55 ++- .../lists/string_split_packed/Makefile | 20 + .../string_split_packed_test.c | 105 ++++++ samples/tasks/database/Makefile | 19 +- samples/tasks/database/core_info_cache_test.c | 353 ++++++++++++++++++ 9 files changed, 677 insertions(+), 146 deletions(-) create mode 100644 libretro-common/samples/lists/string_split_packed/Makefile create mode 100644 libretro-common/samples/lists/string_split_packed/string_split_packed_test.c create mode 100644 samples/tasks/database/core_info_cache_test.c diff --git a/.github/workflows/Linux-samples-gfx.yml b/.github/workflows/Linux-samples-gfx.yml index 9159cb0ae5..5466ce7e4b 100644 --- a/.github/workflows/Linux-samples-gfx.yml +++ b/.github/workflows/Linux-samples-gfx.yml @@ -3074,7 +3074,7 @@ jobs: shell: bash run: | set -eu - for sample in samples/gfx/video_frame_failure libretro-common/samples/lists/file_list_oom libretro-common/samples/gfx/scaler_point libretro-common/samples/gfx/image_scale_transfer samples/tasks/translation_reply; do + for sample in samples/gfx/video_frame_failure libretro-common/samples/lists/file_list_oom libretro-common/samples/lists/string_split_packed libretro-common/samples/gfx/scaler_point libretro-common/samples/gfx/image_scale_transfer samples/tasks/translation_reply; do make -C "$sample" clean make -C "$sample" check SANITIZER=address,undefined make -C "$sample" clean diff --git a/.github/workflows/Linux-samples-tasks.yml b/.github/workflows/Linux-samples-tasks.yml index e29ccecd91..2b0f51c969 100644 --- a/.github/workflows/Linux-samples-tasks.yml +++ b/.github/workflows/Linux-samples-tasks.yml @@ -1241,6 +1241,22 @@ jobs: make clean echo "[pass] database_scan_test (TSan, HAVE_THREADS=0, SCAN_READERS_MAX=0)" + - name: Build and run core_info_cache_test (ASan + UBSan) + shell: bash + working-directory: samples/tasks/database + run: | + set -eu + # core_info.cache: an info file added after its core was + # cached is read, a rewrite keeps the entries served from the + # cache whole, and an unchanged core set rewrites nothing. + make clean SANITIZER=address,undefined && make -j$(nproc) core_info_cache_test SANITIZER=address,undefined + root="$(mktemp -d)" + ASAN_OPTIONS=detect_leaks=1 UBSAN_OPTIONS=print_stacktrace=1:halt_on_error=1 \ + timeout 120 ./core_info_cache_test "$root" + rm -rf "${root:?}" + make clean + echo "[pass] core_info_cache_test (ASan)" + - name: Build and run pl_manager_budget_test (plain, ASan + UBSan, TSan) shell: bash working-directory: samples/tasks/playlist_manager diff --git a/core_info.c b/core_info.c index 99c54d3b99..d849a03697 100644 --- a/core_info.c +++ b/core_info.c @@ -85,6 +85,7 @@ typedef struct char **current_string_val; struct string_list **current_string_list_val; uint32_t *current_entry_uint_val; + size_t firmware_capacity; uint8_t current_entry_flag_mask; /* which CORE_INFO_FLAG_* to set */ bool *current_entry_bool_val; /* for firmware optional */ unsigned array_depth; @@ -98,9 +99,8 @@ static void core_info_free(core_info_t* info); static uint32_t core_info_hash_string(const char *str); #ifdef HAVE_CORE_INFO_CACHE static core_info_cache_list_t *core_info_cache_list_new(void); +static core_info_t *core_info_cache_next_slot(core_info_cache_list_t *list); #endif -static void core_info_cache_add(core_info_cache_list_t *list, - core_info_t *info, bool transfer); static core_info_state_t core_info_st = { #ifdef HAVE_COMPRESSION @@ -377,20 +377,14 @@ static bool CCJSONStringHandler(void *context, free(*pCtx->current_string_val); *pCtx->current_string_val = strdup(pValue); - /* Gate the string_list split on strdup success: if strdup - * returned NULL (OOM), the assignment above stored NULL - * into *current_string_val, and string_split dereferences - * its first parameter unconditionally at - * libretro-common/lists/string_list.c:252 ('while (*p)' with - * p = str). On OOM just leave current_string_list_val - * alone; downstream core_info readers already tolerate - * missing split lists. */ + /* No list for a value strdup could not hold; readers + * tolerate a missing one */ if (pCtx->current_string_list_val && *pCtx->current_string_val) { if (*pCtx->current_string_list_val) string_list_free(*pCtx->current_string_list_val); *pCtx->current_string_list_val = - string_split(*pCtx->current_string_val, "|"); + string_split_packed(*pCtx->current_string_val, "|"); } } @@ -454,14 +448,16 @@ static bool CCJSONStartObjectHandler(void *context) case 1: if (pCtx->object_depth == 2) { + /* Parsed in place, into the slot past the list's end */ if (pCtx->core_info) { core_info_free(pCtx->core_info); - free(pCtx->core_info); pCtx->core_info = NULL; } - if (!(pCtx->core_info = (core_info_t*)calloc(1, sizeof(core_info_t)))) + pCtx->firmware_capacity = 0; + if (!(pCtx->core_info = core_info_cache_next_slot( + pCtx->core_info_cache_list))) return false; /* Assume all cores have 'full' savestate support @@ -474,20 +470,24 @@ static bool CCJSONStartObjectHandler(void *context) if (pCtx->object_depth == 3 && pCtx->to_firmware) { size_t new_idx = pCtx->core_info->firmware_count; - core_info_firmware_t *tmp = (core_info_firmware_t*) - realloc(pCtx->core_info->firmware, - (pCtx->core_info->firmware_count + 1) - * sizeof(core_info_firmware_t)); + core_info_firmware_t *tmp = pCtx->core_info->firmware; - if (!tmp) - return false; + if (new_idx >= pCtx->firmware_capacity) + { + size_t cap = pCtx->firmware_capacity + ? pCtx->firmware_capacity << 1 : 4; + if (!(tmp = (core_info_firmware_t*)realloc(tmp, + cap * sizeof(core_info_firmware_t)))) + return false; + pCtx->core_info->firmware = tmp; + pCtx->firmware_capacity = cap; + } tmp[new_idx].path = NULL; tmp[new_idx].desc = NULL; tmp[new_idx].missing = false; tmp[new_idx].optional = false; - pCtx->core_info->firmware = tmp; pCtx->core_info->firmware_count++; } break; @@ -508,9 +508,13 @@ static bool CCJSONEndObjectHandler(void *context) case 2: if (pCtx->core_info) { - core_info_cache_add( - pCtx->core_info_cache_list, pCtx->core_info, true); - free(pCtx->core_info); + core_info_t *info = pCtx->core_info; + if ( info->core_file_id.hash + && info->core_file_id.str + && *info->core_file_id.str) + pCtx->core_info_cache_list->length++; + else + core_info_free(info); pCtx->core_info = NULL; } break; @@ -549,68 +553,7 @@ static bool CCJSONEndArrayHandler(void *context) /* JSON Handlers END */ #endif -/* Note: 'dst' must be zero initialised, or memory - * leaks will occur */ -static void core_info_copy(core_info_t *src, core_info_t *dst) -{ - dst->path = src->path ? strdup(src->path) : NULL; - dst->display_name = src->display_name ? strdup(src->display_name) : NULL; - dst->display_version = src->display_version ? strdup(src->display_version) : NULL; - dst->core_name = src->core_name ? strdup(src->core_name) : NULL; - dst->system_manufacturer = src->system_manufacturer ? strdup(src->system_manufacturer) : NULL; - dst->systemname = src->systemname ? strdup(src->systemname) : NULL; - dst->system_id = src->system_id ? strdup(src->system_id) : NULL; - dst->supported_extensions = src->supported_extensions ? strdup(src->supported_extensions) : NULL; - dst->authors = src->authors ? strdup(src->authors) : NULL; - dst->permissions = src->permissions ? strdup(src->permissions) : NULL; - dst->licenses = src->licenses ? strdup(src->licenses) : NULL; - dst->categories = src->categories ? strdup(src->categories) : NULL; - dst->databases = src->databases ? strdup(src->databases) : NULL; - dst->notes = src->notes ? strdup(src->notes) : NULL; - dst->required_hw_api = src->required_hw_api ? strdup(src->required_hw_api) : NULL; - dst->description = src->description ? strdup(src->description) : NULL; - - dst->categories_list = src->categories_list ? string_list_clone(src->categories_list) : NULL; - dst->databases_list = src->databases_list ? string_list_clone(src->databases_list) : NULL; - dst->note_list = src->note_list ? string_list_clone(src->note_list) : NULL; - dst->supported_extensions_list = src->supported_extensions_list ? string_list_clone(src->supported_extensions_list) : NULL; - dst->authors_list = src->authors_list ? string_list_clone(src->authors_list) : NULL; - dst->permissions_list = src->permissions_list ? string_list_clone(src->permissions_list) : NULL; - dst->licenses_list = src->licenses_list ? string_list_clone(src->licenses_list) : NULL; - dst->required_hw_api_list = src->required_hw_api_list ? string_list_clone(src->required_hw_api_list) : NULL; - - if (src->firmware_count > 0) - { - dst->firmware = (core_info_firmware_t*)calloc(src->firmware_count, - sizeof(core_info_firmware_t)); - - if (dst->firmware) - { - size_t i; - - dst->firmware_count = src->firmware_count; - - for (i = 0; i < src->firmware_count; i++) - { - dst->firmware[i].path = src->firmware[i].path ? strdup(src->firmware[i].path) : NULL; - dst->firmware[i].desc = src->firmware[i].desc ? strdup(src->firmware[i].desc) : NULL; - dst->firmware[i].missing = src->firmware[i].missing; - dst->firmware[i].optional = src->firmware[i].optional; - } - } - else - dst->firmware_count = 0; - } - - dst->core_file_id.str = src->core_file_id.str - ? strdup(src->core_file_id.str) : NULL; - dst->core_file_id.hash = src->core_file_id.hash; - - dst->savestate_support_level = src->savestate_support_level; - dst->flags = src->flags; -} - -/* Like core_info_copy, but transfers 'ownership' +/* Transfers 'ownership' * of internal objects/data structures from 'src' * to 'dst' */ static void core_info_transfer(core_info_t *src, core_info_t *dst) @@ -751,17 +694,14 @@ static core_info_t *core_info_cache_find( return NULL; } -static void core_info_cache_add( - core_info_cache_list_t *list, core_info_t *info, - bool transfer) +#ifdef HAVE_CORE_INFO_CACHE +/* The zeroed slot past the end; length grows once the entry is whole */ +static core_info_t *core_info_cache_next_slot(core_info_cache_list_t *list) { - core_info_t *info_cache = NULL; + core_info_t *slot; - if ( !list - || !info - || (info->core_file_id.hash == 0) - || (!info->core_file_id.str || !*info->core_file_id.str)) - return; + if (!list) + return NULL; if (list->length >= list->capacity) { @@ -770,7 +710,7 @@ static void core_info_cache_add( (list->capacity << 1) * sizeof(core_info_t)); if (!items_tmp) - return; + return NULL; list->capacity = list->capacity << 1; list->items = items_tmp; @@ -779,17 +719,11 @@ static void core_info_cache_add( (list->capacity - prev_capacity) * sizeof(core_info_t)); } - info_cache = (core_info_t*)&list->items[list->length]; - - if (transfer) - core_info_transfer(info, info_cache); - else - core_info_copy(info, info_cache); - - list->length++; + slot = &list->items[list->length]; + memset(slot, 0, sizeof(*slot)); + return slot; } -#ifdef HAVE_CORE_INFO_CACHE static core_info_cache_list_t *core_info_cache_list_new(void) { core_info_cache_list_t *core_info_cache_list = @@ -891,6 +825,13 @@ static core_info_cache_list_t *core_info_cache_read(const char *info_dir) ? rjson_get_error(parser) : "format error")); + /* The entry being parsed sits past the list's end */ + if (context.core_info) + { + core_info_free(context.core_info); + context.core_info = NULL; + } + /* Info cache is corrupt - discard it. * core_info_cache_list_free() releases the contents but not the * structure itself (see the caller in core_info_list_new, which @@ -905,13 +846,8 @@ static core_info_cache_list_t *core_info_cache_read(const char *info_dir) rjson_free(parser); - /* Clean up leftovers in the event of - * a parsing error */ if (context.core_info) - { core_info_free(context.core_info); - free(context.core_info); - } if (!core_info_cache_list) goto end; @@ -945,7 +881,8 @@ end: } #endif -static bool core_info_cache_write(core_info_cache_list_t *list, const char *info_dir) +static bool core_info_cache_write(const core_info_t *items, size_t count, + const char *info_dir) { intfstream_t *file = NULL; rjsonwriter_t *writer = NULL; @@ -955,7 +892,7 @@ static bool core_info_cache_write(core_info_cache_list_t *list, const char *info char write_path[PATH_MAX_LENGTH]; size_t i, j; - if (!list) + if (!items) return false; /* Open info cache file */ @@ -1025,9 +962,9 @@ static bool core_info_cache_write(core_info_cache_list_t *list, const char *info { bool first_written = true; - for (i = 0; i < list->length; i++) + for (i = 0; i < count; i++) { - core_info_t* info = &list->items[i]; + const core_info_t *info = &items[i]; if (!(info->flags & CORE_INFO_FLAG_IS_INSTALLED)) continue; @@ -1285,7 +1222,6 @@ static bool core_info_cache_write(core_info_cache_list_t *list, const char *info filestream_delete(write_path); RARCH_ERR("[Core info] Failed to write core info cache file: \"%s\".\n", file_path); - list->refresh = false; return false; } @@ -1316,7 +1252,6 @@ end: if (!success && *write_path) filestream_delete(write_path); - list->refresh = false; return success; } @@ -1386,8 +1321,8 @@ bool core_info_cache_force_refresh(const char *path_info) typedef struct { - const char *path; const char *filename; + char **owner; /* the dir_list slot holding the path, handed over */ } core_file_path_t; typedef struct @@ -1469,7 +1404,7 @@ static core_path_list_t *core_info_path_list_new(const char *core_dir, return NULL; if (!(path_list = (core_path_list_t*)calloc(1, sizeof(*path_list)))) return NULL; - if (!(core_ext_list = string_split(core_exts, "|"))) + if (!(core_ext_list = string_split_packed(core_exts, "|"))) { core_info_path_list_free(path_list); return NULL; @@ -1557,10 +1492,11 @@ static core_path_list_t *core_info_path_list_new(const char *core_dir, * 'standalone exempt' file */ if (string_list_find_elem(core_ext_list, file_ext)) { - path_list->core_list->list[ - path_list->core_list->size].path = file_path; path_list->core_list->list[ path_list->core_list->size].filename = filename; + path_list->core_list->list[ + path_list->core_list->size].owner = + &path_list->dir_list->elems[i].data; path_list->core_list->size++; } else if (memcmp(file_ext, FILE_PATH_LOCK_EXTENSION_NO_DOT, sizeof(FILE_PATH_LOCK_EXTENSION_NO_DOT)) == 0) @@ -1787,7 +1723,7 @@ static void core_info_parse_config_file( { info->supported_extensions_list = - string_split(info->supported_extensions, "|"); + string_split_packed(info->supported_extensions, "|"); } info->authors = config_take_string(conf, "authors"); @@ -1796,7 +1732,7 @@ static void core_info_parse_config_file( { info->authors_list = - string_split(info->authors, "|"); + string_split_packed(info->authors, "|"); } info->permissions = config_take_string(conf, "permissions"); @@ -1805,7 +1741,7 @@ static void core_info_parse_config_file( { info->permissions_list = - string_split(info->permissions, "|"); + string_split_packed(info->permissions, "|"); } info->licenses = config_take_string(conf, "license"); @@ -1814,7 +1750,7 @@ static void core_info_parse_config_file( { info->licenses_list = - string_split(info->licenses, "|"); + string_split_packed(info->licenses, "|"); } info->categories = config_take_string(conf, "categories"); @@ -1823,7 +1759,7 @@ static void core_info_parse_config_file( { info->categories_list = - string_split(info->categories, "|"); + string_split_packed(info->categories, "|"); } info->databases = config_take_string(conf, "database"); @@ -1832,7 +1768,7 @@ static void core_info_parse_config_file( { info->databases_list = - string_split(info->databases, "|"); + string_split_packed(info->databases, "|"); } info->notes = config_take_string(conf, "notes"); @@ -1841,7 +1777,7 @@ static void core_info_parse_config_file( { info->note_list = - string_split(info->notes, "|"); + string_split_packed(info->notes, "|"); } info->required_hw_api = config_take_string(conf, "required_hw_api"); @@ -1850,7 +1786,7 @@ static void core_info_parse_config_file( { info->required_hw_api_list = - string_split(info->required_hw_api, "|"); + string_split_packed(info->required_hw_api, "|"); } info->description = config_take_string(conf, "description"); @@ -2188,6 +2124,29 @@ static void core_info_list_free(core_info_list_t *core_info_list) free(core_info_list); } +/* True when a core was cached without an info file that exists now */ +static bool core_info_cache_entry_missed_info(const core_info_t *info_cache, + const char *core_file_id, size_t len, const char *info_dir) +{ + char info_name[256]; + char info_path[PATH_MAX_LENGTH]; + + if (info_cache->flags & CORE_INFO_FLAG_HAS_INFO) + return false; + + strlcpy(info_name, core_file_id, sizeof(info_name)); + strlcpy(info_name + len, FILE_PATH_CORE_INFO_EXTENSION, + sizeof(info_name) - len); + + if (info_dir && *info_dir) + fill_pathname_join_special(info_path, info_dir, info_name, + sizeof(info_path)); + else + strlcpy(info_path, info_name, sizeof(info_path)); + + return path_is_valid(info_path); +} + static core_info_list_t *core_info_list_new(const char *path, const char *libretro_info_dir, const char *exts, @@ -2244,7 +2203,6 @@ static core_info_list_t *core_info_list_new(const char *path, config_file_t *conf = NULL; core_info_t *info = &core_info[i]; core_file_path_t *core_file = &path_list->core_list->list[i]; - const char *base_path = core_file->path; const char *core_filename = core_file->filename; size_t _len = core_info_get_file_id(core_filename, core_file_id, sizeof(core_file_id)); @@ -2258,6 +2216,15 @@ static core_info_list_t *core_info_list_new(const char *path, core_info_t *info_cache = core_info_cache_find( core_info_cache_list, core_file_id); + /* Info file added after caching: parse it, drop the stale entry */ + if ( info_cache + && core_info_cache_entry_missed_info(info_cache, + core_file_id, _len, info_dir)) + { + info_cache->flags &= ~CORE_INFO_FLAG_IS_INSTALLED; + info_cache = NULL; + } + if (info_cache) { /* MEM-3: Transfer ownership instead of deep-copy @@ -2269,7 +2236,8 @@ static core_info_list_t *core_info_list_new(const char *path, * change between runs) */ if (info->path) free(info->path); - info->path = strdup(base_path); + info->path = *core_file->owner; + *core_file->owner = NULL; /* Core lock status is 'dynamic', and * cannot be cached */ @@ -2303,7 +2271,8 @@ static core_info_list_t *core_info_list_new(const char *path, } /* Cache core path */ - info->path = strdup(base_path); + info->path = *core_file->owner; + *core_file->owner = NULL; /* Get core lock status */ if (core_info_path_is_locked( @@ -2341,15 +2310,9 @@ static core_info_list_t *core_info_list_new(const char *path, info->flags |= CORE_INFO_FLAG_IS_INSTALLED; - /* If info cache is enabled and we reach this - * point, current core is uncached - * > Add it to the list, and trigger a cache - * refresh */ + /* Uncached core: the cache is rewritten from the list */ if (core_info_cache_list) - { - core_info_cache_add(core_info_cache_list, info, false); core_info_cache_list->refresh = true; - } } core_info_list_resolve_all_extensions(core_info_list); @@ -2367,7 +2330,7 @@ static core_info_list_t *core_info_list_new(const char *path, if (core_info_cache_list->refresh) *cache_supported = core_info_cache_write( - core_info_cache_list, info_dir); + core_info_list->list, core_info_list->count, info_dir); core_info_cache_list_free(core_info_cache_list); free(core_info_cache_list); diff --git a/libretro-common/include/lists/string_list.h b/libretro-common/include/lists/string_list.h index ce106b19a9..101d370752 100644 --- a/libretro-common/include/lists/string_list.h +++ b/libretro-common/include/lists/string_list.h @@ -88,6 +88,20 @@ bool string_list_find_elem_prefix(const struct string_list *list, */ struct string_list *string_split(const char *str, const char *delim); +/** + * string_split_packed: + * @str : string to split + * @delim : delimiter characters + * + * Like string_split(), in one allocation holding the list, its + * elements and the tokens. The list is read-only: it cannot grow, + * and string_list_deinitialize() refuses it. Free it with + * string_list_free(). A packed list has cap 0 and non-NULL elems. + * + * @return the list, or NULL on allocation failure. + */ +struct string_list *string_split_packed(const char *str, const char *delim); + bool string_split_noalloc(struct string_list *list, const char *str, const char *delim); diff --git a/libretro-common/lists/string_list.c b/libretro-common/lists/string_list.c index 23fd24d4b4..8723525626 100644 --- a/libretro-common/lists/string_list.c +++ b/libretro-common/lists/string_list.c @@ -54,6 +54,8 @@ static bool string_list_deinitialize_internal(struct string_list *list) return true; } +#define STRING_LIST_PACKED(list) (!(list)->cap && (list)->elems) + bool string_list_capacity(struct string_list *list, size_t cap) { struct string_list_elem *new_data; @@ -63,7 +65,7 @@ bool string_list_capacity(struct string_list *list, size_t cap) * still counted them, and the next walk or free ran off the end; * a cap of zero is a realloc(p, 0), which on glibc frees p and * returns NULL - reported here as failure with elems dangling. */ - if (!list || cap < list->size || cap == 0) + if (!list || cap < list->size || cap == 0 || STRING_LIST_PACKED(list)) return false; /* Guard the byte-count multiplication: a huge cap would wrap and @@ -92,14 +94,15 @@ void string_list_free(struct string_list *list) if (!list) return; - string_list_deinitialize_internal(list); + if (!STRING_LIST_PACKED(list)) + string_list_deinitialize_internal(list); free(list); } bool string_list_deinitialize(struct string_list *list) { - if (!list) + if (!list || STRING_LIST_PACKED(list)) return false; if (!string_list_deinitialize_internal(list)) return false; @@ -362,6 +365,52 @@ error: return NULL; } +struct string_list *string_split_packed(const char *str, const char *delim) +{ + size_t i, n, slen, head; + char *out; + const char *p; + struct string_list *list; + + if (!str || !delim) + return NULL; + + n = string_count_tokens(str, delim); + slen = strlen(str); + head = sizeof(*list) + n * sizeof(struct string_list_elem); + if (n > (SIZE_MAX - sizeof(*list)) / sizeof(struct string_list_elem) + || slen >= SIZE_MAX - head) + return NULL; + if (!(list = (struct string_list*)malloc(head + slen + 1))) + return NULL; + + list->size = n; + list->cap = 0; + list->elems = n ? (struct string_list_elem*)(list + 1) : NULL; + out = (char*)list + head; + p = str; + + for (i = 0; i < n; i++) + { + const char *tok; + + while (*p && strchr(delim, *p)) + p++; + tok = p; + while (*p && !strchr(delim, *p)) + p++; + + memcpy(out, tok, (size_t)(p - tok)); + list->elems[i].data = out; + list->elems[i].userdata = NULL; + list->elems[i].attr.i = 0; + out += p - tok; + *out++ = '\0'; + } + + return list; +} + bool string_split_noalloc(struct string_list *list, const char *str, const char *delim) { diff --git a/libretro-common/samples/lists/string_split_packed/Makefile b/libretro-common/samples/lists/string_split_packed/Makefile new file mode 100644 index 0000000000..d5ab11734c --- /dev/null +++ b/libretro-common/samples/lists/string_split_packed/Makefile @@ -0,0 +1,20 @@ +ROOT := ../../.. +CC ?= cc +CFLAGS += -std=gnu89 -Wall -Wextra -Werror -Wdeclaration-after-statement -D_GNU_SOURCE -I$(ROOT)/include +ifneq ($(SANITIZER),) +CFLAGS += -g -fsanitize=$(SANITIZER) -fno-omit-frame-pointer +LDFLAGS += -fsanitize=$(SANITIZER) +endif + +all: string_split_packed_test$(EXE) + +string_split_packed_test$(EXE): string_split_packed_test.c $(ROOT)/lists/string_list.c + $(CC) $(CFLAGS) $< $(ROOT)/lists/string_list.c $(ROOT)/compat/compat_strl.c $(LDFLAGS) -o $@ + +check: string_split_packed_test$(EXE) + $(RUNNER) ./string_split_packed_test$(EXE) + +clean: + rm -f string_split_packed_test string_split_packed_test.exe + +.PHONY: all check clean diff --git a/libretro-common/samples/lists/string_split_packed/string_split_packed_test.c b/libretro-common/samples/lists/string_split_packed/string_split_packed_test.c new file mode 100644 index 0000000000..dcdd170da6 --- /dev/null +++ b/libretro-common/samples/lists/string_split_packed/string_split_packed_test.c @@ -0,0 +1,105 @@ +/* A packed split matches string_split, is one block, and stays read-only. */ +#include +#include +#include + +#include + +static int fails; + +static void check(const char *what, int ok) +{ + if (!ok) + { + fails++; + printf("FAIL %s\n", what); + } +} + +static void same_as_split(const char *str, const char *delim) +{ + size_t i; + char what[256]; + struct string_list *a = string_split(str, delim); + struct string_list *b = string_split_packed(str, delim); + + snprintf(what, sizeof(what), "\"%s\" by \"%s\" splits as string_split", + str, delim); + if (!a || !b || a->size != b->size) + { + check(what, 0); + string_list_free(a); + string_list_free(b); + return; + } + for (i = 0; i < a->size; i++) + if (strcmp(a->elems[i].data, b->elems[i].data) + || b->elems[i].userdata || b->elems[i].attr.i) + check(what, 0); + + /* Every element and token inside the one block */ + snprintf(what, sizeof(what), "\"%s\" is one block", str); + for (i = 0; i < b->size; i++) + if ( (char*)&b->elems[i] < (char*)(b + 1) + || b->elems[i].data < (char*)(b->elems + b->size) + || b->elems[i].data + strlen(b->elems[i].data) + > (char*)(b->elems + b->size) + strlen(str)) + check(what, 0); + + string_list_free(a); + string_list_free(b); +} + +int main(void) +{ + union string_list_elem_attr attr; + struct string_list *packed; + struct string_list *clone; + + same_as_split("", "|"); + same_as_split("|||", "|"); + same_as_split("cue", "|"); + same_as_split("cue|chd|iso", "|"); + same_as_split("|cue||chd|", "|"); + same_as_split("a b,c;;d", " ,;"); + same_as_split("Fran\xc3\xa7ois|\xe6\x97\xa5\xe6\x9c\xac", "|"); + + packed = string_split_packed("cue|chd|iso", "|"); + check("packed split allocates", packed != NULL); + if (!packed) + return 1; + check("packed list finds its tokens", + string_list_find_elem(packed, "chd") == 2 + && !string_list_find_elem(packed, "bin")); + + /* Read-only: growing it would realloc into the middle of the block */ + attr.i = 0; + check("append is refused", !string_list_append(packed, "bin", attr)); + check("append_n is refused", + !string_list_append_n(packed, "bin", 3, attr)); + check("capacity is refused", !string_list_capacity(packed, 64)); + check("deinitialize is refused", !string_list_deinitialize(packed)); + check("refusals leave it whole", packed->size == 3 + && !strcmp(packed->elems[2].data, "iso")); + + /* A clone is an ordinary list again */ + clone = string_list_clone(packed); + check("clone allocates", clone != NULL); + if (clone) + { + check("clone appends", string_list_append(clone, "bin", attr) + && clone->size == 4 && !strcmp(clone->elems[3].data, "bin")); + string_list_free(clone); + } + string_list_free(packed); + + check("NULL input", string_split_packed(NULL, "|") == NULL); + + if (fails) + { + printf("%d check(s) failed\n", fails); + return 1; + } + printf("ok string_split_packed: matches string_split in one read-only block\n"); + return 0; +} diff --git a/samples/tasks/database/Makefile b/samples/tasks/database/Makefile index 40fafd887b..63f4f9106d 100644 --- a/samples/tasks/database/Makefile +++ b/samples/tasks/database/Makefile @@ -162,7 +162,7 @@ SOURCES_C := \ SCAN_TEST_DEFINES ?= -DSCAN_READER_PARK_USEC=30000000 \ -DSCAN_READER_CUTOFF=65536 DEFINES = -DHAVE_LIBRETRODB -DHAVE_COMPRESSION \ - -DHAVE_RZSTD -DHAVE_RBZIP2 -DHAVE_RRAR \ + -DHAVE_RZSTD -DHAVE_RBZIP2 -DHAVE_RRAR -DHAVE_CORE_INFO_CACHE \ $(SCAN_TEST_DEFINES) $(SCAN_DEFINES) LIBS += -lm @@ -222,6 +222,11 @@ CLAIM_TEST_MAIN := $(CORE_DIR)/samples/tasks/database/core_info_claim_test.o CLAIM_TEST_OBJECTS = $(filter-out $(call sample_objs,$(CORE_DIR)/samples/tasks/database/main.o),$(OBJECTS)) \ $(CLAIM_TEST_MAIN) +CACHE_TEST := core_info_cache_test +CACHE_TEST_MAIN := $(CORE_DIR)/samples/tasks/database/core_info_cache_test.o +CACHE_TEST_OBJECTS = $(filter-out $(call sample_objs,$(CORE_DIR)/samples/tasks/database/main.o),$(OBJECTS)) \ + $(CACHE_TEST_MAIN) + OBJOUT = -o LINKOUT = -o @@ -288,6 +293,10 @@ $(BEGIN_TEST)$(EXE_EXT): $(BEGIN_TEST_OBJECTS) $(CLAIM_TEST)$(EXE_EXT): $(CLAIM_TEST_OBJECTS) $(LD) $(LINKOUT)$@ $(SHARED) $(CLAIM_TEST_OBJECTS) $(LDFLAGS) $(LIBS) +# Needs a writable dir for its cores, info files and cache. +$(CACHE_TEST)$(EXE_EXT): $(CACHE_TEST_OBJECTS) + $(LD) $(LINKOUT)$@ $(SHARED) $(CACHE_TEST_OBJECTS) $(LDFLAGS) $(LIBS) + # The scan test runs twice in one root: what a run leaves behind must # not decide the next one's checks. A sanitized build gives the budget # test its sanitizer's allowance, as CI does. @@ -298,11 +307,13 @@ $(CLAIM_TEST)$(EXE_EXT): $(CLAIM_TEST_OBJECTS) TEST_ROOT = root=$$(mktemp -d) && \ { ! command -v cygpath >/dev/null 2>&1 || root=$$(cygpath -m "$$root"); } check: $(INDEX_TEST)$(EXE_EXT) $(SCAN_TEST)$(EXE_EXT) $(BEGIN_TEST)$(EXE_EXT) \ - $(CLAIM_TEST)$(EXE_EXT) + $(CLAIM_TEST)$(EXE_EXT) $(CACHE_TEST)$(EXE_EXT) $(TEST_ROOT) && ./$(INDEX_TEST)$(EXE_EXT) "$$root"; r=$$?; \ rm -rf "$$root"; exit $$r $(TEST_ROOT) && ./$(CLAIM_TEST)$(EXE_EXT) "$$root"; r=$$?; \ rm -rf "$$root"; exit $$r + $(TEST_ROOT) && ./$(CACHE_TEST)$(EXE_EXT) "$$root"; r=$$?; \ + rm -rf "$$root"; exit $$r $(TEST_ROOT) && ./$(SCAN_TEST)$(EXE_EXT) "$$root" \ && ./$(SCAN_TEST)$(EXE_EXT) "$$root"; r=$$?; rm -rf "$$root"; exit $$r ./$(BEGIN_TEST)$(EXE_EXT) $(if $(SANITIZER),sanitize) @@ -324,8 +335,8 @@ $(SAMPLE_OBJDIR)/%.o: $$(call sample_src,$$*).cpp clean: rm -rf $(SAMPLE_OBJDIR) rm -f $(OBJECTS) $(INDEX_TEST_MAIN) $(SCAN_TEST_MAIN) $(BEGIN_TEST_MAIN) \ - $(CLAIM_TEST_MAIN) \ + $(CLAIM_TEST_MAIN) $(CACHE_TEST_MAIN) \ $(TARGET)$(EXE_EXT) $(INDEX_TEST)$(EXE_EXT) $(SCAN_TEST)$(EXE_EXT) \ - $(BEGIN_TEST)$(EXE_EXT) $(CLAIM_TEST)$(EXE_EXT) + $(BEGIN_TEST)$(EXE_EXT) $(CLAIM_TEST)$(EXE_EXT) $(CACHE_TEST)$(EXE_EXT) .PHONY: all check clean diff --git a/samples/tasks/database/core_info_cache_test.c b/samples/tasks/database/core_info_cache_test.c new file mode 100644 index 0000000000..2d9893c4ca --- /dev/null +++ b/samples/tasks/database/core_info_cache_test.c @@ -0,0 +1,353 @@ +/* Core info cache: a late info file is read, and a rebuild keeps every cached entry whole */ + +/* Run with: make core_info_cache_test && ./core_info_cache_test */ + +#include +#include +#include +#include + +#include + +#include +#include +#include +#include +#include + +#include "../../../core_info.h" +#include "../../../list_special.h" +#include "../../../configuration.h" +#include "../../../msg_hash.h" +#include "../../../tasks/tasks_internal.h" + +/* What the linked scanner objects reach for and this test never uses. */ +int msg_hash_get_help_us_enum(enum msg_hash_enums msg, char *s, size_t len) +{ + (void)msg; + if (s && len) + s[0] = '\0'; + return 0; +} + +const char *msg_hash_to_str_us(enum msg_hash_enums msg) +{ + (void)msg; + return ""; +} + +void msg_hash_us_index_init(void) { } + +settings_t *config_get_ptr(void) +{ + static settings_t settings; + return &settings; +} + +void runloop_msg_queue_push(const char *msg, size_t len, + unsigned prio, unsigned duration, + bool flush, char *title, unsigned icon, unsigned category) +{ + (void)msg; (void)len; (void)prio; (void)duration; + (void)flush; (void)title; (void)icon; (void)category; +} + +bool retroarch_override_setting_is_set(unsigned enum_idx, void *data) +{ + (void)enum_idx; (void)data; + return false; +} + +struct string_list *dir_list_new_special(const char *input_dir, + enum dir_list_type type, const char *filter, bool show_hidden_files) +{ + (void)input_dir; (void)type; (void)filter; (void)show_hidden_files; + return NULL; +} + +void ui_companion_driver_notify_refresh(void) { } + +void task_window_progress_cb(retro_task_t *task) { (void)task; } + +static char dir[1024]; + +static bool write_file(const char *name, const char *text) +{ + char p[1100]; + FILE *f; + snprintf(p, sizeof(p), "%s/%s", dir, name); + if (!(f = fopen(p, "wb"))) + return false; + fputs(text, f); + fclose(f); + return true; +} + +/* Builds the list with the cache on, as RetroArch does; returns the core's extensions or NULL */ +static const char *list_up(void) +{ + char core_path[1100]; + bool cache_supported = false; + core_info_t *info = NULL; + + core_info_deinit_list(); + core_info_init_list(dir, dir, "so", true, true, &cache_supported); + snprintf(core_path, sizeof(core_path), "%s/late_libretro.so", dir); + if (!core_info_find(core_path, &info) || !info) + return NULL; + return info->supported_extensions; +} + +/* True when the written cache names every core id given */ +static bool cache_names(const char *a, const char *b) +{ + char p[1100]; + static char buf[65536]; + int64_t n; + intfstream_t *f; + snprintf(p, sizeof(p), "%s/core_info.cache", dir); + if (!(f = intfstream_open_rzip_file(p, RETRO_VFS_FILE_ACCESS_READ))) + return false; + n = intfstream_read(f, buf, sizeof(buf) - 1); + intfstream_close(f); + free(f); + buf[n > 0 ? n : 0] = '\0'; + return strstr(buf, a) && strstr(buf, b); +} + +/* The cache is replaced by a rename, so a rewrite shows as a new inode */ +static unsigned long cache_ino(void) +{ + char p[1100]; + struct stat st; + snprintf(p, sizeof(p), "%s/core_info.cache", dir); + if (stat(p, &st) != 0) + return 0; + return (unsigned long)st.st_ino; +} + +static void use_dir(const char *root, const char *sub) +{ + snprintf(dir, sizeof(dir), "%s/%s", root, sub); + path_mkdir(dir); +} + +static void remove_file(const char *name) +{ + char p[1100]; + snprintf(p, sizeof(p), "%s/%s", dir, name); + remove(p); +} + +static void build_list(void) +{ + bool cache_supported = false; + core_info_deinit_list(); + core_info_init_list(dir, dir, "so", true, true, &cache_supported); +} + +#define MANY_CORES 20 + +/* More cores than the cache list starts with and long firmware lists: + * the second start is served from the cache alone (the info files are + * gone by then), and must give back what the info files said, each + * core at its own path. */ +static int check_many_cores(const char *root) +{ + int c, f, pass; + char name[64], text[4096], core_path[1100]; + + use_dir(root, "many"); + for (c = 0; c < MANY_CORES; c++) + { + size_t _len; + snprintf(name, sizeof(name), "c%02d_libretro.so", c); + if (!write_file(name, "\177ELF")) + return 1; + _len = (size_t)snprintf(text, sizeof(text), + "display_name = \"Core %d\"\nsupported_extensions = \"a%d|b\"\n" + "firmware_count = %d\n", c, c, c % 7); + for (f = 0; f < c % 7; f++) + _len += (size_t)snprintf(text + _len, sizeof(text) - _len, + "firmware%d_path = \"bios/c%d_%d.bin\"\n" + "firmware%d_desc = \"BIOS %d of core %d\"\n" + "firmware%d_opt = \"%s\"\n", + f, c, f, f, f, c, f, (f & 1) ? "true" : "false"); + snprintf(name, sizeof(name), "c%02d_libretro.info", c); + if (!write_file(name, text)) + return 1; + } + + for (pass = 0; pass < 2; pass++) + { + build_list(); + for (c = 0; c < MANY_CORES; c++) + { + char want[64]; + core_info_t *info = NULL; + snprintf(core_path, sizeof(core_path), "%s/c%02d_libretro.so", dir, c); + snprintf(want, sizeof(want), "Core %d", c); + if ( !core_info_find(core_path, &info) || !info + || !info->path || strcmp(info->path, core_path) + || !info->display_name || strcmp(info->display_name, want) + || info->firmware_count != (size_t)(c % 7)) + { + fprintf(stderr, "FAIL core_info_cache: core %d wrong on %s start\n", + c, pass ? "cached" : "first"); + return 1; + } + for (f = 0; f < c % 7; f++) + { + char fw_path[64], fw_desc[64]; + snprintf(fw_path, sizeof(fw_path), "bios/c%d_%d.bin", c, f); + snprintf(fw_desc, sizeof(fw_desc), "BIOS %d of core %d", f, c); + if ( !info->firmware[f].path || strcmp(info->firmware[f].path, fw_path) + || !info->firmware[f].desc || strcmp(info->firmware[f].desc, fw_desc) + || info->firmware[f].optional != ((f & 1) ? true : false)) + { + fprintf(stderr, "FAIL core_info_cache: core %d firmware %d wrong on %s start\n", + c, f, pass ? "cached" : "first"); + return 1; + } + } + } + if (!pass) + for (c = 0; c < MANY_CORES; c++) + { + snprintf(name, sizeof(name), "c%02d_libretro.info", c); + remove_file(name); + } + } + core_info_deinit_list(); + printf("ok core_info_cache: %d cores and their firmware come back whole from the cache\n", + MANY_CORES); + return 0; +} + +static uint32_t id_hash(const char *s) +{ + unsigned char ch; + uint32_t hash = (uint32_t)0x811c9dc5; + while ((ch = (unsigned char)*(s++)) != '\0') + hash = ((hash * (uint32_t)0x01000193) ^ (uint32_t)ch); + return hash ? hash : 1; +} + +/* A cache cut off inside an entry's firmware list is thrown away + * without leaking the half-read entry, and the info file is read. */ +static int check_truncated_cache(const char *root) +{ + char text[1024], core_path[1100]; + core_info_t *info = NULL; + + use_dir(root, "truncated"); + if ( !write_file("t_libretro.so", "\177ELF") + || !write_file("t_libretro.info", "display_name = \"From Info\"\n")) + return 1; + snprintf(text, sizeof(text), + "{\n \"version\": \"1.2\",\n \"items\": [\n" + " {\n \"display_name\": \"Whole\",\n" + " \"core_file_id\": { \"str\": \"other_libretro\", \"hash\": %u }\n },\n" + " {\n \"display_name\": \"Cut\",\n" + " \"core_file_id\": { \"str\": \"t_libretro\", \"hash\": %u },\n" + " \"firmware\": [ { \"path\": \"a.bin\", \"desc\": \"A\" }, { \"path\": \"b", + (unsigned)id_hash("other_libretro"), (unsigned)id_hash("t_libretro")); + if (!write_file("core_info.cache", text)) + return 1; + + build_list(); + snprintf(core_path, sizeof(core_path), "%s/t_libretro.so", dir); + if ( !core_info_find(core_path, &info) || !info + || !info->display_name || strcmp(info->display_name, "From Info")) + { + fprintf(stderr, "FAIL core_info_cache: a cut-off cache was trusted\n"); + return 1; + } + core_info_deinit_list(); + printf("ok core_info_cache: a cache cut off inside an entry is discarded\n"); + return 0; +} + +int main(int argc, char **argv) +{ + int i; + unsigned long ino; + const char *exts; + + if (argc < 2) + { + fprintf(stderr, "usage: %s \n", argv[0]); + return 2; + } + snprintf(dir, sizeof(dir), "%s/late_info", argv[1]); + path_mkdir(dir); + + /* A sideloaded core with no info file: cached as such */ + if (!write_file("late_libretro.so", "\177ELF")) + { + fprintf(stderr, "FAIL core_info_cache: fixture\n"); + return 1; + } + if ((exts = list_up()) && *exts) + { + fprintf(stderr, "FAIL core_info_cache: extensions without an info file\n"); + return 1; + } + + /* The info file arrives later, by hand */ + if (!write_file("late_libretro.info", + "display_name = \"Late Info\"\n" + "supported_extensions = \"cue|chd\"\n")) + { + fprintf(stderr, "FAIL core_info_cache: fixture\n"); + return 1; + } + exts = list_up(); + if (!exts || strcmp(exts, "cue|chd")) + { + fprintf(stderr, "FAIL core_info_cache: info file added later is hidden by the cache (\"%s\")\n", + exts ? exts : "(none)"); + return 1; + } + + /* The rewritten cache serves it from now on */ + exts = list_up(); + if (!exts || strcmp(exts, "cue|chd")) + { + fprintf(stderr, "FAIL core_info_cache: rewritten cache lost the info (\"%s\")\n", + exts ? exts : "(none)"); + return 1; + } + + /* A new core makes the cache be rewritten; the entries served from it must survive that */ + if (!write_file("other_libretro.so", "\177ELF")) + { + fprintf(stderr, "FAIL core_info_cache: fixture\n"); + return 1; + } + for (i = 0; i < 3; i++) + { + list_up(); + if (!cache_names("\"late_libretro\"", "\"other_libretro\"")) + { + fprintf(stderr, "FAIL core_info_cache: build %d wrote a cache that lost a core\n", i + 1); + return 1; + } + } + + /* Settled: the next start is served from the cache and writes nothing */ + ino = cache_ino(); + list_up(); + if (ino != cache_ino()) + { + fprintf(stderr, "FAIL core_info_cache: an unchanged core set rewrote the cache\n"); + return 1; + } + + core_info_deinit_list(); + printf("ok core_info_cache: a late info file is read, and rebuilds keep the cache whole\n"); + + if (check_many_cores(argv[1]) || check_truncated_cache(argv[1])) + return 1; + return 0; +}