Audit and harden the content scanner database stack by LibretroAdmin · Pull Request #19279 · libretro/RetroArch

LibretroAdmin · 2026-07-28T12:52:55Z

What this is

An audit of the content scanner and everything it calls: tasks/task_database.c, tasks/task_database_cue.c, database_info.c, manual_content_scan.c, and all six sources under libretro-db/. Every file in that chain was read. The fixes are here, along with the test coverage that demonstrates them.

Two archive-handling fixes came out of testing this against a real library and are included; they sit outside that chain and could be split out if preferred.

.rdb files are downloaded from the buildbot, so the reader has to survive whatever is in one. Most of the parser findings are of that shape.

Fixes

Parser and query (libretro-db/)

rmsgpack_dom_read_with() freed an output value it had never initialised; in the cursor loop that slot still held the previous record, so the free walked a dangling map.
rmsgpack_dom_read_into() switched on the type found in the file and consumed a different number of va_args per case, sliding the caller's list out of step. The caller now declares the type.
The container readers recursed once per level of nesting with no depth cap, so a run of 0x91 exhausted the stack.
EOF arrives from intfstream_read() as a short read, not -1, so the reader took its zero-initialised type byte for a positive fixint and returned a fabricated record forever.
binsearch() read before its count == 0 test, recursed without shrinking on a one-element range, and loaded the record offset through an unaligned cast. Nothing cross-checked the index header's count against the payload it reserved.
query_parse_table() and query_raise_unknown_function() passed identifier slices to strlcpy(), which strlen()s its source. query_parse_value(), query_parse_argument() and the compile entry point indexed buff.data[buff.offset] after query_chomp(), which stops at offset == len. Callers all pass NUL-terminated strings, so the byte read was that terminator — luck, not correctness.
query_parse_integer() accumulated result * 10 + digit into an int64_t unbounded, from a digit count the query controls.
Two file-scope statics in query.c (intermediate_res, tmp_err_buff) were shared across concurrent compiles. Both TSan-confirmed with a two-thread harness; 0 warnings after.

Scanner (tasks/, database_info.c, manual_content_scan.c)

Stack-buffer-overflow write building the serial query: the buffer was sized for a typical serial, not the one in hand.
<= count instead of < count walking the info list; a dead NULL test on list[0].
A 256-database ceiling that wrote past the per-database arrays.
manual_content_scan_set_menu_content_dir() trimmed a trailing separator using strlcpy()'s return, which is the length of its source. The source is char[PATH_MAX_LENGTH], the destination char[DIR_MAX_LENGTH] — half of it on every platform. ASan: READ of size 1 ... located 1022 bytes after 1024-byte region. Reached by any deeply nested folder picked in the file browser. Its NULL check also sat after the pointer had been dereferenced.
An over-long content directory was accepted truncated. Truncation cuts mid-component, so the scan targeted a different directory and the fragment became the system name. Now rejected — nothing is lost, since such a path already produced a broken scan.
cb_task_manual_content_scan() invoked the caller's completion callback from inside a HAVE_MENU block, so a build without menu support ran the scan and then dropped the callback. In-tree callers only supply one under HAVE_MENU themselves, which is why nothing noticed.
playlist_init() left scan_record.overwrite_playlist uninitialised — 9 of the struct's 10 members were set. It is written into the playlist and read back on the next scan to decide whether to overwrite. UBSan: load of value 190, which is not a valid value for type '_Bool'.
Assorted: GDI prune stream leak, info-list leak on cancel, quadratic playlist reopening, inverted nested-document validation, create_index leaking every key buffer, bintree reporting a failed grow as a successful insert.

Archive handling (libretro-common/)

These are outside the database chain and could reasonably be split out; they are here because a scan over a directory of .7z files stops dead without them.

RLZMA_LCLP_MAX was 3, on the stated assumption that "the 7z streams libretro-common reads do not exceed 3 either". 7-Zip routinely writes lc=4, lp=0 — a properties byte of 0x5E — and rlzma_dec_init() rejected every such stream. Since 7z holds its own header as an LZMA stream, the rejection landed at r7z_archive_open(): the archive could not be opened at all and the member list came back empty. The neighbouring RLZMA2_LCLP_MAX is already 4, and the comment describing the probability array documents the size for 4, so the constant and its own comment disagreed.
file_archive_get_file_crc32_and_size() looped with no exit. Its iterate call is guarded on the transfer still being in ITERATE, but nothing broke once it left that state, and the name tests then re-read a current_file_path that could no longer change — so a member the archive does not hold spun at full CPU indefinitely. An archive that fails to open produces an empty walk and lands in the same place, which is how it was found. It also returned whichever entry the walk stopped on, so a miss could match content against the wrong record; a miss now reports nothing.

Measured on a real DS archive: opening went from failing outright to working, and the member lookup from not returning within 240 s to 0.021 s.

Performance

The scanner asked each database the same question once per content file, and each was a full cursor walk. Building a crc index in one pass and answering from it instead:

query path    600 probes   10.59 ms each   6.35 s
index build     3 databases                0.04 s
index path    600 probes   under 1 us each 0.00 s

Serial lookups (disc content) got the same treatment, and the per-database size range now comes out of the same walk rather than two more queries. Indexes cost 8 bytes per record, are capped at a share of mem_stats_free(), and anything the index cannot serve falls back to the query path.

Equivalence was checked rather than assumed, because the scanner takes the first match and 15–24% of CRCs in the shipped databases are duplicates: 1,225 single-record lookups and 126 duplicate groups return the same records in the same order through both paths, and a composed decision over four databases agrees on all 300 items.

Tests

make check in libretro-db/samples/libretrodb, samples/tasks/database, libretro-common/samples/file/archive_file and libretro-common/samples/formats/r7z:

30 checks — parser: malformed metadata, unbounded nesting, EOF handling, truncated records, index headers, query slices fed from unterminated buffers, cursor-window edges.
12 checks — index/database pairing, memory budget, and that a binary serial survives extraction.
10 checks — end-to-end scan: builds its own databases, content and core info, drives task_push_dbscan(), and checks each game lands in its own database's playlist. Covers plain files, a cue sheet resolved through its track, and an archive member.
6 checks — malformed zip parsing, plus a member the archive does not hold, which used to hang rather than return.
4 checks — the lc/lp limit the LZMA decoder accepts, including the properties byte 7-Zip actually emits, and that the probability array is sized for the declared limit so the two cannot drift apart again.

Every case was verified to fail without its fix. libretro-db/samples/libretrodb/sweep.sh runs everything under ASan, UBSan, LeakSanitizer and TSan, and gates on the result.

A mutational fuzzer (attached to the discussion, not in the tree) ran ~115,000 cases against the patched reader with no crashes or hangs. The same fuzzer against the pre-audit code reproduces five of the findings above in under 20 seconds.

Every file touched here builds clean under -std=gnu90 -pedantic-errors. No C++ comments, non-ASCII bytes, tab-indented lines or trailing whitespace are introduced. libretro-db/c_converter.c reports 9 C89 errors, unchanged from before this series — it is the offline converter, built with the host compiler.

Three bugs I introduced and fixed here

All three are the same shape — a wrong answer with no error:

Gating the string fields on RDT_STRING stopped every disc-based system matching. serial is stored as binary in all 145 shipped databases — 206,276 records, none as string — and the readers allocate len + 1 and write the terminator, so reading one as a string was correct. Excluding binary left db_info->serial NULL, and the scanner tests that pointer before it compares, so serial matching stopped entirely while crc-keyed systems carried on working. Found by a maintainer scanning a real library: 605 ISOs and 48 bin/cue produced two entries, both exact dumps that had matched on crc instead.
The index caches were keyed by a database's position but not moved when a match promoted that database to the front, so content was filed under whichever database took the vacated slot. Reproduced end-to-end: a title recorded under a wholly unrelated system.
The teardown read dbstate->list->size after dir_list_free(), and the garbage count then drove a loop freeing pointers past the end of the cache array.

None was caught by anything in place at the time. The equivalence checks exercise one database at a time and never the promotion; no harness reached the task lifecycle; and every test fixture built its serials as strings, a shape the real data never uses, while the differentials compared the index against the query path — both reading through the same broken extraction, so they agreed with each other on all 1,225 comparisons.

All three now have regression tests that fail without their fixes, and the sweep runs them. Every field in all 145 shipped databases has since been surveyed against how the code reads it, across roughly 740,000 records; serial was the only mismatch.

What has not been done

The index memory share (an eighth of free) is a judgement, not a measurement against a real console working set.
samples/tasks/database needed three fixes to complete a scan at all, and its content enumeration still depends on a stubbed dir_list_new_special.
libretro-db/c_converter.c and bintree.c are audited but are offline-tool paths the scanner never reaches.

It has been thoroughly tested on CRC and serial-based content

intfstream_read() reports a hard error as -1 but signals end-of-file as a zero-length or short read, so every "== -1" check in the msgpack reader was blind to EOF. The visible consequence is in rmsgpack_read(): at EOF the type byte keeps its "uint8_t type = 0" initialiser, the reader takes the positive-fixint branch and returns success, handing the caller a fabricated integer 0. It does this every time it is called, so a .rdb whose trailing nil sentinel is missing - a truncated download is enough - spins libretrodb_cursor_read_item() forever. Both consumers loop on that return (the "while (ret != -1)" in database_info_list_new_filtered() and the "for (;;)" in database_info_list_new_names_only()), so the scan task never completes and never reports an error. Reproduced with a 30-byte .rdb missing its sentinel: the cursor returned 5000001 records of type RDT_INT before the test harness gave up. The same blindness let rmsgpack_skip_bytes() report success for bytes it never read, let rmsgpack_read_uint()/rmsgpack_read_int() decode a partially-filled union, and left libretrodb_open()'s header struct partly uninitialised for a file shorter than the header before handing it to strncmp(). Add rmsgpack_read_exact() and route every fixed-length read through it. Every length in a MsgPack stream is exact by construction - a type byte is one byte, a declared payload is that many bytes - so a short read is always a truncated or malformed stream. Zero the decode unions so a future miss cannot surface stale bytes, and give libretrodb.c's three open-coded reads the same treatment.

rmsgpack_dom_read_with() frees 'out' on the error path, but never initialises it. Every caller passes the address of a bare automatic: database_cursor_iterate(), database_info_list_new_names_only(), and the fast path in libretrodb_cursor_read_item(). rmsgpack_read() only writes through the pointer once a callback fires, so a stream that fails before the first callback left the error path freeing whatever happened to occupy that stack slot. In the cursor loop that slot is reused across iterations and still holds the previous record - type RDT_MAP with an 'items' pointer that rmsgpack_dom_value_free() already released - so reading a truncated .rdb walked freed pairs and then freed the pair array a second time. ASan on a two-record .rdb whose second record is a bare map header: ERROR: AddressSanitizer: heap-use-after-free READ of size 4 at 0x5040000000e0 [0] rmsgpack_dom_value_free rmsgpack_dom.c:204 [1] rmsgpack_dom_value_free rmsgpack_dom.c:215 [2] rmsgpack_dom_read_with rmsgpack_dom.c:438 [3] libretrodb_cursor_read_item libretrodb.c:504 freed by thread T0 here: [1] rmsgpack_dom_value_free rmsgpack_dom.c:218 This was previously masked: before the preceding commit the reader never failed at EOF, it looped instead. Claim the object before handing it to the reader so the error path always has a well-defined value to release, and have rmsgpack_dom_value_free() leave the value in the state a fresh RDT_NULL has. libretrodb_create() already open-coded that assignment after every call; it belongs in the free function, where it also makes a second free of the same object a no-op rather than a double free.

rmsgpack_dom_read_into() switched on the type tag stored in the file and consumed a different number of va_args per case: one output pointer for RDT_UINT, two for RDT_STRING and RDT_BINARY. Nothing checked that against what the caller had actually passed, so a .rdb that declared a key with an unexpected type slid the entire va_list out of step. Every key after the mismatch then wrote a file-controlled uint64 through a pointer belonging to a different field. libretrodb_find_index() passes five outputs, which makes that an arbitrary write rather than just a crash. A missing key was unsafe for a simpler reason: rmsgpack_dom_value_map_value() returns NULL when the key is absent and the result went straight into "switch (value->type)". Both reproduce against libretrodb_open(). A 26-byte .rdb whose metadata map is keyed "kount" instead of "count": rmsgpack_dom.c:526:23: runtime error: member access within null pointer of type 'struct rmsgpack_dom_value' AddressSanitizer: SEGV on unknown address 0x000000000010 [0] rmsgpack_dom_read_into rmsgpack_dom.c:526 [1] libretrodb_open libretrodb.c:248 and a 33-byte .rdb that stores "count" as a string, which consumes the caller's &md.count as the char* buffer and then the NULL terminator as the uint64_t* length: rmsgpack_dom.c:561:72: runtime error: load of null pointer AddressSanitizer: SEGV on unknown address 0x000000000000 Take an explicit enum rmsgpack_dom_field_type per key, read before the file's own type is consulted. The va_list now advances by an amount fixed at the call site, an absent key is a clean failure instead of a NULL dereference, and a stored type that disagrees with the requested one is a parse error. RDF_UINT and RDF_INT accept both integer spellings. MsgPack has no single canonical encoding for a small integer - a positive fixint decodes as RDT_INT while rmsgpack_write_uint() emits UINT8..UINT64, which decode as RDT_UINT - so demanding one tag would reject validly encoded files that our own writer does not happen to produce. While here, terminate the RDF_STRING output unconditionally. libretrodb_find_index() calls strlen() on idx->name, which is char[50]; a stored name of 50 bytes or more filled the buffer with no room for a terminator and ran strlen() off the end of the struct. Both .rdb files above are now rejected by libretrodb_open() with no sanitiser report, and a 500-record database still reads back byte-identically.

rmsgpack_read() recurses through rmsgpack_read_map() and rmsgpack_read_array() once per level of container nesting, with no limit. The "len > remaining bytes" guards those two functions already carry bound only how *wide* a container may be; depth costs one byte per level, so a run of 0x91 (fixarray holding one element) recurses once per byte of input. A 200 KB .rdb consisting of 0x91 exhausts the stack: AddressSanitizer: stack-overflow on address 0x7ffc1fb16f18 [4] rmsgpack_read rmsgpack.c:530 [5] rmsgpack_read_array rmsgpack.c:513 [6] rmsgpack_read rmsgpack.c:547 ... repeated to exhaustion The DOM reader's own MAX_DEPTH does not help: a one-element array pops one stack entry and pushes one, so its stack index never grows and the cap never fires. rmsgpack_skip_value(), used by the cursor fast path to step over fields the query does not reference, recurses on the same input for the same reason. Thread a depth counter through both readers and cap it at 32. Records written by libretrodb_create() nest two deep - a map of scalars - so this leaves ample headroom for any plausible schema change while keeping worst-case recursion bounded. rmsgpack_dom_value_free() and rmsgpack_dom_value_cmp() recurse over the resulting tree and are bounded by the same cap, since the reader can no longer build anything deeper. Verified at the boundary: 30 and 31 levels of nesting still parse, 40 and 200000 are rejected without a sanitiser report, and a 500-record database reads back byte-identically.

…ed load binsearch() had three defects, all reachable from a malformed index header: - the memcmp() ran before the "count == 0" test, so an empty sub-range was still compared against buff[0..field_size) - a read past the end of the allocation whenever the search narrowed to nothing; - the tail call passed "count - mid", and mid is 0 when count is 1, so a one-element range recursed on itself forever, walking further past the buffer on each step until it faulted; - the stored offset was read as *(uint64_t*)(current + field_size). field_size comes from the file, so unless it is a multiple of eight every entry after the first is misaligned. UBSan flags this on the *well-formed* index our own converter writes, and it faults outright on strict-alignment targets. field_size was also declared uint8_t while the caller passed (ssize_t)idx.key_size, so a key_size of 256 truncated to 0 and made every memcmp() a zero-length match returning a garbage record offset. Nothing related idx.next, idx.count and idx.key_size to each other either, so a header claiming a large count over a small payload sent binsearch() off the end of the heap allocation. libretrodb_find_index() had a matching hang: idx->next is a file-supplied relative seek, and zero re-reads the same header forever while a value that wraps negative seeks backwards, so intfstream_eof() is never reached. Make binsearch() iterative over a half-open range, assemble the offset with memcpy so alignment is irrelevant, widen field_size to uint64_t, cross-check count against the payload the header reserved, reject degenerate key sizes, and require forward progress when walking the index chain. Six crafted index headers, before and after: i_count (count >> payload) SEGV -> rejected i_empty (payload 0, count) heap-buffer-overflow -> rejected i_ks0 (key_size 0) bogus match -> rejected i_unalign (key_size 3) heap-buffer-overflow -> rejected i_next0 (next == 0) UBSan misaligned -> rejected i_ok (well-formed) UBSan misaligned -> found A 500-record database still reads back byte-identically.

strlcpy() returns the length of its source, not the number of bytes it copied. task_database_iterate_serial_lookup() advanced _len by that return value and then stored three bytes at query[_len], query[_len + 1] and query[_len + 2]: char query[50]; _len = strlcpy(query, "{'serial': b'", sizeof(query)); _len += strlcpy(query + _len, serial_buf, sizeof(query) - _len); query[ _len] = '\''; query[++_len] = '}'; query[++_len] = '\0'; serial_buf is the hex expansion of db_state->serial, so _len is 13 + 2 * strlen(serial) regardless of how much actually fit. The inner strlcpy() truncates correctly; it is the three explicit stores that run off the end, for any serial longer than 17 characters. Reproduced against the shipped code with ASan: serial 17 chars: _len=49 ok serial 18 chars: stack-buffer-overflow, WRITE of size 1 serial 40 chars: stack-buffer-overflow, WRITE of size 1 db_state->serial is a 4 KiB buffer carrying its own "TODO/FIXME - check size" note, and detect_dc_game() concatenates lgame_id[20] and rgame_id[20] before cue_append_multi_disc_suffix() adds to that, so close to forty characters is reachable from a Dreamcast image. Far past the buffer the store skips the sanitiser's redzone entirely and lands in live stack, where nothing reports it. Size the buffer from the string being built. The common case stays on the stack; a serial that does not fit falls back to the heap, the same way database_info_list_iterate_found_match() already handles db_crc. Verified clean from 10 to 200 characters.

task_database_iterate_serial_lookup() looped while (db_state->entry_index <= db_state->info->count) and dereferenced list[entry_index] on the final iteration, reading ->serial out of whatever follows the allocation and passing it to string_is_equal(). This is not a crafted-input path: the loop runs to completion on every serial lookup that finds no match, which is the common case for content absent from the database. task_database_iterate_crc_lookup() had the same shape without the loop - it indexed list[entry_index] whenever db_state->info was non-NULL, with no check against count, so a query that matched nothing still had list[0] dereferenced. Bound both against count. The "db_info_entry &&" tests they were guarded with were dead code - the address of an array element is never NULL - and go with them.

task_database_fill_db_min_max() ran a {size:min(0)} query and then read db_state->info->count with no NULL check. database_info_list_new_filtered() returns NULL whenever the .rdb cannot be opened, the query fails to compile, or an allocation fails, so any unreadable or malformed database in the scan directory dereferences NULL here. The second {size:max(0)} query had the same problem. Treat a failed query the way the existing empty-result branch already treats no size information: record the placeholder range and move on to the next database.

min_sizes[], max_sizes[] and flags[] were [MAX_DATABASE_COUNT] arrays declared inline in database_state_handle_t and indexed by db_state->list_index. list_index is bounded only by list->size, which is however many .rdb files dir_list_new() found in the database directory. Nothing clamped it against MAX_DATABASE_COUNT. Past 256 databases every write through those indices is out of bounds, and the most-recently-matched shuffle in database_info_list_iterate_found_match() memmove()s list_index + 1 elements over the same arrays. The shipped database set is already well over half of 256 and gains entries every release, so this is a question of when rather than whether. Allocate the three arrays from list->size once the database list is known, and release them with the rest of the handle. An empty database directory allocates nothing: every lookup path returns on "list_index == list->size" before touching them, and the one read of flags[0] is guarded by "list_index > 0".

gdi_prune() called free(fd) on its intfstream_t without calling intfstream_close() first, leaking the OS file descriptor and the inner stream object for every .gdi file in a scan. task_database_cue_prune(), five lines above, does it correctly - this is a straight omission. A large GDI collection walks the descriptor table. Separately, db_state->info is only released along the iterate paths. free_manual_content_scan_handle() never touched it, so cancelling a scan while a query result was live leaked the entire database_info_list_t: the record array plus every strdup'd field of every record in it.

type_is_prioritized() read ext[1] and ext[2] before anything established the extension was that long, and only then tested ext[3] == '\0'. path_get_extension() returns a pointer to the terminator for a path with no extension, so this reads up to three bytes past the end of the string - and it runs inside the qsort() comparator, i.e. on every entry of the database directory listing. ASan on the unpatched function, with a path carrying no extension: ERROR: AddressSanitizer: global-buffer-overflow READ of size 1 [0] in type_is_prioritized extension_to_file_type() compared a char[6] filled by strlcpy() against fixed byte counts - memcmp(ext_lower, "lutro", 6) and friends. memcmp() is specified to read all n bytes, so for any extension shorter than the count, and for the empty extension in particular, those are uninitialised stack bytes. MSan reports it and vectorised memcmp() implementations really do load them. Check the length before the character reads in the first case, and use string_is_equal(), which stops at the terminator, in the second. Prioritisation behaviour is unchanged: cue and gdi still sort first, case-insensitively, for both bare filenames and paths.

cue_append_multi_disc_suffix() bounded its snprintf() with "PATH_MAX_LENGTH - __len" no matter what buffer it had been handed. Twenty of the twenty-one call sites pass the detector's 's' output, whose size the caller knows as 'len'; detect_gc_game() passes a char[20] stack buffer. In every case the size argument described a buffer that was not the destination. Only the fact that cue_find_disc_number() maps a single character - so the append is at most four bytes - kept that from overflowing. Take the destination size as a parameter and bound the append with it. While here, fix the ordering in detect_wii_game(). It called the helper on 's' and then overwrote 's' with strlcpy(), so the suffix was computed and immediately discarded - every other detector copies the serial first and appends afterwards. The effect was that every disc of a multi-disc Wii set produced the same serial and matched the same database entry, which is the wrong one for all but the first disc. Against a synthetic Wii image with game id RMCE01: filename before after Mario Kart Wii (USA).iso RMCE01 RMCE01 Some Game (Disc 1).iso RMCE01 RMCE01-0 Some Game (Disc 2).iso RMCE01 RMCE01-1 Some Game (Disk B).iso RMCE01 RMCE01-1

…cators remove_disc_indicators() matched eight prefixes but then skipped a hard-coded seven characters to find the indicator text. That is correct for " (Disc " and its three siblings; the tape and side prefixes deliberately omit the leading space and are six characters long. For "(Tape 1)" the indicator was therefore taken to start at the ')' rather than the '1', giving a zero-length indicator that is_valid_disc_indicator() rejects. The net effect is that no tape or side indicator has ever been stripped, so those titles collapse differently from disc titles when M3U entries are merged. When the closing parenthesis fell before the mis-computed start the subtraction wrapped, but is_valid_disc_indicator()'s "len > 10" sanity check caught the result, so this was a behaviour bug rather than an out-of-bounds read. Compare end_pos against the start anyway now that the offset is per-pattern. Table-drive the patterns so each carries its own length. Differential over the old and new implementations: input old new Final Fantasy VII (Disc 1) Final Fantasy VII Final Fantasy VII Some Game (Disk A) Some Game Some Game Elite (Tape 1) Elite (Tape 1) Elite Head Over Heels (Side A) Head Over Heels (..) Head Over Heels Game (Disc 1 of 2) Game Game Game (Europe) Game (Europe) Game (Europe) Game (Disc Extra Content) unchanged unchanged Disc and disk handling is unaffected; only the tape and side prefixes change, and non-indicator parentheticals are still left alone.

libretrodb_validate_document() recursed into nested maps with if ((rv == libretrodb_validate_document(&value)) != 0) return rv; "==" where "=" was meant. rv is 0 at that point, so the condition is true exactly when the recursive call returned 0 - a *valid* nested document - and then returns rv, which is still 0. When the recursive call returned -1 for an invalid document the condition was false and the loop simply carried on. The check is therefore inverted: illegal keys nested inside a sub-map, which is what the "$" prefix test exists to reject, were accepted, while a valid sub-map short-circuited the rest of the outer document's validation. Driving libretrodb_create() with a document whose nested map carries a "$"-prefixed key: nested key before after "ok" accepted accepted "$bad" accepted <- should not be rejected (-1) This only affects the database creation path (c_converter, libretrodb_tool), not runtime scanning.

add_files_from_archive() writes the archive member name at new_path + _len + 1, so the space remaining is sizeof(new_path) - _len - 1. The bound passed to strlcpy() was sizeof(new_path) - _len, one byte more than exists. The enclosing "if (_len + strlen(...) + 1 < PATH_MAX_LENGTH)" test keeps the extra byte unreachable today, so this is not a live overflow - but the guard and the bound have to describe the same space or the next change to either lands on it.

scan_results_batch_update_playlists() decides whether to switch playlists with if (!single_playlist && (!current_playlist || !string_is_equal(current_playlist, result->db_name))) but when task_config->playlist_file is set the branch inside assigns current_playlist = task_config->playlist_file, a path such as ".../MyList.lpl". That never compares equal to a db_name such as "Sega - Genesis.lpl", so the test is true on every iteration: each result writes the playlist out in full, frees it, and calls playlist_init() to parse it back from disk. A scan producing N results performs N complete loads and N complete writes of the same file, and every intermediate write is thrown away. Compare against the key that actually identifies the target playlist - the fixed path when one is configured, the database name otherwise - so the fixed-file case opens once and accumulates. Two other problems in the same function: playlist_init()'s result was passed to seven playlist_set_scan_* calls before "if (!playlist)" was reached. Not all of those are NULL-guarded - playlist_set_scan_search_recursively(), playlist_set_scan_overwrite_playlist(), playlist_set_scan_db_usage(), playlist_set_scan_omit_db_ref(), playlist_set_scan_filter_dat_content() and playlist_set_scan_search_archives() dereference unconditionally, as do playlist_set_sort_mode(), playlist_qsort() and playlist_write_file() later - so a failed init was a NULL dereference, not a skipped entry. Move the check to the assignment. The final cleanup decided whether to free the playlist by comparing current_playlist against task_config->playlist_file, using a path string as a proxy for "is this the borrowed manual_scan->playlist". With a fixed playlist file that test also matched the playlist this function had opened itself, which then leaked. Compare the handles, which says what was meant.

…ompile query_func_min() and query_func_max() report "smaller/larger than everything seen so far", so the last row a cursor yields carries the actual extreme. That accumulator lives in a file-scope intermediate_res, and the only place it was reset is libretrodb_query_compile(). A compiled query outlives the walk that uses it - libretrodb_cursor_open() takes a reference to it - so opening a second cursor over the same query starts with the previous walk's extreme still in place. Nothing is then smaller than the running minimum and the query yields no rows at all: walk 1 (min size) 131072 131072 walk 2, same query -1 131072 before after This is latent with the current call pattern, because database_cursor_open() compiles a fresh query for every database_info_list_new_filtered() call. It becomes live the moment a compiled query is cached across walks, which is the obvious fix for the per-(file x database) recompilation cost in the scanner. Reset at libretrodb_cursor_open(), which is the accumulator's actual lifetime boundary, and keep the compile-time reset. Also make intermediate_res static; it was an unqualified global symbol and nothing outside query.c refers to it. This does not make min()/max() thread-safe. The accumulator is still shared, so two cursor walks evaluating them concurrently corrupt each other. Fixing that means moving the state into struct query, which needs a context parameter added to rarch_query_func and threaded through all nine query_func_* implementations and their recursive invocation sites - a bigger change than this one, noted in the comment on intermediate_res.

dom_read_map_start() and dom_read_array_start() assigned v->val.map.len / v->val.array.len before calling calloc(). On allocation failure they returned -1 with the value already marked RDT_MAP or RDT_ARRAY, carrying a non-zero length and a NULL items pointer. That error propagates to rmsgpack_dom_read_with(), whose cleanup path calls rmsgpack_dom_value_free() - which walks items[0..len) and dereferences NULL. The allocation size is attacker-influenced. len is bounded only by half the bytes remaining in the file, so a large .rdb carrying a MAP32 header sized past the process' remaining memory reaches it. A 2 MB .rdb claiming 1000000 pairs, run with a constrained allocator: rmsgpack_dom.c:219: member access within null pointer of type 'struct rmsgpack_dom_pair' AddressSanitizer: SEGV on unknown address 0x000000000010 [0] rmsgpack_dom_value_free rmsgpack_dom.c:204 [1] rmsgpack_dom_value_free rmsgpack_dom.c:219 [2] rmsgpack_dom_read_with rmsgpack_dom.c:473 [3] libretrodb_cursor_read_item libretrodb.c:569 Publish the length only once storage backs it, so a failed allocation leaves a well-formed empty container for the cleanup path to release. Handle the zero-length case explicitly while here. An empty map or array is legal MsgPack, but calloc(0, n) is permitted to return NULL and the old code could not distinguish that from failure - so on a platform whose allocator does return NULL for a zero request, a record containing an empty container failed to parse. Verified that a record with an empty map and one with an empty array both still read back, alongside the existing corpus and the 500-record byte-exact readback.

string.buff, binary.buff, map.items and array.items all sit at the same offset in the rmsgpack_dom_value union. The extraction paths read val->val.string.buff for name, serial, genre, region and the rest without checking val->type, so a field stored with an unexpected type handed strdup() a pointer to the wrong kind of data - the raw bytes of a binary field, or the head of a map's pair array - and that went into the playlist entry verbatim. Reading a .rdb whose "name" is stored as something other than a string, via the field-filtered path the scanner uses: stored type before after uint (null) (null) binary \x01\x02\x03\xff (null) map \x01 (null) The scalar case was already harmless: uint_ and int_ occupy offset 0 while buff is at offset 8, so buff reads the calloc'd zero and the existing "vs && *vs" test rejects it. The string and binary readers NUL-terminate their buffers, so the binary case stays inside its allocation too. The map case has nothing bounding it - strdup() walks a pair array looking for a zero byte - but that array is calloc'd and in practice contains one early. So this is a correctness fix, not a memory-safety one: the visible symptom is a garbage playlist label rather than a crash. The md5 and sha1 fields already gate on val->type; the string fields simply never did. Do the same for them, in both database_cursor_iterate() and database_cursor_iterate_filtered(), and for "size", which read val_.uint_ regardless of type. Also NULL-check the key's buffer before strlen().

The four task_title constructions use _len = strlcpy(task_title, msg_hash_to_str(...), sizeof(task_title)); strlcpy(task_title + _len, x, sizeof(task_title) - _len); strlcpy() returns the length of its source rather than the number of bytes copied, so when the message does not fit, _len exceeds the buffer: "task_title + _len" is an out-of-bounds pointer and "sizeof(task_title) - _len" wraps to a huge size_t. This is not currently reachable. task_title is char[128] and the longest shipped string among the four is 26 bytes (MSG_MANUAL_CONTENT_SCAN_PLAYLIST_CLEANUP), so there is a wide margin. It is the same misuse of the return value that overflowed the serial query buffer, though, and these strings come from Crowdin, so make the pattern safe rather than relying on the margin. Skip the append when the first copy already filled the buffer - there is no room for it in that case anyway - which keeps the existing single-pass idiom rather than reaching for strlcat().

query_func_min() and query_func_max() keep the running extreme in a file-scope intermediate_res. Two queries evaluated at the same time - a scan task walking a size probe while the menu populates a database view - write to it concurrently. ThreadSanitizer, two threads each doing what database_info_list_new_filtered() does (own db handle, own compiled query, own cursor): WARNING: ThreadSanitizer: data race Write of size 8 by thread T2: [1] query_func_min query.c:310 [1] libretrodb_query_compile query.c:1021 Location is global 'intermediate_res' of size 24 The accumulator is per-evaluation state, so put it in the compiled query and pass it to the evaluation functions instead of reaching for a global. rarch_query_func gains a struct query_ctx * first parameter, threaded through all nine query_func_* implementations, the recursive invocations inside query_func_operator_or() and query_func_operator_and(), and the three entry points (libretrodb_query_filter, libretrodb_query_eval_field and query_func_all_map). libretrodb_query_reset_accumulator() now takes the query whose accumulator to clear; libretrodb_cursor_open() passes the query it was handed. Race reports on intermediate_res under the harness above: 1 before, 0 after. Behaviour is unchanged - the corpus still parses clean, the 500-record database still reads back byte-identically, min() over a reused query still returns 131072 on both walks, and the crc fast path still matches the expected record. This does not address the other shared static in this file, tmp_err_buff in libretrodb_query_compile(), which the same harness also flags.

libretrodb_query_compile() formatted its diagnostics into a function-scope static and returned that address through err_string, so the buffer was shared process-wide. Two concurrent compiles that both fail overwrite each other's message, and the caller reading err_string can see text belonging to the other thread's query. ThreadSanitizer, two threads compiling a malformed query against the same database: WARNING: ThreadSanitizer: data race Location is global 'tmp_err_buff.0' of size 256 The pointer has to outlive the call, which is why it was static in the first place - the query object is freed on the error path, so it cannot live there. The db handle can: it is already a parameter, it outlives the compile, and every caller uses it immediately afterwards. Move the storage onto struct libretrodb and expose it through libretrodb_query_err_buf(). Compiling without a handle now fails with a fixed message instead of writing anywhere, rather than teaching ten snprintf() sites to accept a NULL destination. All four in-tree callers - database_info.c, libretrodb_tool.c twice, and lua/testlib.c - already pass an open handle. With the preceding commit this clears both shared statics in query.c. The failing-query harness reports two race locations before (intermediate_res and tmp_err_buff) and zero warnings after. Behaviour is unchanged: the corpus parses clean, the 500-record database reads back byte-identically, min() over a reused query is still correct, the crc fast path still matches, and a malformed query still reports "7::Expected '}' found ':'".

A content scan spends essentially all of its time walking record streams. Timing the three phases of one probe against the databases shipped today: Nintendo - NES open 0.07ms compile 0.006ms walk 22.6ms Sega - Genesis open 0.05ms compile 0.007ms walk 6.6ms Sony - PS1 open 0.07ms compile 0.006ms walk 15.7ms The walk is 99.7% of it, and with the file already in the page cache most of that is per-read overhead through intfstream/filestream/VFS rather than parsing - the same walk over a memory-backed stream runs about 2.2x faster (17.9 -> 8.1 ms for NES, 14.2 -> 6.1 ms for PS1). Read the database in once at libretrodb_cursor_open() and walk it from memory. Best-effort: a database larger than LIBRETRODB_MAX_IMAGE_SIZE, or an allocation that fails, keeps the file-backed stream, so behaviour degrades to what it does now rather than failing. The largest database shipped today is about 8 MB against a 32 MB cap. The image is owned by the cursor rather than shared with the db handle. libretrodb_cursor_open() already opened its own stream, and some callers (lua/testlib.c) manage cursor and db lifetimes independently, so tying the image to the db would make closing the db first a use-after-free. Measured on the scan pattern - 20 content files probed against all three databases, which is what task_database_iterate_crc_lookup() does: before 60 probes in 0.84 s (14.0 ms/probe) after 60 probes in 0.45 s ( 7.5 ms/probe) Results are unchanged. Dumping every record of all three databases through the cursor walk is byte-identical before and after (30359, 7398 and 13487 records), as is a 40-CRC query differential across all three (79 matches). The malformed-input corpus still parses the same way, the 500-record readback is still byte-exact, memory streams report EOF as a zero-length read exactly as file streams do so the short-read checks are unaffected, and ASan/UBSan/LeakSanitizer are clean over all three real databases. Forcing every image allocation to fail still yields the full 7398 records via the fallback.

The preceding commit read the whole database into one buffer so that the walk's very many small reads became memcpy instead of a trip through filestream/VFS/fread. That works, but the buffer is the size of the database - about 7 MB for Nintendo - Nintendo Entertainment System - and needed a 32 MB ceiling with a silent fallback above it. A fixed sliding window gets the same effect without the footprint. Reads inside the window are a memcpy; the window is refilled by seek+read when one is not. A 6.7 MB walk needs about a hundred refills at 64 KB, against roughly 800000 individual freads before. approach probe peak RSS size cap file-backed (two commits ago) 14.0 ms +0 none whole-file image (previous) 7.5 ms +6984 KB 32 MB sliding window (this) 8.3 ms + 564 KB none Twelve times less resident memory for six percent of the time, and the ceiling goes away: a database larger than any cap now costs the same 64 KB as a small one. Window size is a compile-time knob. Measured 32/64/128/256 KB, probe time is flat across all of them (8.0-8.3 ms) while resident memory tracks the window, so the smallest workable size wins. 64 KB leaves room for the largest backwards jump the cursor makes - a rewind to the start of the record being inspected - which has to stay inside the window or it costs a refill. intfstream_open_buffered() is implemented over filestream alone. A data_transfer streaming window was tried first and rejected: it is slower (9.0 ms, from 80000 minor faults committing pages as the window slides against 5500 for the image), it holds more (1016 KB), and data_transfer_reserve_supported() is false wherever the platform cannot reserve address space - which is the newlib consoles, i.e. exactly the platforms whose memory this was meant to save. A plain malloc'd window has no such requirement and beats it on every axis. Verified against the three databases shipped for NES, Genesis and PlayStation: every record dumped through the cursor walk is identical to before (30359, 7398 and 13487 records), ASan/UBSan/LeakSanitizer report nothing, the malformed-input corpus parses unchanged, the 500-record readback is still byte-exact, and the crc fast path still matches.

Every case corresponds to a defect that was live at some point in this file's history, so the reader now has something that fails loudly if any of them come back. metadata key absent the NULL from a failed map lookup fed straight into "switch (value->type)" metadata key mistyped rmsgpack_dom_read_into() switched on the type found in the file and consumed a different number of va_args per case metadata count as fixint a legal alternative encoding that the type check must not reject nesting past the limit the readers recursed once per level of container nesting with no depth cap sentinel missing EOF arrives as a short read, not -1, so the reader returned a fabricated record forever record truncated mid-map the reader freed an output value it had never initialised, which in the cursor loop still held the previous record empty containers calloc(0) may return NULL and that was read as an allocation failure index headers binsearch() read before its count == 0 test, recursed without shrinking on a one-element range, and loaded the record offset through an unaligned cast, with nothing cross-checking count against the payload the header reserved window spanning records have to survive crossing the cursor's sliding-buffer boundary field wider than window a read the window cannot serve must go to the file rather than drop the record The test builds each .rdb itself rather than shipping binaries, so what every byte is for stays readable, and it does not use the library under test to produce its own inputs. Output is unbuffered and each case announces itself before it runs: several of these crash outright when the defect is present, and the line already printed is what says which one. Verified to discriminate, not just pass: upstream 9094fc6 (pre-audit) segfaults on case 1 audit_db 69387f9 (no window) 16/16, no false failures with the oversized-read fix backed out "field wider than the window" fails Wired into the existing sample Makefile alongside libretrodb_leak_test, including its SANITIZER= convention; "make check" runs both. Clean under -fsanitize=address,undefined with leak detection on.

libretrodb_query_compile() takes a pointer and a length, but the parser treated the query as a C string in two places and read past the length in three more. query_parse_table() copied a parsed identifier with strlcpy(dest, ident_name, _len + 1); strlcpy() strlen()s its source to compute a return value, and ident_name points into the query buffer, which carries no terminator of its own. Reading a query from an exactly-sized heap allocation: ERROR: AddressSanitizer: heap-buffer-overflow READ of size 21 [1] strlcpy_retro__ compat_strl.c:30 [2] query_parse_table query.c:853 [3] libretrodb_query_compile query.c:1076 query_raise_unknown_function() had the same pattern on the function name it reports. Both now memcpy the slice they measured and terminate it themselves. query_parse_value(), query_parse_argument() and the entry point in libretrodb_query_compile() each indexed buff.data[buff.offset] directly after query_chomp(). Chomping stops at offset == len, so a query ending in whitespace - or consisting only of whitespace - read one byte past. query_peek() already guards this way; these three did not. Callers in tree pass strlen() of a NUL-terminated string, so the byte read was that terminator and the parse still came out right. That is what kept this from being visible, not anything in the parser. Two more while here: query_parse_integer() accumulated result * 10 + digit into an int64_t with nothing bounding it. The digit count comes from the query, so a long run of digits is signed overflow, which is undefined. Reject the number instead. An empty binary string b"" sizes its allocation to (0 + 1) / 2 == 0, and calloc(0) may return NULL, which the check below reports as an allocation failure - the same shape as the container-length fix in rmsgpack_dom.c. Allocate at least one byte. Verified with eight queries fed from exactly-sized heap buffers with no terminator, so any read past the length is a heap overflow the sanitizer sees: clean after, and reverting the fixes puts the overflow back. Real crc queries against the shipped NES, Genesis and PlayStation databases still match the same records, and every record of all three still dumps identically.

Eight queries fed to libretrodb_query_compile() from exactly-sized heap buffers with no terminator, which is what its (pointer, length) signature says is allowed. Any read past the length is then a heap overflow the sanitizer reports, rather than landing on a NUL that happened to be there - which is what hid these until now. Covers identifier slices, binary strings, queries ending at a value, queries ending in whitespace, a query that is nothing but whitespace, an integer long enough to overflow the accumulator, an empty binary string, and an unknown function name. Whether a given query compiles or is rejected is not the assertion - both are legitimate answers, and the point is that deciding does not read out of bounds. Verified to discriminate: with the parser fixes reverted the suite reports a heap-buffer-overflow at the first case.

manual_content_scan_set_menu_content_dir() copies the caller's path into scan_content_dir and then trims a trailing separator with if ((_len = strlcpy(scan_content_dir, content_dir, sizeof(scan_content_dir))) <= 0) goto error; if (scan_content_dir[_len - 1] == PATH_DEFAULT_SLASH_C()) scan_content_dir[_len - 1] = '\0'; strlcpy() returns the length of its source, not the number of bytes written, and here the source is longer than the destination can be: the path is first expanded into a char[PATH_MAX_LENGTH], while scan_content_dir is char[DIR_MAX_LENGTH] - half of PATH_MAX_LENGTH on every platform (2048/1024 on desktop, 512/256 on the consoles). So for any content directory longer than DIR_MAX_LENGTH the index is past the end of the buffer, both for the read and for the store that may follow it. A deeply nested folder chosen in the file browser is enough; the path never has to be unusual. The same function verbatim, with its destination on the heap so the bound is unambiguous: [probe] strlcpy returned _len=2047, buffer is 1024, about to touch index 2046 AddressSanitizer: heap-buffer-overflow READ of size 1 [0] manual_content_scan_set_menu_content_dir located 1022 bytes after 1024-byte region Clamp to what was actually written. Also move the sanity check ahead of the expansion. It read fill_pathname_expand_special(_tmpbuf, content_dir, sizeof(_tmpbuf)); content_dir = _tmpbuf; if (!content_dir || !*content_dir) which dereferences the caller's pointer first and then tests a variable that has just been reassigned to a stack array, so the NULL half of the check could never fire. task_push_dbscan() and menu_cbs_ok.c both reach here with paths from outside this file. Behaviour for valid paths is unchanged: short paths are accepted with their trailing separator trimmed, NULL and empty are rejected as before. An over-long path is still silently truncated rather than rejected; that is pre-existing and left alone, since changing it would alter what happens for users who currently get a truncated scan.

libretrodb_create_index() mallocs a (key_size + 8) buffer per record and hands it to bintree_insert(). The tree stores values by pointer and never owns them, and bintree_free() only clears the pointer, so every one of those buffers leaked: before 2000 records: 24000 bytes leaked in 2000 allocations 20000 records: 240000 bytes leaked in 20000 allocations after no leaks The same loop also skipped its own cleanup: the "field not found" continue jumped over the rmsgpack_dom_value_free() at the end of the body, leaking a whole DOM value per record that does not carry the indexed field. Release the key buffers through bintree_iterate() before tearing the tree down, and free the DOM value on the continue. bintree itself reported two failures as success: - bintree_insert() returned 0 - indistinguishable from a completed insert - when it reached a NULL node, which is what an earlier failed bintree_new_nil_node() leaves behind. The caller then wrote an index that silently omitted whatever fell into the hole. - bintree_new() returned a tree whose root allocation had failed, so every subsequent insert took that same path. Report -2 for an allocation failure, distinct from the -1 that means an equal value is already present, and fail bintree_new() outright if it cannot create a root. Also allocate both nil children before committing either, so a half-grown node cannot be left in the tree. Document the ownership rule in the header, since it was only discoverable by reading bintree_free(). Behaviour is otherwise unchanged: an index over unique keys is still built and is still usable - a create-then-look-up round trip resolves crc 337 to the right record through binsearch() - and a database with duplicate keys is still refused, which is every database shipped today. Note this whole path is reachable only from libretrodb_tool; the scanner never creates or reads indices.

Nothing exercised libretrodb_create_index() or libretrodb_find_entry() together, which is the only path that reaches bintree_insert() and binsearch() - both of which have been corrected in this series without an end-to-end check behind them. Builds an index over 600 unique keys, then resolves one from the middle of the range so the search has to descend, and requires the right record back. Under the suite's ASan+UBSan+LeakSan build this also pins the key-buffer leak that index creation used to have. 26 checks, 0 failures.

Disc content matches on serial rather than crc, and that lookup walks the whole database per content file exactly as the crc one did. Measured against the shipped PlayStation Portable and PlayStation databases, a serial probe is 3.06 ms and 8.59 ms - the same full walk. Give it the same index. A serial is variable-length, so the index keeps a hash rather than the bytes and reads candidate records back to compare them exactly: a collision costs an extra read, never a wrong match. 200 content files against the PlayStation Portable, PlayStation and Genesis databases: query path 3.37 s 600 probes, 5.61 ms each index build 0.02 s 3 databases index path 0.00 s 600 probes, under a microsecond each The second half of this fixes a bug in the crc index from the preceding commit, which the serial work found. Both lookups gather matching records into a fixed buffer, and both stopped filling it when it was full and returned what they had. That is a shorter list than the query path returns for the same key - a wrong answer, not a slow one. Databases really do carry placeholder keys: Sega - Mega Drive - Genesis has a "00000000-00" serial shared by hundreds of records, and comparing the two paths over its serials gave 89 mismatches until this was fixed. The preceding commit's message claimed a key shared by more records than the buffer holds "returns NULL and the query path runs instead". It did not - it truncated. It does now, on both paths. Equivalence re-checked over both, comparing every extracted field of every returned record in order: crc NES / Genesis / PS1 / PSP 600 each, 0 mismatches serial PSP / PS1 / NES 500 each, 0 mismatches serial Genesis 500: 411 answered from the index, 89 correctly handed to the query path, 0 mismatches The differential now models the fallback rather than counting it as a difference, since NULL is the documented "cannot serve this" signal the scanner acts on.

A path longer than DIR_MAX_LENGTH was copied in truncated and accepted. Truncation cuts mid-component, so what is stored is a different directory: input length 2161 -> accepted, kept 1023 stored tail ".../subdirectory_number_041/subdirectory_n" system name "subdirectory_n" The scan then runs against a path that in all likelihood does not exist, reports nothing found, and gives no indication why. If something does sit at the shortened path it scans that instead. The tail also becomes the content-directory system name, so any entries produced would be filed under a fragment of a folder name. Nothing is lost by refusing. A path this long already produced one of those two outcomes; failing here surfaces it as an invalid content directory, which is what the menu already reports for the NULL, empty and single-slash cases the function rejects a few lines above. The bound is exact: DIR_MAX_LENGTH - 1 is accepted and stored whole, DIR_MAX_LENGTH is refused. length 1023 accepted stored 1023 length 1024 rejected stored 0 The preceding fix in this function clamped the index instead, which made the read safe but left the truncated path in place. This replaces the clamp; the out-of-bounds index it was there to prevent cannot arise once the copy has to have fit.

The per-database size range is queried lazily, guarded by if (db_state->min_sizes[db_state->list_index] == 0) task_database_fill_db_min_max(db_state); Zero is also what an unqueried slot holds, so a database whose probe legitimately yields zero is asked again for every content file - and each ask is two full walks of that database. Reachable, though not by anything shipped today. query_func_min() treats a zero specially: a zero-sized record matches, and so does every record after it, so the stored value is the size of the last match rather than the smallest. That lands on zero when the last matching record is itself zero-sized. Of the databases shipped for NES, Genesis, PlayStation and PlayStation Portable, none do: NES min 2048 max 67108880 50.2 + 11.5 ms Genesis min 8192 max 15466496 13.3 + 3.5 ms PS1 min 738528 max 751746240 41.9 + 8.7 ms PSP min 1605632 max 7815495680 12.7 + 3.1 ms A database built to hit it - descending sizes ending in a zero-sized record - stores min 0 and re-probes indefinitely. Record the probe in the per-database flags instead, set before any of the function's exits so that every one of them counts as having been asked, including the paths that store the -1 placeholder. Nothing else changes: the same queries run, once. Worth noting for later that those two walks are now a visible share of a scan. With crc and serial lookups indexed, the remaining cost is about 60 ms per database of size probing against roughly 50 ms of index building - the size range could come out of the same pass that builds the index rather than costing two more walks.

The per-database size range came from two queries, {size:min(0)} and {size:max(0)}, each a full walk that also materialises every record it matches - and min() matches a lot of them, since it reports everything smaller than the running value. Measured against the shipped databases that pair costs far more than the index build it sits next to: NES queries 232.2 ms index build 18.4 ms Genesis queries 374.9 ms index build 5.1 ms PS1 queries 214.6 ms index build 11.6 ms PSP queries 459.3 ms index build 3.5 ms The index already walks every record. Collect the size range in that same pass and the two queries go away entirely: libretrodb_scan_field() now takes a companion numeric field reported alongside the key, and the crc index keeps the smallest and largest it saw. The range is the true minimum and maximum rather than what the queries returned, and that is a deliberate difference. It is identical on every database shipped today. It differs only where min() misreports: a zero-sized record matches, and so does every record after it, so the stored value ends up being the last match rather than the smallest. On a database built to hit that the queries give [0, 0] - which would skip every database for any content with a size - against [0, 3072000] from the walk. The range is only used to skip databases that cannot hold a file of a given size, so a wider range can only mean fewer skips, never a missed match. HAS_SERIAL is set rather than derived, because this walk scans crc and does not observe serial keys. Leaving it clear would skip every database for disc content, which is a missed match; setting it costs at most one wasted serial-index build for a database carrying no serials, and every database shipped today carries some. HAS_CRC is set for symmetry - nothing reads it beyond a debug line. Both equivalence differentials were re-run after the callback signature changed: crc 400 keys across four databases and serial 500 across three, comparing every extracted field of every returned record in order, 0 mismatches.

Builds and runs the two sample tests under AddressSanitizer, UndefinedBehaviorSanitizer and LeakSanitizer, and the threaded harnesses under ThreadSanitizer, returning non-zero if anything reports so it can gate a change. The in-tree tests run with no setup. The larger checks - real databases, a malformed-input corpus, concurrent scans - need .rdb data far too big to keep in the repository, so those are opt-in through SWEEP_CORPUS pointing at a directory holding them: ./sweep.sh in-tree tests only SWEEP_CORPUS=~/rdb-corpus ./sweep.sh everything Reachable as "make sweep" alongside the existing "make check". Written while auditing this code, where it repeatedly caught things reading the diff did not - most recently a signature change that compiled everywhere except the test that exercised it.

The scanner promotes a matched database to the front of its list so the next content file tries it first, moving the entry along with its size bounds and flags. The crc and serial index caches added recently are keyed by the same position and were not moved with it. After the first match at a non-zero position, therefore, the index at a slot described a different database than the entry at that slot. The lookup answered from whichever database its index had been built over, and the scanner recorded the result against whichever database now occupied the slot. Reproduced over the shipped NES, Genesis and PlayStation databases, probing a crc only PlayStation holds: before slot 2 Sony - PlayStation -> radicalgames@psygnosis v.3 after slot 0 Sony - PlayStation -> (no match) slot 2 Sega - Mega Drive -> radicalgames@psygnosis v.3 so a PlayStation title would have been filed as a Mega Drive one, with nothing reporting an error. Move the caches with everything else that is keyed by position. The lookups now also take the database the caller believes the index describes, and refuse when it does not match. That cannot happen after this fix, which is the point: if the pairing is ever broken again the result is the query path rather than another system's records. The equivalence checks could not have caught the original mistake, since they exercise one database at a time and never the promotion. Found while adding the memory ceiling below, which is the other half of this commit. Indexes live for the whole scan and had no ceiling. Roughly 360 KB for the largest database shipped today is comfortable on desktop and not on the consoles, which is exactly where DIR_MAX_LENGTH is already halved for the same reason. Both builders now take a byte allowance and give up rather than exceed it - a partial index would miss matches without saying so - and the scan tracks a budget across databases, 24 MB on desktop and 2 MB where memory is tight. Running out only means later databases use the query path, which is slower and not wrong.

Covers the mistake fixed in the preceding commit and the ceiling added with it. Lives beside the existing database sample because it needs database_info.c, which the libretro-db samples cannot link. Builds two databases of its own, then checks that a lookup against the index for that database returns the right record, and that a lookup against an index built over a *different* database is refused rather than answered. The second is the one that matters: it is what turns a broken pairing into a slow scan rather than content filed under the wrong system. Also checks that an index asked for more than its allowance is refused outright rather than returned short, and that one built within its allowance is still complete. Verified to discriminate: with the pairing check backed out, the two mismatch cases return the other database's record and the suite fails. Reachable as "make check" in that directory; the SANITIZER= variable the Makefile already honoured applies. The new file needed .gitignore fixing to land. Line 96 read "database", part of the block covering the runtime directories RetroArch creates when run from the source tree - saves, config, playlists and so on. Unanchored, it also matched samples/tasks/database, so anything added there was invisible; the two files already in that directory predate the rule. Anchored to /database it still covers the runtime directory, verified, and nothing else in the tree becomes newly visible.

The teardown frees the database list and then sized the index-cache loops from it: if (dbstate->list) dir_list_free(dbstate->list); ... size_t cn = dbstate->list ? dbstate->list->size : 0; for (ci = 0; ci < cn; ci++) database_info_crc_index_free(dbstate->crc_index[ci]); dir_list_free() does not clear the caller's pointer, so the guard passes, ->size is read out of freed memory, and whatever it returns then drives a loop that frees pointers past the end of the cache array. That is a use-after-free feeding an out-of-bounds walk, on the path taken whenever a scan finishes or is cancelled. Mine, from the commit that added those caches. Take the count first, and clear the list pointer once it is gone so a later read fails loudly instead of silently. Not caught by anything already in place: the harnesses and the regression tests drive lookups, not the task lifecycle, so no sweep ever reached this teardown. The sample in samples/tasks/database would reach it, but its content enumeration is stubbed out, so the scan never completes. Worth knowing that the teardown path still has no automated coverage; this was found by reading the diff for position-keyed state after the pairing bug, not by a tool.

The sample passed NULL for cache_supported. core_info_list_new() writes through it unconditionally: *cache_supported = true; core_info.c:2307 so the sample died on startup, before reaching the scan it exists to exercise: runtime error: store to null pointer of type '_Bool' AddressSanitizer: SEGV ... in core_info_list_new Pre-existing and outside the database chain, but it blocks running the scanner under the sanitizers at all, which is what this sample is for. Fixed on the caller's side, since the other caller in retroarch.c passes a real pointer and the parameter is not documented as optional. A guard in core_info_list_new() would be the more defensive choice; that is a call for whoever owns that file. The sample still does not complete a scan - dir_list_new_special() is stubbed to return NULL there, so no content is enumerated - but it now gets far enough to be useful under a sanitizer.

dir_list_new_special() was stubbed to return NULL for every request. The scanner only asks it for DIR_LIST_DATABASES, which retroarch.c answers with a plain "rdb" listing of the directory, and dir_list_new() is already linked here, so the stub can answer that much honestly rather than claiming no databases exist. This does not make the sample complete a scan. Driven over three real .rdb files and a directory of content, it still reports MSGQ: 0% MSGQ: 100% scan did not finish within 120 seconds so the task runs to completion of its work and never sets RETRO_TASK_FLG_FINISHED, or sets it without the callback firing. That is in the task-queue plumbing rather than anything the database chain does, and it is why nothing in this tree exercises the scan teardown - the path where a use-after-free in the index caches sat undetected through every sanitizer run in this series. Getting this sample to finish a scan is worth more than any further static review of the code it drives; it is the only harness that reaches the task lifecycle at all.

cb_task_manual_content_scan() invoked the caller's completion callback from inside a HAVE_MENU block: #if defined(HAVE_MENU) end: if (manual_scan && manual_scan->user_cb) manual_scan->user_cb(task, task_data, user_data, err); /* menu refresh */ #endif so a build without menu support ran the scan to completion and then dropped the callback. Anything waiting on it waited forever. Nothing in tree noticed because the in-tree callers only supply a callback under HAVE_MENU themselves - retroarch.c sets cb_task_dbscan inside the same guard, and the menu_cbs callers exist only in menu builds. But task_push_dbscan() and task_push_manual_content_scan() take that callback unconditionally and nothing says it is menu-only. Move the invocation out of the guard; the label stays inside it, since only the HAVE_MENU paths jump to it, and the menu refresh that follows stays too. The two early returns above are unchanged: both happen when there is no handle to read a callback from. Found via samples/tasks/database, which builds without HAVE_MENU, passes a callback, and has therefore never been able to finish a scan. With this it does, which puts the whole scan lifecycle - including the task teardown - under the sanitizers for the first time. Reverting the teardown fix from earlier in this series is now caught: AddressSanitizer: heap-use-after-free in free_manual_content_scan_handle where before it produced a clean run.

task_push_dbscan() ignores its playlist_directory and content_database arguments - both are marked "always from settings" and the scan reads settings->paths instead. The sample set the playlist directory there but not the database one, so the scan ran with an empty database path: [INFO] [Scanner] ""... is that empty path being logged, and with no databases to match against the scan produced nothing. With this and the callback fix in the preceding commit the sample completes: [INFO] [Scanner] "/tmp/rdb"... PASS: scan completed under -fsanitize=address,undefined with leak detection on, reporting nothing. That is the first end-to-end sanitizer coverage of the scanner in this tree, and it reaches the task teardown, which no other harness here does.

The ceiling on index memory was a platform list with a comment claiming the consoles get less "where DIR_MAX_LENGTH is already halved for the same reason". There is no same reason. DIR_MAX_LENGTH is documented as "an upper limit for the length of a path (including the filename)" - a filesystem constraint. It says nothing about how much heap a platform has. What I actually did was copy the platform list out of that #if and write a justification that implied a shared rationale I never established. The numbers either side of it, 24 MB and 2 MB, were picked to look reasonable rather than measured against anything. mem_stats_free() is the right source and was already available: implemented for the 3DS and the GameCube/Wii among others - the platforms this was guessing about - already linked wherever task_database.c is, and already used this way by task_audio_mixer_threshold(), which sizes a task-lifetime allocation as a share of what is free. Take an eighth, deliberately more conservative than the quarter the mixer uses because these indexes are held for the whole scan rather than consulted once. Cap at 32 MB so a large-memory host does not hoard - the biggest database shipped indexes to roughly 360 KB, so that covers far more databases than exist. Below 256 KB do not start at all. Where the platform has no implementation and reports zero, fall back to 2 MB, which is small enough to be harmless given that running out only means using the query path. 3DS, most of 128 MB free 12 MB 3DS, squeezed 1 MB Wii, 40 MB free 5 MB 1.5 MB free none, query path only desktop 32 MB (capped) platform reports nothing 2 MB Behaviour is unchanged when the budget is ample, which is every case the equivalence checks and the end-to-end scan cover; running out was already handled by falling back.

Prompted by the DIR_MAX_LENGTH correlation in the preceding commit being fabricated, I went back over the comments added in this series looking for the same failure - a claim written from intent rather than checked. Three did not survive. "The scanner stops at the first entry that matches, so a bounded number of hits is always enough", above the 32-entry lookup buffer. That is precisely the reasoning that produced the truncation bug fixed earlier in this series: Sega - Mega Drive - Genesis carries a placeholder serial shared by hundreds of records, and comparing the two lookup paths over its serials gave 89 mismatches until the overflow was handed back to the query path instead of cut short. The comment survived the fix and told the next reader the fallback was unnecessary. "Same shape as the serial path above". The serial lookup starts at task_database_iterate_serial_lookup(), below the crc one, not above. "12 bytes per indexed record", and the 360 KB that followed from it. struct db_crc_entry is a uint64 offset and a uint32 key, which pads to 16. Measured against the shipped databases: Nintendo - NES 30359 crc entries 474 KB Sony - PlayStation 12673 crc entries 198 KB 13487 serial 210 KB Sega - Genesis 7398 crc entries 115 KB Sony - PSP 4695 crc entries 73 KB so the largest is 474 KB rather than the 360 KB claimed, and a full set of databases costs about a third more than the figure quoted throughout this series. The budget added in the preceding commit is unaffected - it measures what an index actually holds rather than predicting it - but the numbers used to justify the cap were wrong. Nothing about the code changes here.

An index entry was a uint64 offset beside a uint32 key, which pads to 16 bytes. A 32-bit offset packs the pair to 8 with no padding, so an index costs half what it did: Nintendo - NES 30359 crc entries 474 -> 237 KB Sony - PlayStation 12673 crc entries 198 -> 99 KB 13487 serial 210 -> 105 KB Sega - Genesis 7398 crc entries 115 -> 57 KB Sony - PSP 4695 crc entries 73 -> 36 KB The range is not close to being a constraint - the largest database shipped is 7.3 MB - but both collectors refuse to index a database whose offsets would not fit rather than storing a wrong one, so it degrades to the query path if that ever changes. This also removes a trap. The offset comparator read the leading eight bytes of an entry as a uint64: uint64_t x = *(const uint64_t*)a; which happened to work while the offset was that wide, and would have compared offset and key together the moment it was not - sorting the lookup results wrongly and silently, since the records would all still be correct records. It is now typed against the struct it actually sorts, and the comment claiming both entry types "lead with a uint64 offset" is gone with it. Verified after the change: crc equivalence over 250 keys in each of three databases, serial over 500 in Genesis - which includes the 89 that exceed the lookup buffer and fall back - and the composed decision over four databases, all with no mismatch. Sweep, the pairing test and the end-to-end scan are clean.

playlist_manual_scan_record_t has ten members. playlist_init() mallocs the playlist and then sets nine of them; overwrite_playlist is the one it misses, so it is read out of uninitialised memory: playlist.c:3855: runtime error: load of value 190, which is not a valid value for type '_Bool' playlist_set_scan_overwrite_playlist playlist.c:2034: runtime error: load of value 190, which is not a valid value for type '_Bool' playlist_write_file 190 is 0xBE, the allocator's fill. The second site is the one that matters: the flag is written into the playlist's scan record on disk, and a later manual re-scan of that playlist reads it back to decide whether to overwrite the existing entries or add to them. Whichever it does was decided by whatever was in that byte. Found by the scan sample in samples/tasks/database, which reached the playlist-writing path for the first time once it could complete a scan that matches something. Nothing in this series touches playlist.c; it is reported by UBSan on the way through.

Drives task_push_dbscan() over content built to match, and checks the playlists it produces name the right games under the right databases. This is the only test here that reaches the scan as a whole - the task lifecycle, database iteration, crc lookup, the promotion of a matched database to the front of the list, the playlist write and the teardown. Two defects went undetected because nothing exercised that: the index caches were keyed by a database's position but not moved when a match promoted it, and playlist_init() left scan_record.overwrite_playlist uninitialised. The second content file matches the *second* database deliberately, and sorts first so a lookup happens after the promotion. Neither detail is incidental: a match only in the first database never promotes anything, and a promotion on the last file is never consulted again. With both the promotion fix and the pairing guard removed the test reports FAIL first database's game in its playlist Alpha The Game and with only one of them removed it passes, because either alone is enough to keep the answer right - the guard by falling back to the query path. That is the intended design and it is why the test needs both removed to demonstrate it discriminates. Under the sanitizers it also catches the playlist defect: reverting that initialiser gives playlist.c:3855: runtime error: load of value 190, which is not a valid value for type '_Bool' Databases, content and core info are all built by the test, so it needs nothing but a writable directory. Content is made to match by forcing its crc32 - four appended bytes bring a buffer's checksum to any value - rather than by shipping data. One part of the fixture is worth knowing about: the core info has to name the databases as well as the file extension, because the scanner skips any database no installed core claims. Without that line the scan reports no match for content whose crc is certainly present, which reads exactly like a lookup bug. Reachable as "make check" alongside the index test.

The sweep is the gate this series runs before producing a patch, and it did not touch the scanner at all - only the libretro-db tests below it. The two defects that reached the branch in this series were both in the scan lifecycle, so the gate was blind to exactly the code most likely to break. Runs "make check" in samples/tasks/database under -fsanitize=address,undefined, which covers the index pairing test and the end-to-end scan. Those link most of the frontend and so are slower to build than everything else here; they are also the only coverage of the scan as a whole, including the task teardown and the playlist write. Verified to gate rather than decorate: breaking one assertion in the scan test gives scanner tests 11 checks, 0 failures 6 checks, 1 failures FAIL first database's game in its playlist Alpha The Game SWEEP FAILED where before the sweep stayed clean.

task_database_cue.c is patched in this series - the disc-suffix append was given the real destination size, and the disc-indicator stripping was using one pattern's length for all of them - and none of it had end-to-end coverage. Adds a third database and a cue sheet naming a track beside it. Only the track carries the matching crc, so the scan can only succeed by reading the sheet, resolving the file it names and checksumming that; the sheet's own checksum is not the target and could not match by chance. The playlist entry is then checked to record the sheet rather than the track, which is what a frontend loads. 8 checks.

add_files_from_archive() expands an archive that did not match into its members and rescans them. Nothing exercised it. Adds a fourth database and a one-entry zip whose member carries the matching crc. The archive itself checksums to something else, so a match can only come from expanding it. The zip is written by hand with the stored method - no compressor needed, and no binary fixture in the tree. Two things this does not do, stated because the commit that touched this code is in the same series: The bound corrected there - the copy starts at _len + 1 so the space is sizeof(new_path) - _len - 1, not - _len - is not demonstrated by this. Reverting it to the off-by-one form leaves the test passing with no sanitizer report, which matches what that commit said at the time: the enclosing length test keeps it unreachable. The coverage here is of the expansion path, not of that bound. The entry is also asserted to record the archive rather than the "archive#member" form the scan builds internally, because that is what the playlist actually contains. The first version of this test asserted the member form and failed; the assertion was wrong, not the scanner. 10 checks.

Comments and one script path named specific consoles and their databases as illustrations. None of it was load-bearing: the figures stand without saying which database produced them, and the sweep can take whichever database it finds rather than one by name. database_info.c "at least one shipped database" for the placeholder-serial example task_database.c "the biggest database shipped today" for the index size, and "a db_name" for the playlist comparison database_index_test.c "a wholly unrelated system" for the misfiling example sweep.sh picks the first .rdb in the corpus rather than a named file, which also stops the threaded check being skipped when a corpus happens not to contain that one The system names in task_database_cue.c are untouched: the magic table and the string comparisons around it are how the scanner identifies a disc, so they are functional rather than illustrative, and they predate this series. No behaviour change. Sweep and both scanner suites unchanged.

Every database ships "serial" as a binary field rather than a string: 27056 binary and 0 string in Sony - PlayStation 2, 26957 and 0 in Sony - PlayStation, 1949 and 0 in Nintendo - Nintendo Entertainment System. The readers allocate len + 1 and write the terminator, so such a field is a valid C string, and reading it as one was correct. An earlier commit in this series gated the string fields on RDT_STRING to stop a map or array being strdup'd through the shared union member. That was the right fix for maps and arrays - their items pointer need not contain a zero byte - but it also excluded binary, which left db_info->serial NULL. The scanner's serial match tests that pointer before it compares: if (db_info_entry->serial) if (string_is_equal(db_state->serial, db_info_entry->serial)) so the comparison never ran and no disc title matched. Reported against a folder of 605 ISOs and 48 bin/cue where two entries survived - both of them exact dumps that matched on crc instead. Systems keyed on crc were unaffected throughout, which is why NES, SNES and N64 scanned normally. Accept RDT_BINARY alongside RDT_STRING at both extraction sites. Maps and arrays stay excluded, which is what the original change was for. The regression test now asserts the serial field survives extraction rather than only that the right record is found; with the gate back to RDT_STRING it reports FAIL binary serial survives extraction (NULL) Nothing caught this before because every test built its fixtures with string serials, and the equivalence checks compared the index against the query - both of which read through the same broken extraction and so agreed with each other.

RLZMA_LCLP_MAX was 3, on the stated assumption that "the 7z streams libretro-common reads do not exceed 3 either". They do. 7-Zip emits lc=4, lp=0 routinely - the properties byte reads 0x5E - and rlzma_dec_init() rejected every such stream: if (dec->lc + dec->lp > RLZMA_LCLP_MAX) return RLZMA_ERROR_PARAM; Since 7z stores its own header as an LZMA stream, this failed at r7z_archive_open() rather than on any particular member: the archive could not be opened at all. Six archives from a DS set, all of them LZMA1 with lc=4, all rejected. With the limit at 4 they open and report entry checksums matching an independent reader exactly. The neighbouring constants already say 4 - RLZMA2_LCLP_MAX is 4, the reference encoder's limit is 4 - and the struct comment in this same header documents the probability array as "~29 KiB", which is the size for 4 and not for 3, so the two comments contradicted each other. Costs 6144 more uint16 entries, about 12 KiB per decoder instance, allocated on the heap by its only caller.

file_archive_get_file_crc32_and_size() looped with no way out. The iterate call is guarded on the transfer still being in ITERATE, but nothing breaks once it leaves that state, and the two tests below then re-read a current_file_path that can no longer change: for (;;) { if (state.type == ARCHIVE_TRANSFER_ITERATE) file_archive_parse_file_iterate(...); if (!contains_compressed) break; if (archive_path) { if (string_is_equal(...)) break; } else break; } So a member that is not in the archive spun at full CPU forever. Not a rare shape: the archive failing to open produces an empty enumeration and the same outcome, which is how it was found - a scanner walking a directory of 7z files stopped making progress instead of skipping them. It also returned whatever entry the walk stopped on. The caller cannot tell a leftover checksum from a real one, and the scanner would match content against the wrong record, so a miss now reports zero size and zero crc.

Two defects stopped a content scan rather than failing it, and neither had coverage. archive_zip_test gains a missing-member case. file_archive_get_file_crc32_and_size() looped with no exit once the transfer left ITERATE, so asking for a member the archive does not hold spun at full CPU. The test builds a one-entry zip and asks for a name that is not in it, expecting nothing back. A regression does not fail this so much as hang it - there is no portable way to bound the call from inside - but a stuck test is a visible failure either way, and reverting the fix does hang it. samples/formats/r7z is new and covers the lc/lp limit. RLZMA_LCLP_MAX was 3 while 7-Zip routinely writes lc=4, and since 7z holds its own header as an LZMA stream, such an archive could not be opened at all. Checks the properties byte 7-Zip actually emits (0x5E), that every lc/lp pair inside the declared limit is accepted, and that the probability array is sized for that limit - the constant and the comment describing the array had already drifted apart once, the comment documenting the size for 4 while the constant said 3. Both are verified to discriminate: with RLZMA_LCLP_MAX back at 3 the property test reports FAIL lc=4 lp=0 pb=2, as 7-Zip writes it props 0x5E FAIL limit covers what the format allows in practice and with the loop fix reverted the archive test hangs. The archive_file sample gained a check target and SANITIZER support so the sweep can run it; it had neither. Both now run there.

encoding_crc32.c gained a PCLMULQDQ path that calls cpu_features_get(), so every harness linking it now needs features/features_cpu.c too: encoding_crc32.c:274: undefined reference to `cpu_features_get' The in-tree tests build through their own makefiles and were unaffected; only the corpus harnesses the sweep compiles directly failed, which is why the tests all reported clean while the sweep did not.

LibretroAdmin added 30 commits July 28, 2026 16:28

LibretroAdmin added 26 commits July 28, 2026 16:29

LibretroAdmin force-pushed the audit_db branch 2 times, most recently from fcd0ec3 to ec6ab78 Compare July 28, 2026 14:36

LibretroAdmin merged commit 93ccb92 into libretro:master Jul 28, 2026
48 checks passed

LibretroAdmin deleted the audit_db branch July 28, 2026 19:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Audit and harden the content scanner database stack - #19279

Audit and harden the content scanner database stack#19279
LibretroAdmin merged 60 commits into
libretro:masterfrom
LibretroAdmin:audit_db

LibretroAdmin commented Jul 28, 2026 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Uh oh!

Conversation

LibretroAdmin commented Jul 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What this is

Fixes

Performance

Tests

Three bugs I introduced and fixed here

What has not been done

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

LibretroAdmin commented Jul 28, 2026 •

edited

Loading