diff --git a/.gitignore b/.gitignore index c14c88575c83..75bf7f9a3a54 100644 --- a/.gitignore +++ b/.gitignore @@ -93,7 +93,7 @@ saves screenshots autoconfig config -database +/database overlays playlists states diff --git a/database_info.c b/database_info.c index f70c33362bd3..e742c4118760 100644 --- a/database_info.c +++ b/database_info.c @@ -264,10 +264,47 @@ static int database_cursor_iterate(libretrodb_cursor_t *cur, if (key->type != RDT_STRING) continue; - str = key->val.string.buff; + if (!(str = key->val.string.buff)) + continue; str_len = strlen(str); - val_string = val->val.string.buff; + /* Only read the string member when the value actually holds a + * string. string.buff, binary.buff, map.items and array.items + * all sit at the same offset in the union, so a field stored + * with an unexpected type used to hand the strdup()s below a + * pointer to the wrong kind of data - the raw bytes of a binary + * field, or the first bytes of a map's pair array - which then + * went into the playlist label verbatim. + * + * The scalar types are harmless in practice (uint_/int_ sit at + * offset 0, so buff reads the calloc'd zero and the + * "val_string &&" tests below reject it), and the readers + * NUL-terminate their buffers, so this is a correctness problem + * rather than a memory-safety one - but nothing bounds the walk + * over a map's pair array, which is not guaranteed to contain a + * zero byte. + * + * The md5 and sha1 fields already gate on val->type; the string + * fields simply never did. */ + /* Binary counts as well as string. The readers allocate + * len + 1 and write the terminator, so a binary field holding + * text is a valid C string - and "serial" is stored that way in + * every database shipped: 27056 binary and 0 string in + * Sony - PlayStation 2, 26957 and 0 in Sony - PlayStation, + * 1949 and 0 in Nintendo - Nintendo Entertainment System. + * + * Rejecting it, as this did briefly, left db_info->serial NULL, + * and the scanner's serial match tests that pointer before it + * compares - so every disc system stopped matching while the + * crc-based ones carried on working. + * + * What actually needed excluding is a map or an array: their + * items pointer sits at the same offset in the union and the + * strdup() walks it looking for a zero byte that need not be + * there. */ + val_string = (val->type == RDT_STRING || val->type == RDT_BINARY) + ? val->val.string.buff + : NULL; switch (str_len) { @@ -545,40 +582,37 @@ static int database_cursor_iterate(libretrodb_cursor_t *cur, * * @fields: Bitmask of DB_EXTRACT_* flags. 0 = extract all. */ -static int database_cursor_iterate_filtered(libretrodb_cursor_t *cur, +/* Extract @fields from an already-read record. Split out of + * database_cursor_iterate_filtered() below so the crc-index path, + * which reads records by offset rather than through a cursor, fills + * database_info_t through exactly the same code. + * + * Returns 0 when the record was a map and was extracted, 1 otherwise. + * Does not free @item; the caller owns it. */ +static int database_info_fill_from_dom(struct rmsgpack_dom_value *item, database_info_t *db_info, unsigned fields) { size_t i; - struct rmsgpack_dom_value item; - - /* Fall back to full extraction if no mask specified */ - if (fields == 0) - return database_cursor_iterate(cur, db_info); - - if (libretrodb_cursor_read_item(cur, &item) != 0) - return -1; - if (item.type != RDT_MAP) - { - rmsgpack_dom_value_free(&item); + if (item->type != RDT_MAP) return 1; - } db_info->analog_supported = -1; db_info->rumble_supported = -1; db_info->coop_supported = -1; - for (i = 0; i < item.val.map.len; i++) + for (i = 0; i < item->val.map.len; i++) { - struct rmsgpack_dom_value *key = &item.val.map.items[i].key; - struct rmsgpack_dom_value *val = &item.val.map.items[i].value; + struct rmsgpack_dom_value *key = &item->val.map.items[i].key; + struct rmsgpack_dom_value *val = &item->val.map.items[i].value; const char *str; size_t str_len; if (!key || !val || key->type != RDT_STRING) continue; - str = key->val.string.buff; + if (!(str = key->val.string.buff)) + continue; str_len = key->val.string.len; switch (str_len) @@ -608,12 +642,25 @@ static int database_cursor_iterate_filtered(libretrodb_cursor_t *cur, case 4: if ((fields & DB_EXTRACT_NAME) && memcmp(str, "name", 4) == 0) { - const char *vs = val->val.string.buff; - if (vs && *vs) - db_info->name = strdup(vs); + /* Gate on the stored type: string.buff, binary.buff and + * map.items share an offset in the union, so a + * mistyped field otherwise strdup()s the wrong kind of + * data straight into the playlist label. Matches the + * md5/sha1 handling above. */ + if (val->type == RDT_STRING) + { + const char *vs = val->val.string.buff; + if (vs && *vs) + db_info->name = strdup(vs); + } } else if ((fields & DB_EXTRACT_SIZE) && memcmp(str, "size", 4) == 0) - db_info->size = (uint64_t)val->val.uint_; + { + if (val->type == RDT_UINT) + db_info->size = val->val.uint_; + else if (val->type == RDT_INT && val->val.int_ >= 0) + db_info->size = (uint64_t)val->val.int_; + } else if ((fields & DB_EXTRACT_SHA1) && memcmp(str, "sha1", 4) == 0) { if (val->type == RDT_BINARY) @@ -625,9 +672,15 @@ static int database_cursor_iterate_filtered(libretrodb_cursor_t *cur, case 6: if ((fields & DB_EXTRACT_SERIAL) && memcmp(str, "serial", 6) == 0) { - const char *vs = val->val.string.buff; - if (vs && *vs) - db_info->serial = strdup(vs); + /* Stored as binary in every shipped database; see the + * note on val_string in database_info_fill_from_dom's + * sibling above. */ + if (val->type == RDT_STRING || val->type == RDT_BINARY) + { + const char *vs = val->val.string.buff; + if (vs && *vs) + db_info->serial = strdup(vs); + } } break; @@ -636,10 +689,27 @@ static int database_cursor_iterate_filtered(libretrodb_cursor_t *cur, } } - rmsgpack_dom_value_free(&item); return 0; } +static int database_cursor_iterate_filtered(libretrodb_cursor_t *cur, + database_info_t *db_info, unsigned fields) +{ + int rv; + struct rmsgpack_dom_value item; + + /* Fall back to full extraction if no mask specified */ + if (fields == 0) + return database_cursor_iterate(cur, db_info); + + if (libretrodb_cursor_read_item(cur, &item) != 0) + return -1; + + rv = database_info_fill_from_dom(&item, db_info, fields); + rmsgpack_dom_value_free(&item); + return rv; +} + static int database_cursor_open(libretrodb_t *db, libretrodb_cursor_t *cur, const char *path, const char *query) { @@ -673,12 +743,22 @@ static bool type_is_prioritized(const char *path) const char *ext = path_get_extension(path); if (ext) { - char e0 = ext[0] | 0x20; - char e1 = ext[1] | 0x20; - char e2 = ext[2] | 0x20; - if (ext[3] == '\0') - return (e0 == 'c' && e1 == 'u' && e2 == 'e') - || (e0 == 'g' && e1 == 'd' && e2 == 'i'); + char e0, e1, e2; + + /* ext[1] and ext[2] were read before anything established the + * string was that long, and path_get_extension() returns a + * pointer to the terminator for a path with no extension - a + * three byte read past the end of the string, on every element + * of the qsort() comparator below. */ + if (!ext[0] || !ext[1] || !ext[2] || ext[3] != '\0') + return false; + + e0 = ext[0] | 0x20; + e1 = ext[1] | 0x20; + e2 = ext[2] | 0x20; + + return (e0 == 'c' && e1 == 'u' && e2 == 'e') + || (e0 == 'g' && e1 == 'd' && e2 == 'i'); } return false; } @@ -882,6 +962,663 @@ static database_info_list_t *database_info_list_new_names_only( return list; } + +/* ------------------------------------------------------------------ + * CRC index + * + * The scanner asks each database the same question once per content + * file - does it hold this crc - and every one of those is a full + * cursor walk. Walking once up front and keeping crc -> record + * offset turns the repeats into a binary search. + * + * Entries are sorted by crc so a lookup finds the run of records + * sharing one. Matches are then re-sorted by offset before the list + * is built, because the query path returns records in file order and + * the scanner takes the first entry that matches; reproducing that + * order is what makes the two paths interchangeable. + * ------------------------------------------------------------------ */ + +/* uint32 offset rather than uint64: paired with the key this packs to + * 8 bytes where a uint64 would pad the pair to 16, halving what an + * index costs. The largest database shipped today is 7.3 MB, so the + * range is not close to being a constraint, and db_crc_collect() + * refuses to index anything that would not fit. */ +struct db_crc_entry +{ + uint32_t offset; + uint32_t crc; +}; + +struct database_info_crc_index +{ + struct db_crc_entry *entries; + size_t count; + char *rdb_path; + /* Size range of the indexed records, collected during the same + * walk. The scanner otherwise pays two more walks per database + * for exactly this. */ + uint64_t size_min; + uint64_t size_max; + bool have_size; +}; + +static int db_crc_by_crc(const void *a, const void *b) +{ + uint32_t x = ((const struct db_crc_entry*)a)->crc; + uint32_t y = ((const struct db_crc_entry*)b)->crc; + if (x < y) return -1; + if (x > y) return 1; + return 0; +} + +static int db_crc_by_offset(const void *a, const void *b) +{ + uint64_t x = ((const struct db_crc_entry*)a)->offset; + uint64_t y = ((const struct db_crc_entry*)b)->offset; + if (x < y) return -1; + if (x > y) return 1; + return 0; +} + +struct db_crc_build +{ + struct db_crc_entry *entries; + size_t count; + size_t capacity; + size_t max_bytes; + uint64_t size_min; + uint64_t size_max; + bool have_size; + bool failed; +}; + +static int db_crc_collect(void *ctx, const uint8_t *key, size_t key_len, + uint64_t offset, const uint64_t *aux) +{ + struct db_crc_build *b = (struct db_crc_build*)ctx; + + if (aux) + { + if (!b->have_size) + { + b->size_min = *aux; + b->size_max = *aux; + b->have_size = true; + } + else + { + if (*aux < b->size_min) b->size_min = *aux; + if (*aux > b->size_max) b->size_max = *aux; + } + } + + /* Only fixed 4-byte crcs are indexable. Anything else is left to + * the query path, which is still what runs without an index. */ + if (key_len != 4) + return 0; + + /* Offsets are held in 32 bits; a database large enough to exceed + * that is handed to the query path rather than indexed wrongly. */ + if (offset > (uint64_t)0xFFFFFFFFu) + { + b->failed = true; + return 1; + } + + if (b->count == b->capacity) + { + size_t cap = b->capacity ? b->capacity * 2 : 1024; + struct db_crc_entry *tmp; + + /* Give up rather than exceed the caller's allowance. A partial + * index would be worse than none: it would miss matches without + * saying so. */ + if (b->max_bytes && cap * sizeof(*b->entries) > b->max_bytes) + { + b->failed = true; + return 1; + } + + if (!(tmp = (struct db_crc_entry*) + realloc(b->entries, cap * sizeof(*tmp)))) + { + b->failed = true; + return 1; /* stop the scan */ + } + b->entries = tmp; + b->capacity = cap; + } + + b->entries[b->count].crc = ((uint32_t)key[0] << 24) + | ((uint32_t)key[1] << 16) + | ((uint32_t)key[2] << 8) + | (uint32_t)key[3]; + b->entries[b->count].offset = (uint32_t)offset; + b->count++; + return 0; +} + +database_info_crc_index_t *database_info_crc_index_new(const char *rdb_path, + size_t max_bytes) +{ + struct db_crc_build build; + database_info_crc_index_t *idx = NULL; + libretrodb_t *db = NULL; + + if (!rdb_path || !*rdb_path) + return NULL; + + memset(&build, 0, sizeof(build)); + build.max_bytes = max_bytes; + + if (!(db = libretrodb_new())) + return NULL; + + if (libretrodb_open(rdb_path, db, false) != 0) + { + libretrodb_free(db); + return NULL; + } + + if ( libretrodb_scan_field(db, "crc", "size", db_crc_collect, + &build) != 0 + || build.failed) + goto error; + + if (!(idx = (database_info_crc_index_t*)calloc(1, sizeof(*idx)))) + goto error; + + if (!(idx->rdb_path = strdup(rdb_path))) + { + free(idx); + idx = NULL; + goto error; + } + + if (build.count > 1) + qsort(build.entries, build.count, sizeof(*build.entries), + db_crc_by_crc); + + idx->entries = build.entries; + idx->count = build.count; + idx->size_min = build.size_min; + idx->size_max = build.size_max; + idx->have_size = build.have_size; + + libretrodb_close(db); + libretrodb_free(db); + return idx; + +error: + free(build.entries); + libretrodb_close(db); + libretrodb_free(db); + return NULL; +} + +bool database_info_crc_index_size_range( + const database_info_crc_index_t *idx, int64_t *min, int64_t *max) +{ + if (!idx || !idx->have_size || !min || !max) + return false; + *min = (int64_t)idx->size_min; + *max = (int64_t)idx->size_max; + return true; +} + +size_t database_info_crc_index_bytes(const database_info_crc_index_t *idx) +{ + if (!idx) + return 0; + return idx->count * sizeof(*idx->entries) + sizeof(*idx); +} + +void database_info_crc_index_free(database_info_crc_index_t *idx) +{ + if (!idx) + return; + free(idx->entries); + free(idx->rdb_path); + free(idx); +} + +size_t database_info_crc_index_count(const database_info_crc_index_t *idx) +{ + return idx ? idx->count : 0; +} + +/* Append every entry whose crc is @crc, returning the new count, or + * (size_t)-1 if the run does not fit. Truncating instead would hand + * the caller a shorter list than the query path returns for the same + * crc, which is a wrong answer rather than a slow one - databases do + * carry placeholder keys shared by hundreds of records. */ +static size_t db_crc_gather(const database_info_crc_index_t *idx, + uint32_t crc, struct db_crc_entry *out, size_t have, size_t cap) +{ + size_t lo = 0; + size_t hi = idx->count; + + while (lo < hi) + { + size_t mid = lo + ((hi - lo) / 2); + if (idx->entries[mid].crc < crc) + lo = mid + 1; + else + hi = mid; + } + + while (lo < idx->count && idx->entries[lo].crc == crc) + { + if (have >= cap) + return (size_t)-1; + out[have++] = idx->entries[lo++]; + } + + return have; +} + +database_info_list_t *database_info_list_new_crc( + const database_info_crc_index_t *idx, const char *rdb_path, + uint32_t crc, uint32_t archive_crc, unsigned fields) +{ + /* Working set for one lookup. A run longer than this is handed + * back to the query path rather than truncated - see + * db_crc_gather(). It is not rare: at least one shipped database + * carries a placeholder serial shared by hundreds of records, and + * assuming a bounded run was always enough is what silently + * shortened those results before. */ + struct db_crc_entry hits[32]; + database_info_list_t *list = NULL; + database_info_t *items = NULL; + libretrodb_t *db = NULL; + size_t found = 0; + size_t i, kept = 0; + + if (!idx) + return NULL; + + /* The caller keys its index cache by database position, and that + * position moves when a matched database is promoted. Refuse an + * index that belongs to a different database rather than answering + * with another system's records: the query path then runs, which + * is slow but right. */ + if (rdb_path && idx->rdb_path && strcmp(rdb_path, idx->rdb_path)) + return NULL; + + found = db_crc_gather(idx, crc, hits, found, + sizeof(hits) / sizeof(hits[0])); + if (found != (size_t)-1 && archive_crc && archive_crc != crc) + found = db_crc_gather(idx, archive_crc, hits, found, + sizeof(hits) / sizeof(hits[0])); + + /* Too many records share this crc to answer from the index; let + * the query path, which has no such bound, handle it. */ + if (found == (size_t)-1) + return NULL; + + if (!(list = (database_info_list_t*)calloc(1, sizeof(*list)))) + return NULL; + + if (found == 0) + return list; /* empty result, as a miss returns */ + + /* File order, so the scanner sees what the query path showed it. */ + if (found > 1) + qsort(hits, found, sizeof(hits[0]), db_crc_by_offset); + + if (!(items = (database_info_t*)calloc(found, sizeof(*items)))) + { + free(list); + return NULL; + } + + if (!(db = libretrodb_new()) + || libretrodb_open(idx->rdb_path, db, false) != 0) + { + free(items); + free(list); + libretrodb_free(db); + return NULL; + } + + for (i = 0; i < found; i++) + { + struct rmsgpack_dom_value item; + if (libretrodb_read_at(db, hits[i].offset, &item) != 0) + continue; + if (database_info_fill_from_dom(&item, &items[kept], fields) == 0) + kept++; + rmsgpack_dom_value_free(&item); + } + + libretrodb_close(db); + libretrodb_free(db); + + list->list = items; + list->count = kept; + return list; +} + + +/* ------------------------------------------------------------------ + * Serial index + * + * Disc content is matched on serial rather than crc, and that lookup + * walks the database exactly like the crc one did. Same treatment, + * with one difference: a serial is variable-length, so the index + * keeps a hash rather than the bytes and the candidate records are + * read back and compared exactly. A collision therefore costs an + * extra record read, never a wrong answer. + * ------------------------------------------------------------------ */ + +/* See struct db_crc_entry for why the offset is 32-bit. */ +struct db_serial_entry +{ + uint32_t offset; + uint32_t hash; +}; + +struct database_info_serial_index +{ + struct db_serial_entry *entries; + size_t count; + char *rdb_path; +}; + +static uint32_t db_serial_hash(const uint8_t *key, size_t len) +{ + uint32_t h = 5381; + size_t i; + for (i = 0; i < len; i++) + h = ((h << 5) + h) + key[i]; + return h; +} + +/* Typed rather than punning the leading field: the previous version + * read the first eight bytes as a uint64, which only worked while the + * offset happened to be that wide and would silently have compared + * offset and key together once it was not. */ +static int db_serial_by_offset(const void *a, const void *b) +{ + uint32_t x = ((const struct db_serial_entry*)a)->offset; + uint32_t y = ((const struct db_serial_entry*)b)->offset; + if (x < y) return -1; + if (x > y) return 1; + return 0; +} + +static int db_serial_by_hash(const void *a, const void *b) +{ + uint32_t x = ((const struct db_serial_entry*)a)->hash; + uint32_t y = ((const struct db_serial_entry*)b)->hash; + if (x < y) return -1; + if (x > y) return 1; + return 0; +} + +struct db_serial_build +{ + struct db_serial_entry *entries; + size_t count; + size_t capacity; + size_t max_bytes; + bool failed; +}; + +static int db_serial_collect(void *ctx, const uint8_t *key, size_t key_len, + uint64_t offset, const uint64_t *aux) +{ + struct db_serial_build *b = (struct db_serial_build*)ctx; + (void)aux; + + if (!key_len) + return 0; + + /* A record can carry the key more than once; one entry per record + * is enough because the lookup reads the record back anyway. */ + if (b->count && b->entries[b->count - 1].offset == (uint32_t)offset) + return 0; + + /* See db_crc_collect(). */ + if (offset > (uint64_t)0xFFFFFFFFu) + { + b->failed = true; + return 1; + } + + if (b->count == b->capacity) + { + size_t cap = b->capacity ? b->capacity * 2 : 1024; + struct db_serial_entry *tmp; + + /* See db_crc_collect(). */ + if (b->max_bytes && cap * sizeof(*b->entries) > b->max_bytes) + { + b->failed = true; + return 1; + } + + if (!(tmp = (struct db_serial_entry*) + realloc(b->entries, cap * sizeof(*tmp)))) + { + b->failed = true; + return 1; + } + b->entries = tmp; + b->capacity = cap; + } + + b->entries[b->count].hash = db_serial_hash(key, key_len); + b->entries[b->count].offset = (uint32_t)offset; + b->count++; + return 0; +} + +database_info_serial_index_t *database_info_serial_index_new( + const char *rdb_path, size_t max_bytes) +{ + struct db_serial_build build; + database_info_serial_index_t *idx = NULL; + libretrodb_t *db = NULL; + + if (!rdb_path || !*rdb_path) + return NULL; + + memset(&build, 0, sizeof(build)); + build.max_bytes = max_bytes; + + if (!(db = libretrodb_new())) + return NULL; + + if (libretrodb_open(rdb_path, db, false) != 0) + { + libretrodb_free(db); + return NULL; + } + + if ( libretrodb_scan_field(db, "serial", NULL, db_serial_collect, + &build) != 0 + || build.failed) + goto error; + + if (!(idx = (database_info_serial_index_t*)calloc(1, sizeof(*idx)))) + goto error; + + if (!(idx->rdb_path = strdup(rdb_path))) + { + free(idx); + idx = NULL; + goto error; + } + + if (build.count > 1) + qsort(build.entries, build.count, sizeof(*build.entries), + db_serial_by_hash); + + idx->entries = build.entries; + idx->count = build.count; + + libretrodb_close(db); + libretrodb_free(db); + return idx; + +error: + free(build.entries); + libretrodb_close(db); + libretrodb_free(db); + return NULL; +} + +size_t database_info_serial_index_bytes( + const database_info_serial_index_t *idx) +{ + if (!idx) + return 0; + return idx->count * sizeof(*idx->entries) + sizeof(*idx); +} + +void database_info_serial_index_free(database_info_serial_index_t *idx) +{ + if (!idx) + return; + free(idx->entries); + free(idx->rdb_path); + free(idx); +} + +size_t database_info_serial_index_count( + const database_info_serial_index_t *idx) +{ + return idx ? idx->count : 0; +} + +/* True when @item carries @serial in a "serial" field. */ +static bool db_record_has_serial(struct rmsgpack_dom_value *item, + const char *serial, size_t serial_len) +{ + size_t i; + + if (item->type != RDT_MAP) + return false; + + for (i = 0; i < item->val.map.len; i++) + { + struct rmsgpack_dom_value *k = &item->val.map.items[i].key; + struct rmsgpack_dom_value *v = &item->val.map.items[i].value; + + if ( !k || !v + || k->type != RDT_STRING || !k->val.string.buff + || strcmp(k->val.string.buff, "serial")) + continue; + + if ( v->type == RDT_BINARY + && v->val.binary.len == serial_len + && v->val.binary.buff + && memcmp(v->val.binary.buff, serial, serial_len) == 0) + return true; + + if ( v->type == RDT_STRING + && v->val.string.buff + && strlen(v->val.string.buff) == serial_len + && memcmp(v->val.string.buff, serial, serial_len) == 0) + return true; + } + + return false; +} + +database_info_list_t *database_info_list_new_serial( + const database_info_serial_index_t *idx, const char *rdb_path, + const char *serial, unsigned fields) +{ + /* See database_info_list_new_crc(): a run too long to hold goes to + * the query path rather than being cut short. */ + struct db_serial_entry hits[32]; + database_info_list_t *list = NULL; + database_info_t *items = NULL; + libretrodb_t *db = NULL; + size_t serial_len; + uint32_t want; + size_t lo, hi, found = 0, i, kept = 0; + + if (!idx || !serial || !*serial) + return NULL; + + /* See database_info_list_new_crc(). */ + if (rdb_path && idx->rdb_path && strcmp(rdb_path, idx->rdb_path)) + return NULL; + + serial_len = strlen(serial); + want = db_serial_hash((const uint8_t*)serial, serial_len); + + lo = 0; + hi = idx->count; + while (lo < hi) + { + size_t mid = lo + ((hi - lo) / 2); + if (idx->entries[mid].hash < want) + lo = mid + 1; + else + hi = mid; + } + + while (lo < idx->count && idx->entries[lo].hash == want) + { + /* Same rule as the crc index: a run too long to hold is handed + * back to the query path rather than truncated. Placeholder + * serials shared by hundreds of records do occur. */ + if (found >= sizeof(hits) / sizeof(hits[0])) + return NULL; + hits[found++] = idx->entries[lo++]; + } + + if (!(list = (database_info_list_t*)calloc(1, sizeof(*list)))) + return NULL; + + if (found == 0) + return list; + + if (found > 1) + qsort(hits, found, sizeof(hits[0]), db_serial_by_offset); + + if (!(items = (database_info_t*)calloc(found, sizeof(*items)))) + { + free(list); + return NULL; + } + + if (!(db = libretrodb_new()) + || libretrodb_open(idx->rdb_path, db, false) != 0) + { + free(items); + free(list); + libretrodb_free(db); + return NULL; + } + + for (i = 0; i < found; i++) + { + struct rmsgpack_dom_value item; + + if (libretrodb_read_at(db, hits[i].offset, &item) != 0) + continue; + + /* The index matched a hash; this is where it is made exact. */ + if ( db_record_has_serial(&item, serial, serial_len) + && database_info_fill_from_dom(&item, &items[kept], fields) == 0) + kept++; + + rmsgpack_dom_value_free(&item); + } + + libretrodb_close(db); + libretrodb_free(db); + + list->list = items; + list->count = kept; + return list; +} + database_info_list_t *database_info_list_new( const char *rdb_path, const char *query) { diff --git a/database_info.h b/database_info.h index dd53cc97a806..7c449a097556 100644 --- a/database_info.h +++ b/database_info.h @@ -161,6 +161,61 @@ database_info_list_t *menu_dbinfo_cache_get(const char *path, const char *query); bool menu_dbinfo_cache_has(const char *path, const char *query); +/* A crc -> record-offset index over one database, built in a single + * pass. The scanner otherwise walks a whole database per content + * file; with an index it walks once and binary-searches thereafter. + * Costs 8 bytes per indexed record - a 32-bit offset beside the key - + * and lives for as long as the caller keeps it. */ +typedef struct database_info_crc_index database_info_crc_index_t; + +/* @max_bytes caps the entry table; the build gives up and returns + * NULL rather than exceed it, because a partial index would miss + * matches without saying so. Zero means no limit. */ +database_info_crc_index_t *database_info_crc_index_new(const char *rdb_path, + size_t max_bytes); + +/* Bytes the index actually holds, for a caller tracking a budget + * across several databases. */ +size_t database_info_crc_index_bytes(const database_info_crc_index_t *idx); +void database_info_crc_index_free(database_info_crc_index_t *idx); +size_t database_info_crc_index_count(const database_info_crc_index_t *idx); + +/* Size range of the records the index covers, collected during the + * same walk that built it. False when no record carried a size. */ +bool database_info_crc_index_size_range( + const database_info_crc_index_t *idx, int64_t *min, int64_t *max); + +/* Records whose crc is @crc or @archive_crc, in file order, with + * @fields extracted - the same list database_info_list_new_filtered() + * returns for "{crc:or(...)}". NULL if the lookup could not be + * served, so the caller falls back to the query path. */ +/* @rdb_path is the database the caller believes @idx describes; the + * lookup refuses if it does not match, so an index paired with the + * wrong database degrades to the query path instead of answering with + * another system's records. Pass NULL to skip the check. */ +database_info_list_t *database_info_list_new_crc( + const database_info_crc_index_t *idx, const char *rdb_path, + uint32_t crc, uint32_t archive_crc, unsigned fields); + +/* The same treatment for the serial lookup disc content uses. A + * serial is variable-length, so the index keeps a hash and the + * candidate records are read back and compared exactly: a collision + * costs an extra read, never a wrong match. */ +typedef struct database_info_serial_index database_info_serial_index_t; + +database_info_serial_index_t *database_info_serial_index_new( + const char *rdb_path, size_t max_bytes); + +size_t database_info_serial_index_bytes( + const database_info_serial_index_t *idx); +void database_info_serial_index_free(database_info_serial_index_t *idx); +size_t database_info_serial_index_count( + const database_info_serial_index_t *idx); + +database_info_list_t *database_info_list_new_serial( + const database_info_serial_index_t *idx, const char *rdb_path, + const char *serial, unsigned fields); + database_info_list_t *database_info_list_new_filtered(const char *rdb_path, const char *query, unsigned fields); diff --git a/libretro-common/file/archive_file.c b/libretro-common/file/archive_file.c index 6b08c5b8abae..2797c7db812b 100644 --- a/libretro-common/file/archive_file.c +++ b/libretro-common/file/archive_file.c @@ -782,6 +782,7 @@ uint32_t file_archive_get_file_crc32_and_size(const char *path, uint64_t *size) file_archive_transfer_t state; struct archive_extract_userdata userdata = {0}; bool returnerr = false; + bool found = true; const char *archive_path = NULL; bool contains_compressed = path_contains_compressed_file(path); @@ -808,11 +809,22 @@ uint32_t file_archive_get_file_crc32_and_size(const char *path, uint64_t *size) for (;;) { + /* Nothing left to look at. Without this the loop spins: the + * iterate call is skipped once the transfer leaves ITERATE, and + * the two tests below then read a current_file_path that can no + * longer change. A member that is not in the archive - or an + * archive that failed to open at all - hung here rather than + * returning. */ + if (state.type != ARCHIVE_TRANSFER_ITERATE) + { + found = false; + break; + } + /* Now find the first file in the archive. */ - if (state.type == ARCHIVE_TRANSFER_ITERATE) - file_archive_parse_file_iterate(&state, - &returnerr, path, NULL, NULL, - &userdata); + file_archive_parse_file_iterate(&state, + &returnerr, path, NULL, NULL, + &userdata); /* If no path specified within archive, stop after * finding the first file. @@ -831,6 +843,16 @@ uint32_t file_archive_get_file_crc32_and_size(const char *path, uint64_t *size) } file_archive_parse_file_iterate_stop(&state); + + /* Report nothing rather than whichever entry the walk stopped on: + * the caller cannot tell a real checksum from a leftover one, and + * the scanner would match content against the wrong record. */ + if (!found) + { + *size = 0; + return 0; + } + *size = userdata.size; return userdata.crc; } diff --git a/libretro-common/include/7z/r7z_lzma.h b/libretro-common/include/7z/r7z_lzma.h index 59a9ee3cd828..191196aa5371 100644 --- a/libretro-common/include/7z/r7z_lzma.h +++ b/libretro-common/include/7z/r7z_lzma.h @@ -62,7 +62,7 @@ RETRO_BEGIN_DECLS * what the probability array is sized for. Streams asking for more are * rejected at init rather than silently mis-decoded. Raising this costs * 0x300 * 2 bytes of struct per increment. */ -#define RLZMA_LCLP_MAX 3 +#define RLZMA_LCLP_MAX 4 /* Base tables (1984, including the layout bias the decoder relies on) * plus the literal coder, sized for the largest lc+lp we accept. */ #define RLZMA_NUM_PROBS (1984 + ((uint32_t)0x300 << RLZMA_LCLP_MAX)) diff --git a/libretro-common/include/streams/interface_stream.h b/libretro-common/include/streams/interface_stream.h index 5eef13baa9da..a3abdd29f13f 100644 --- a/libretro-common/include/streams/interface_stream.h +++ b/libretro-common/include/streams/interface_stream.h @@ -37,7 +37,10 @@ enum intfstream_type INTFSTREAM_FILE = 0, INTFSTREAM_MEMORY, INTFSTREAM_CHD, - INTFSTREAM_RZIP + INTFSTREAM_RZIP, + /* Read-only file access through a sliding buffer - see + * intfstream_open_buffered(). */ + INTFSTREAM_BUFFERED }; typedef struct intfstream_internal intfstream_internal_t, intfstream_t; @@ -118,6 +121,23 @@ bool intfstream_get_crc(intfstream_internal_t *intf, uint32_t *crc); intfstream_t *intfstream_open_file(const char *path, unsigned mode, unsigned hints); +/* Open @path read-only behind a sliding window of @window bytes. + * + * Reads that fall inside the window are a memcpy; the window is + * refilled by seek+read when one does not. For a consumer issuing + * very many small reads - a MsgPack walk over an .rdb spends its time + * there - this removes the per-read trip through + * filestream/VFS/fread while keeping the resident cost fixed at + * @window rather than the size of the file. + * + * A backwards seek inside the window is free; one outside it costs a + * refill, so @window should comfortably exceed the caller's largest + * backwards jump. + * + * Returns NULL if the file cannot be opened or the window cannot be + * allocated; callers should fall back to intfstream_open_file(). */ +intfstream_t *intfstream_open_buffered(const char *path, uint64_t window); + intfstream_t *intfstream_open_memory(void *data, unsigned mode, unsigned hints, uint64_t size); diff --git a/libretro-common/samples/file/archive_file/Makefile b/libretro-common/samples/file/archive_file/Makefile index 70b046a97159..7c7d14bc0fc8 100644 --- a/libretro-common/samples/file/archive_file/Makefile +++ b/libretro-common/samples/file/archive_file/Makefile @@ -34,6 +34,11 @@ ifneq ($(SANITIZER),) LDFLAGS := -fsanitize=$(SANITIZER) $(LDFLAGS) endif +ifneq ($(SANITIZER),) + CFLAGS := -fsanitize=$(SANITIZER) $(CFLAGS) + LDFLAGS := -fsanitize=$(SANITIZER) $(LDFLAGS) +endif + all: $(TARGET) %.o: %.c Makefile @@ -42,7 +47,10 @@ all: $(TARGET) $(TARGET): $(OBJS) $(CC) -o $@ $^ $(LDFLAGS) +check: $(TARGET) + ./$(TARGET) + clean: rm -f $(TARGET) $(OBJS) -.PHONY: clean +.PHONY: all check clean diff --git a/libretro-common/samples/file/archive_file/archive_zip_test.c b/libretro-common/samples/file/archive_file/archive_zip_test.c index 32cec561661e..ec768cb2c17f 100644 --- a/libretro-common/samples/file/archive_file/archive_zip_test.c +++ b/libretro-common/samples/file/archive_file/archive_zip_test.c @@ -607,6 +607,49 @@ static void test_init_failure_cleanup(void) printf("[SUCCESS] archive init failure cleaned up immediately\n"); } +/* file_archive_get_file_crc32_and_size() used to loop with no way out. + * 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 asking + * for a member the archive does not hold spun at full CPU forever. + * + * An archive that fails to open produces an empty walk and lands in + * exactly the same place, which is how it was found: a directory of + * 7z files stopped a scan dead rather than being skipped. + * + * A regression here does not fail this test so much as hang it. That + * is deliberate - there is no portable way to bound the call from + * inside - and a stuck test is a visible failure either way. */ +static void test_missing_member_terminates(void) +{ + const char *tmp_path = "rarch_zip_regression_test.zip"; + uint8_t buf[512]; + size_t len = 0; + char member[256]; + uint64_t size = 12345; + uint32_t crc; + + /* One stored entry named "present.bin", built the same way as the + * cases above. */ + len += write_minimal_lfh(buf + len, "present.bin"); + len += write_eocd(buf + len, 1, 0, (uint32_t)len); + + write_file(tmp_path, buf, len); + + snprintf(member, sizeof(member), "%s#absent.bin", tmp_path); + crc = file_archive_get_file_crc32_and_size(member, &size); + remove(tmp_path); + + if (crc != 0 || size != 0) + { + printf("FAIL missing member reported crc=%08X size=%llu, " + "expected nothing\n", crc, (unsigned long long)size); + failures++; + } + else + printf("ok missing member returns nothing rather than hanging\n"); +} + int main(void) { test_truncated_entry(); @@ -616,6 +659,7 @@ int main(void) test_directory_size_overflow(); test_appledouble_exact_member_selection(); test_init_failure_cleanup(); + test_missing_member_terminates(); if (failures) { diff --git a/libretro-common/samples/formats/r7z/Makefile b/libretro-common/samples/formats/r7z/Makefile new file mode 100644 index 000000000000..17ff325b4b39 --- /dev/null +++ b/libretro-common/samples/formats/r7z/Makefile @@ -0,0 +1,32 @@ +TARGET := r7z_lzma_props_test + +LIBRETRO_COMM_DIR := ../../.. + +SOURCES := \ + r7z_lzma_props_test.c \ + $(LIBRETRO_COMM_DIR)/formats/7z/r7z_lzma.c + +OBJS := $(SOURCES:.c=.o) + +CFLAGS += -Wall -pedantic -std=gnu99 -g -I$(LIBRETRO_COMM_DIR)/include + +ifneq ($(SANITIZER),) + CFLAGS := -fsanitize=$(SANITIZER) $(CFLAGS) + LDFLAGS := -fsanitize=$(SANITIZER) $(LDFLAGS) +endif + +all: $(TARGET) + +%.o: %.c + $(CC) $(CFLAGS) -c $< -o $@ + +$(TARGET): $(OBJS) + $(CC) $(OBJS) $(LDFLAGS) -o $@ + +check: $(TARGET) + ./$(TARGET) + +clean: + rm -f $(TARGET) $(OBJS) + +.PHONY: all check clean diff --git a/libretro-common/samples/formats/r7z/r7z_lzma_props_test.c b/libretro-common/samples/formats/r7z/r7z_lzma_props_test.c new file mode 100644 index 000000000000..8647d336c544 --- /dev/null +++ b/libretro-common/samples/formats/r7z/r7z_lzma_props_test.c @@ -0,0 +1,120 @@ +/* Regression test for the lc/lp limit the LZMA decoder accepts. + * + * RLZMA_LCLP_MAX was 3, on the stated assumption that the 7z streams + * libretro-common reads would not exceed that. They do: 7-Zip emits + * lc=4, lp=0 routinely, which is a properties byte of 0x5E, and + * rlzma_dec_init() rejected every such stream. + * + * 7z keeps its own header as an LZMA stream, so the rejection landed + * at r7z_archive_open() rather than on any one member: an archive + * whose header was written that way could not be opened at all, and + * the file list came back empty. A scan walking a directory of those + * then stalled, because the caller looking for a member it would + * never see had no way out of its loop. + * + * Two things are checked: + * + * - the properties byte 7-Zip actually produces is accepted; + * - every lc/lp pair within the declared limit is accepted, and the + * probability array is sized for it, so the constant and the + * array cannot drift apart again. + * + * Build: make -C libretro-common/samples/formats/r7z check + */ + +#include +#include + +#include <7z/r7z_lzma.h> + +static int failures = 0; +static int checks = 0; + +static void check(int ok, const char *what, const char *detail) +{ + checks++; + printf(" %-5s %-44s %s\n", ok ? "ok" : "FAIL", what, + detail ? detail : ""); + if (!ok) + failures++; +} + +/* props[0] encodes (pb * 5 + lp) * 9 + lc; the remaining four bytes + * are the dictionary size, which this test does not care about. */ +static void make_props(uint8_t out[5], unsigned lc, unsigned lp, + unsigned pb) +{ + out[0] = (uint8_t)((pb * 5 + lp) * 9 + lc); + out[1] = 0; + out[2] = 0; + out[3] = 0; + out[4] = 1; +} + +int main(void) +{ + /* Static rather than automatic: the probability array inside is + * tens of kilobytes, which is more than some of the platforms this + * builds for give a thread. */ + static rlzma_dec_t dec; + uint8_t props[5]; + unsigned lc, lp, pb; + int rejected_within_limit = 0; + char detail[128]; + + printf("lzma property limit test\n\n"); + + /* The case that was failing in the field. */ + make_props(props, 4, 0, 2); + sprintf(detail, "props 0x%02X", props[0]); + check(rlzma_dec_init(&dec, props) == RLZMA_OK, + "lc=4 lp=0 pb=2, as 7-Zip writes it", detail); + + /* Anything the header says it accepts, it has to accept. */ + for (pb = 0; pb <= 4; pb++) + { + for (lp = 0; lp <= 4; lp++) + { + for (lc = 0; lc <= 8; lc++) + { + unsigned d = (pb * 5 + lp) * 9 + lc; + + if (d >= 9 * 5 * 5) /* not encodable in one byte */ + continue; + if (lc + lp > RLZMA_LCLP_MAX) + continue; + + make_props(props, lc, lp, pb); + if (rlzma_dec_init(&dec, props) != RLZMA_OK) + { + if (!rejected_within_limit) + printf(" first rejected: lc=%u lp=%u pb=%u " + "(props 0x%02X)\n", lc, lp, pb, d); + rejected_within_limit++; + } + } + } + } + sprintf(detail, "%d rejected", rejected_within_limit); + check(rejected_within_limit == 0, + "every lc/lp within RLZMA_LCLP_MAX accepted", detail); + + /* The array has to be big enough for the limit the header claims. + * These moved apart once already: the constant said 3 while the + * comment beside it described the size for 4. */ + sprintf(detail, "max=%d, probs=%u", RLZMA_LCLP_MAX, + (unsigned)RLZMA_NUM_PROBS); + check(RLZMA_NUM_PROBS + >= (unsigned)(1984 + (0x300u << RLZMA_LCLP_MAX)), + "probability array sized for the limit", detail); + + /* LZMA2 wraps the same coder; a stream legal for one is legal for + * the other, so a lower limit here would reject content that + * arrives through the other path perfectly well. */ + check(RLZMA_LCLP_MAX >= 4, + "limit covers what the format allows in practice", + RLZMA_LCLP_MAX >= 4 ? "4 or more" : "below 4"); + + printf("\n%d checks, %d failures\n", checks, failures); + return failures ? 1 : 0; +} diff --git a/libretro-common/streams/interface_stream.c b/libretro-common/streams/interface_stream.c index 87493f2b6369..394c45b80a0c 100644 --- a/libretro-common/streams/interface_stream.c +++ b/libretro-common/streams/interface_stream.c @@ -22,6 +22,7 @@ #include +#include #include #include #include @@ -57,9 +58,81 @@ struct intfstream_internal rzipstream_t *fp; } rzip; #endif + struct + { + RFILE *fp; + uint8_t *buf; /* window contents */ + uint64_t cap; /* window size */ + uint64_t lo; /* file offset of buf[0] */ + uint64_t hi; /* file offset one past buf's last byte */ + uint64_t pos; /* logical read position */ + uint64_t size; + } buffered; enum intfstream_type type; }; +/* Refill the window so that [pos, pos+len) is resident. */ +static bool intfstream_buffered_fill(intfstream_internal_t *intf, + uint64_t pos, uint64_t len) +{ + int64_t got; + + if (len > intf->buffered.cap || pos + len > intf->buffered.size) + return false; + if (filestream_seek(intf->buffered.fp, (int64_t)pos, + RETRO_VFS_SEEK_POSITION_START) < 0) + return false; + if ((got = filestream_read(intf->buffered.fp, intf->buffered.buf, + (int64_t)intf->buffered.cap)) <= 0) + return false; + + intf->buffered.lo = pos; + intf->buffered.hi = pos + (uint64_t)got; + return (pos + len <= intf->buffered.hi); +} + +intfstream_t *intfstream_open_buffered(const char *path, uint64_t window) +{ + intfstream_info_t info; + intfstream_internal_t *fd = NULL; + RFILE *fp = NULL; + int64_t sz; + + if (!path || !*path || !window) + return NULL; + + if (!(fp = filestream_open(path, RETRO_VFS_FILE_ACCESS_READ, + RETRO_VFS_FILE_ACCESS_HINT_NONE))) + return NULL; + + if ((sz = filestream_get_size(fp)) < 0) + { + filestream_close(fp); + return NULL; + } + + memset(&info, 0, sizeof(info)); + info.type = INTFSTREAM_BUFFERED; + + if (!(fd = (intfstream_internal_t*)intfstream_init(&info))) + { + filestream_close(fp); + return NULL; + } + + if (!(fd->buffered.buf = (uint8_t*)malloc((size_t)window))) + { + filestream_close(fp); + free(fd); + return NULL; + } + + fd->buffered.fp = fp; + fd->buffered.cap = window; + fd->buffered.size = (uint64_t)sz; + return fd; +} + int64_t intfstream_get_size(intfstream_internal_t *intf) { if (!intf) @@ -77,6 +150,8 @@ int64_t intfstream_get_size(intfstream_internal_t *intf) #else break; #endif + case INTFSTREAM_BUFFERED: + return (int64_t)intf->buffered.size; case INTFSTREAM_RZIP: #if defined(HAVE_COMPRESSION) return rzipstream_get_size(intf->rzip.fp); @@ -114,6 +189,10 @@ bool intfstream_open(intfstream_internal_t *intf, const char *path, #else return false; #endif + case INTFSTREAM_BUFFERED: + if (!intf->buffered.fp) + return false; + break; case INTFSTREAM_RZIP: #if defined(HAVE_COMPRESSION) intf->rzip.fp = rzipstream_open(path, mode); @@ -139,6 +218,8 @@ int intfstream_flush(intfstream_internal_t *intf) return filestream_flush(intf->file.fp); case INTFSTREAM_MEMORY: case INTFSTREAM_CHD: + case INTFSTREAM_BUFFERED: + return 0; /* read-only */ case INTFSTREAM_RZIP: /* Should we stub this for these interfaces? */ break; @@ -168,6 +249,14 @@ int intfstream_close(intfstream_internal_t *intf) chdstream_close(intf->chd.fp); #endif return 0; + case INTFSTREAM_BUFFERED: + if (intf->buffered.fp) + filestream_close(intf->buffered.fp); + if (intf->buffered.buf) + free(intf->buffered.buf); + intf->buffered.fp = NULL; + intf->buffered.buf = NULL; + return 0; case INTFSTREAM_RZIP: #if defined(HAVE_COMPRESSION) if (intf->rzip.fp) @@ -189,6 +278,7 @@ void *intfstream_init(intfstream_info_t *info) return NULL; intf->type = info->type; + memset(&intf->buffered, 0, sizeof(intf->buffered)); intf->file.fp = NULL; intf->memory.fp = NULL; #ifdef HAVE_CHD @@ -214,6 +304,8 @@ void *intfstream_init(intfstream_info_t *info) free(intf); return NULL; #endif + case INTFSTREAM_BUFFERED: + break; /* filled in by intfstream_open_buffered() */ case INTFSTREAM_RZIP: break; } @@ -255,6 +347,32 @@ int64_t intfstream_seek( #else break; #endif + case INTFSTREAM_BUFFERED: + { + int64_t origin; + switch (whence) + { + case RETRO_VFS_SEEK_POSITION_START: + origin = 0; + break; + case RETRO_VFS_SEEK_POSITION_CURRENT: + origin = (int64_t)intf->buffered.pos; + break; + case RETRO_VFS_SEEK_POSITION_END: + origin = (int64_t)intf->buffered.size; + break; + default: + return -1; + } + if ( origin + offset < 0 + || (uint64_t)(origin + offset) > intf->buffered.size) + return -1; + /* Purely logical: a seek back into the window costs + * nothing, and one outside it is paid for by the refill + * on the next read rather than here. */ + intf->buffered.pos = (uint64_t)(origin + offset); + return (int64_t)intf->buffered.pos; + } case INTFSTREAM_RZIP: /* Unsupported */ break; @@ -276,6 +394,8 @@ int64_t intfstream_truncate(intfstream_internal_t *intf, uint64_t len) break; case INTFSTREAM_CHD: break; + case INTFSTREAM_BUFFERED: + return -1; /* read-only */ case INTFSTREAM_RZIP: break; } @@ -300,6 +420,47 @@ int64_t intfstream_read(intfstream_internal_t *intf, void *s, uint64_t len) #else break; #endif + case INTFSTREAM_BUFFERED: + { + uint64_t p = intf->buffered.pos; + uint64_t n = len; + if (p >= intf->buffered.size) + return 0; + if (p + n > intf->buffered.size) + n = intf->buffered.size - p; + if (n == 0) + return 0; + /* A read wider than the window cannot be served from it. + * Go straight to the file rather than failing: field + * payloads are read in one call, so a single large field + * would otherwise be unreadable and the record silently + * lost. */ + if (n > intf->buffered.cap) + { + int64_t got; + if (filestream_seek(intf->buffered.fp, (int64_t)p, + RETRO_VFS_SEEK_POSITION_START) < 0) + return -1; + if ((got = filestream_read(intf->buffered.fp, s, + (int64_t)n)) <= 0) + return -1; + /* The window no longer describes the file position. */ + intf->buffered.lo = 0; + intf->buffered.hi = 0; + intf->buffered.pos = p + (uint64_t)got; + return got; + } + /* Hit test inline: the callers this exists for issue on + * the order of a million reads, so an out-of-line call on + * the common path is itself measurable. */ + if ( !(p >= intf->buffered.lo && p + n <= intf->buffered.hi) + && !intfstream_buffered_fill(intf, p, n)) + return -1; + memcpy(s, intf->buffered.buf + (p - intf->buffered.lo), + (size_t)n); + intf->buffered.pos = p + n; + return (int64_t)n; + } case INTFSTREAM_RZIP: #if defined(HAVE_COMPRESSION) return rzipstream_read(intf->rzip.fp, s, len); @@ -325,6 +486,8 @@ int64_t intfstream_write(intfstream_internal_t *intf, return memstream_write(intf->memory.fp, s, len); case INTFSTREAM_CHD: return -1; + case INTFSTREAM_BUFFERED: + return -1; /* read-only */ case INTFSTREAM_RZIP: #if defined(HAVE_COMPRESSION) return rzipstream_write(intf->rzip.fp, s, len); @@ -356,6 +519,8 @@ int intfstream_printf(intfstream_internal_t *intf, return -1; case INTFSTREAM_CHD: return -1; + case INTFSTREAM_BUFFERED: + return -1; /* read-only */ case INTFSTREAM_RZIP: #if defined(HAVE_COMPRESSION) va_start(vl, format); @@ -383,6 +548,8 @@ int64_t intfstream_get_ptr(intfstream_internal_t* intf) return memstream_get_ptr(intf->memory.fp); case INTFSTREAM_CHD: return -1; + case INTFSTREAM_BUFFERED: + return (int64_t)intf->buffered.pos; case INTFSTREAM_RZIP: return -1; } @@ -410,6 +577,25 @@ char *intfstream_gets(intfstream_internal_t *intf, #else break; #endif + case INTFSTREAM_BUFFERED: + { + uint64_t i = 0; + if (len == 0) + return NULL; + while (i + 1 < (uint64_t)len) + { + uint8_t c; + if (intfstream_read(intf, &c, 1) != 1) + break; + s[i++] = (char)c; + if (c == '\n') + break; + } + if (i == 0) + return NULL; + s[i] = '\0'; + return s; + } case INTFSTREAM_RZIP: #if defined(HAVE_COMPRESSION) return rzipstream_gets(intf->rzip.fp, s, (size_t)len); @@ -438,6 +624,13 @@ int intfstream_getc(intfstream_internal_t *intf) #else break; #endif + case INTFSTREAM_BUFFERED: + { + uint8_t c; + if (intfstream_read(intf, &c, 1) != 1) + return EOF; + return (int)c; + } case INTFSTREAM_RZIP: #if defined(HAVE_COMPRESSION) return rzipstream_getc(intf->rzip.fp); @@ -466,6 +659,8 @@ int64_t intfstream_tell(intfstream_internal_t *intf) #else break; #endif + case INTFSTREAM_BUFFERED: + return (int64_t)intf->buffered.pos; case INTFSTREAM_RZIP: #if defined(HAVE_COMPRESSION) return (int64_t)rzipstream_tell(intf->rzip.fp); @@ -494,6 +689,8 @@ int intfstream_eof(intfstream_internal_t *intf) /* TODO: Add this functionality to * chd_stream interface */ break; + case INTFSTREAM_BUFFERED: + return (intf->buffered.pos >= intf->buffered.size); case INTFSTREAM_RZIP: #if defined(HAVE_COMPRESSION) return rzipstream_eof(intf->rzip.fp); @@ -520,6 +717,9 @@ void intfstream_rewind(intfstream_internal_t *intf) chdstream_rewind(intf->chd.fp); #endif break; + case INTFSTREAM_BUFFERED: + intf->buffered.pos = 0; + return; case INTFSTREAM_RZIP: #if defined(HAVE_COMPRESSION) rzipstream_rewind(intf->rzip.fp); @@ -543,6 +743,8 @@ void intfstream_putc(intfstream_internal_t *intf, int c) break; case INTFSTREAM_CHD: break; + case INTFSTREAM_BUFFERED: + return; /* read-only */ case INTFSTREAM_RZIP: #if defined(HAVE_COMPRESSION) rzipstream_putc(intf->rzip.fp, c); @@ -604,6 +806,8 @@ bool intfstream_is_compressed(intfstream_internal_t *intf) return false; case INTFSTREAM_CHD: return true; + case INTFSTREAM_BUFFERED: + return false; case INTFSTREAM_RZIP: #if defined(HAVE_COMPRESSION) return rzipstream_is_compressed(intf->rzip.fp); diff --git a/libretro-db/bintree.c b/libretro-db/bintree.c index 6f5889842109..2825f694c48a 100644 --- a/libretro-db/bintree.c +++ b/libretro-db/bintree.c @@ -50,12 +50,30 @@ int bintree_insert(bintree_t *t, struct bintree_node *root, void *value) { int cmp_res = 0; + /* A NULL node means an earlier bintree_new_nil_node() failed and + * left a hole in the tree. This used to report 0 - the same value + * a successful insert returns - so a caller could not tell that + * nothing had been stored, and went on to write an index missing + * the entries that fell into the hole. */ if (!root) - return 0; + return -2; + if (root->value == NIL_NODE) { - root->left = bintree_new_nil_node(root); - root->right = bintree_new_nil_node(root); + struct bintree_node *l = bintree_new_nil_node(root); + struct bintree_node *r = bintree_new_nil_node(root); + + /* Commit only if both children exist, otherwise the next insert + * that descends this way hits the NULL case above. */ + if (!l || !r) + { + free(l); + free(r); + return -2; + } + + root->left = l; + root->right = r; root->value = value; return 0; } @@ -108,7 +126,14 @@ bintree_t *bintree_new(bintree_cmp_func cmp, void *ctx) if (!t) return NULL; - t->root = bintree_new_nil_node(NULL); + if (!(t->root = bintree_new_nil_node(NULL))) + { + /* Returning a tree with no root made every later insert take + * the !root path, which reported success. */ + free(t); + return NULL; + } + t->cmp = cmp; t->ctx = ctx; diff --git a/libretro-db/bintree.h b/libretro-db/bintree.h index 84d71c846e00..006b7935469a 100644 --- a/libretro-db/bintree.h +++ b/libretro-db/bintree.h @@ -47,6 +47,10 @@ typedef struct bintree bintree_t *bintree_new(bintree_cmp_func cmp, void *ctx); +/* Returns 0 on success, -1 if an equal value is already present, + * -2 if the tree could not be grown. The tree stores @value by + * pointer and never frees it; releasing the values is the caller's + * job, and bintree_iterate() is the way to reach them all. */ int bintree_insert(bintree_t *t, struct bintree_node *root, void *value); int bintree_iterate(struct bintree_node *n, bintree_iter_cb cb, void *ctx); diff --git a/libretro-db/c_converter.c b/libretro-db/c_converter.c index a01abb21588e..b808e2824ec5 100644 --- a/libretro-db/c_converter.c +++ b/libretro-db/c_converter.c @@ -112,6 +112,15 @@ static dat_converter_list_t* dat_converter_list_create( { dat_converter_list_t* list = (dat_converter_list_t*)malloc(sizeof(*list)); + /* Neither allocation was checked; the field writes below and the + * indexed stores in dat_converter_list_append() both go through a + * NULL on OOM. */ + if (!list) + { + printf("fatal error: out of memory\n"); + dat_converter_exit(1); + } + list->type = type; list->count = 0; list->capacity = (1 << 2); @@ -119,6 +128,12 @@ static dat_converter_list_t* dat_converter_list_create( list->values = (dat_converter_list_item_t*)malloc( sizeof(*list->values) * list->capacity); + if (!list->values) + { + printf("fatal error: out of memory\n"); + dat_converter_exit(1); + } + return list; } @@ -214,8 +229,21 @@ static void dat_converter_list_append(dat_converter_list_t* dst, void* item) { if (dst->count == dst->capacity) { + /* realloc-to-tmp: assigning the result straight back drops the + * only reference to the old block on failure, and left capacity + * claiming the larger size while the allocation was still the + * old one. */ + dat_converter_list_item_t *tmp = (dat_converter_list_item_t*) + realloc(dst->values, sizeof(*dst->values) * (dst->capacity << 1)); + + if (!tmp) + { + printf("fatal error: out of memory\n"); + dat_converter_exit(1); + } + + dst->values = tmp; dst->capacity <<= 1; - dst->values = realloc(dst->values, sizeof(*dst->values) * dst->capacity); } switch (dst->type) @@ -325,14 +353,45 @@ static dat_converter_list_t* dat_converter_lexer( return token_list; } +/* Deepest table nesting accepted from a DAT. dat_parser_table() + * recurses once per '(' and had no limit, so a file consisting of + * nested tables exhausted the stack rather than being reported: + * 100000 levels (a ~1 MB DAT) gives + * + * AddressSanitizer: stack-overflow + * + * Real DAT files nest three deep at most (the root table, a game + * table, and a rom table inside it), so this is far above anything + * legitimate. */ +#define DAT_CONVERTER_MAX_DEPTH 64 + +static dat_converter_list_t* dat_parser_table_depth( + dat_converter_list_item_t** start_token, unsigned depth); + static dat_converter_list_t* dat_parser_table( dat_converter_list_item_t** start_token) +{ + return dat_parser_table_depth(start_token, 0); +} + +static dat_converter_list_t* dat_parser_table_depth( + dat_converter_list_item_t** start_token, unsigned depth) { dat_converter_list_t* parsed_table = dat_converter_list_create(DAT_CONVERTER_MAP_LIST); dat_converter_map_t map = {0}; dat_converter_list_item_t* current = *start_token; + if (depth >= DAT_CONVERTER_MAX_DEPTH) + { + printf("%s:%d:%d: fatal error: tables nested deeper than %d\n", + current->token.fname, + current->token.line_no, + current->token.column, + DAT_CONVERTER_MAX_DEPTH); + dat_converter_exit(1); + } + while (current->token.label) { @@ -364,7 +423,7 @@ static dat_converter_list_t* dat_parser_table( { current++; map.type = DAT_CONVERTER_LIST_MAP; - map.value.list = dat_parser_table(¤t); + map.value.list = dat_parser_table_depth(¤t, depth + 1); dat_converter_list_append(parsed_table, &map); } else if (string_is_equal(current->token.label, ")")) diff --git a/libretro-db/libretrodb.c b/libretro-db/libretrodb.c index 3d195779bb20..a463e486f486 100644 --- a/libretro-db/libretrodb.c +++ b/libretro-db/libretrodb.c @@ -44,6 +44,17 @@ #define MAGIC_NUMBER "RARCHDB" +/* Must match MAX_ERROR_LEN in query.c */ +#define LIBRETRODB_QUERY_ERR_LEN 256 + +/* Sliding window used for the cursor walk. Has to comfortably exceed + * the largest backwards jump the cursor makes - a rewind to the start + * of the record being inspected - so that the rewind stays inside the + * window and costs nothing. */ +#ifndef LIBRETRODB_WINDOW_SIZE +#define LIBRETRODB_WINDOW_SIZE (64 * 1024) +#endif + /* MsgPack type bytes — needed by the fast cursor read path */ #define _MPF_FIXMAP 0x80 #define _MPF_FIXARRAY 0x90 @@ -64,6 +75,13 @@ struct node_iter_ctx struct libretrodb { intfstream_t *fd; + /* Scratch for libretrodb_query_compile()'s error text. It used to + * be a function-scope static, so the pointer handed back through + * err_string was shared by every caller in the process and two + * concurrent compiles overwrote each other's message. Tying it to + * the db handle keeps the pointer valid for as long as the caller + * can meaningfully use it while making it per-handle. */ + char query_err[LIBRETRODB_QUERY_ERR_LEN]; char *path; bool can_write; uint64_t root; @@ -71,6 +89,11 @@ struct libretrodb uint64_t first_index_offset; }; +/* Widest index key the format is expected to carry (SHA-1 is 20 + * bytes). Used to reject nonsense key sizes from a malformed index + * header before they reach binsearch(). */ +#define LIBRETRODB_MAX_KEY_SIZE 256 + struct libretrodb_index { char name[50]; @@ -124,7 +147,7 @@ static int libretrodb_validate_document(const struct rmsgpack_dom_value *doc) if (value.type != RDT_MAP) continue; - if ((rv == libretrodb_validate_document(&value)) != 0) + if ((rv = libretrodb_validate_document(&value)) != 0) return rv; } @@ -217,7 +240,11 @@ int libretrodb_open(const char *path, libretrodb_t *db, bool write) db->path = strdup(path); db->root = intfstream_tell(fd); - if ((int)intfstream_read(fd, &header, sizeof(header)) == -1) + /* intfstream_read() signals EOF as a short read, not as -1, so a + * file smaller than the header used to leave the tail of 'header' + * uninitialised and then feed it to strncmp() below. Require the + * full header. */ + if (intfstream_read(fd, &header, sizeof(header)) != (int64_t)sizeof(header)) goto error; if (strncmp(header.magic_number, MAGIC_NUMBER, sizeof(MAGIC_NUMBER)) != 0) @@ -245,7 +272,7 @@ int libretrodb_open(const char *path, libretrodb_t *db, bool write) RETRO_VFS_SEEK_POSITION_START) < 0) goto error; - if (rmsgpack_dom_read_into(fd, "count", &md.count, NULL) < 0) + if (rmsgpack_dom_read_into(fd, "count", RDF_UINT, &md.count, NULL) < 0) goto error; db->count = md.count; @@ -291,11 +318,11 @@ static int libretrodb_find_index(libretrodb_t *db, const char *index_name, uint64_t name_len = 50; /* Read index header */ if (rmsgpack_dom_read_into(db->fd, - "name", idx->name, &name_len, - "key_size", &idx->key_size, - "next", &idx->next, - "count", &idx->count, - NULL) < 0) + "name", RDF_STRING, idx->name, &name_len, + "key_size", RDF_UINT, &idx->key_size, + "next", RDF_UINT, &idx->next, + "count", RDF_UINT, &idx->count, + NULL) < 0) { printf("Invalid index header\n"); break; @@ -304,35 +331,76 @@ static int libretrodb_find_index(libretrodb_t *db, const char *index_name, if (strncmp(index_name, idx->name, strlen(idx->name)) == 0) return 0; - intfstream_seek(db->fd, (ssize_t)idx->next, - RETRO_VFS_SEEK_POSITION_CURRENT); + /* idx->next is a file-supplied relative seek. Zero re-reads + * the same header forever and a value that wraps negative + * seeks backwards, so intfstream_eof() is never reached and + * the lookup hangs. Require forward progress. */ + if (idx->next == 0 || idx->next > (uint64_t)INT64_MAX) + break; + + if (intfstream_seek(db->fd, (ssize_t)idx->next, + RETRO_VFS_SEEK_POSITION_CURRENT) < 0) + break; } return -1; } +/** + * binsearch: + * + * Locate @item in the @count fixed-size entries at @buff and return + * the record offset stored alongside it. Each entry is @field_size + * key bytes followed by a uint64 offset. + * + * The previous recursive form had three defects, all reachable from + * a malformed index header: + * + * - the memcmp() ran before the "count == 0" test, so an empty + * sub-range was 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 offset was read with *(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 - undefined + * behaviour that UBSan flags and that faults outright on + * strict-alignment targets. + * + * Iterative, half-open [lo, hi), and the offset is assembled with + * memcpy so alignment never matters. + */ static int binsearch(const void *buff, const void *item, - uint64_t count, uint8_t field_size, uint64_t *offset) + uint64_t count, uint64_t field_size, uint64_t *offset) { - int mid = (int)(count / 2); - int item_size = field_size + sizeof(uint64_t); - uint8_t *current = ((uint8_t *)buff + (mid * item_size)); - int rv = memcmp(current, item, field_size); + uint64_t lo = 0; + uint64_t hi = count; + uint64_t item_size = field_size + sizeof(uint64_t); - if (rv == 0) + if (field_size == 0) + return -1; + + while (lo < hi) { - *offset = *(uint64_t *)(current + field_size); - return 0; - } + uint64_t mid = lo + ((hi - lo) / 2); + const uint8_t *current = (const uint8_t *)buff + (mid * item_size); + int rv = memcmp(current, item, (size_t)field_size); - if (count == 0) - return -1; + if (rv == 0) + { + memcpy(offset, current + field_size, sizeof(uint64_t)); + return 0; + } - if (rv > 0) - return binsearch(buff, item, mid, field_size, offset); + if (rv > 0) + hi = mid; + else + lo = mid + 1; + } - return binsearch(current + item_size, item, - count - mid, field_size, offset); + return -1; } int libretrodb_find_entry(libretrodb_t *db, const char *index_name, @@ -342,19 +410,38 @@ int libretrodb_find_entry(libretrodb_t *db, const char *index_name, int rv; uint8_t *buff; uint64_t offset; - ssize_t bufflen, nread = 0; + uint64_t item_size; + uint64_t bufflen; + int64_t nread = 0; if (libretrodb_find_index(db, index_name, &idx) < 0) return -1; + /* idx.next, idx.count and idx.key_size all come from the file + * with no relationship enforced between them. binsearch() walks + * idx.count entries of (key_size + 8) bytes, so without this + * check a small "next" and a large "count" send it off the end of + * the allocation. Require the payload the header describes to + * actually fit in the payload the header reserved, and reject + * degenerate key sizes outright. */ + if (idx.key_size == 0 || idx.key_size > LIBRETRODB_MAX_KEY_SIZE) + return -1; + + item_size = idx.key_size + sizeof(uint64_t); + if (idx.count > idx.next / item_size) + return -1; + bufflen = idx.next; - if (!(buff = (uint8_t*)malloc(bufflen))) + if (bufflen == 0 || bufflen > (uint64_t)INT64_MAX) + return -1; + if (!(buff = (uint8_t*)malloc((size_t)bufflen))) return -1; - while (nread < bufflen) + while (nread < (int64_t)bufflen) { void *buff_ = (buff + nread); - rv = (int)intfstream_read(db->fd, buff_, bufflen - nread); + rv = (int)intfstream_read(db->fd, buff_, + (uint64_t)((int64_t)bufflen - nread)); if (rv <= 0) { @@ -364,7 +451,7 @@ int libretrodb_find_entry(libretrodb_t *db, const char *index_name, nread += rv; } - rv = binsearch(buff, key, idx.count, (ssize_t)idx.key_size, &offset); + rv = binsearch(buff, key, idx.count, idx.key_size, &offset); free(buff); if (rv == 0) @@ -404,7 +491,7 @@ static int32_t rmsgpack_read_map_header(intfstream_t *fd) uint8_t type = 0; uint64_t len = 0; - if (intfstream_read(fd, &type, 1) == -1) + if (intfstream_read(fd, &type, 1) != 1) return -1; if (type == _MPF_NIL) @@ -442,7 +529,7 @@ static int32_t rmsgpack_read_key_string(intfstream_t *fd, uint8_t type = 0; uint64_t len = 0; - if (intfstream_read(fd, &type, 1) == -1) + if (intfstream_read(fd, &type, 1) != 1) return -1; /* fixstr: length embedded in type byte */ @@ -475,7 +562,7 @@ static int32_t rmsgpack_read_key_string(intfstream_t *fd, return -1; } - if (intfstream_read(fd, buf, (size_t)len) == -1) + if (intfstream_read(fd, buf, (size_t)len) != (int64_t)len) return -1; buf[len] = '\0'; @@ -486,6 +573,271 @@ static int32_t rmsgpack_read_key_string(intfstream_t *fd, * fast path. Typical .rdb records have 10-15 fields. */ #define CURSOR_MAX_MAP_FIELDS 24 +/** + * libretrodb_scan_field: + * + * Walk the database once, reporting the binary value of @field and + * the offset of the record carrying it. Records without the field, + * or carrying it as something other than binary, are skipped. + * + * This exists so a caller can build its own lookup structure in one + * pass instead of re-walking the whole database for every key it + * wants to test. Nothing is parsed beyond the field itself: values + * the caller did not ask for are stepped over, not decoded. + * + * @cb is called for each match and may return non-zero to stop the + * scan, which is then reported as success. + * + * Returns: 0 when the database was walked, -1 on a malformed stream. + */ +/** + * rmsgpack_read_bin_inplace: + * + * Read a MsgPack binary value into a caller buffer. Returns -1 and + * leaves the stream positioned after the value when it is not binary + * or does not fit, so the caller can carry on scanning. + */ +static int rmsgpack_read_bin_inplace(intfstream_t *fd, uint8_t *buf, + size_t buf_size, uint64_t *len) +{ + uint8_t type = 0; + uint64_t n = 0; + + if (intfstream_read(fd, &type, 1) != 1) + return -1; + + switch (type) + { + case 0xc4: + if (rmsgpack_read_uint(fd, &n, 1) == -1) + return -1; + break; + case 0xc5: + if (rmsgpack_read_uint(fd, &n, 2) == -1) + return -1; + break; + case 0xc6: + if (rmsgpack_read_uint(fd, &n, 4) == -1) + return -1; + break; + default: + /* Not binary: step back over the type byte and let the + * generic skip deal with the whole value. */ + if (intfstream_seek(fd, -1, RETRO_VFS_SEEK_POSITION_CURRENT) < 0) + return -1; + if (rmsgpack_skip_value(fd) < 0) + return -1; + return -1; + } + + if (n > buf_size) + { + if (intfstream_seek(fd, (int64_t)n, + RETRO_VFS_SEEK_POSITION_CURRENT) < 0) + return -1; + return -1; + } + + if (n && intfstream_read(fd, buf, n) != (int64_t)n) + return -1; + + *len = n; + return 0; +} + +/** + * rmsgpack_read_uint_inplace: + * + * Read a MsgPack unsigned (or non-negative signed) value. Returns -1 + * and leaves the stream positioned after the value for anything else, + * so the caller can carry on scanning. + */ +static int rmsgpack_read_uint_inplace(intfstream_t *fd, uint64_t *out) +{ + uint8_t type = 0; + uint64_t n = 0; + + if (intfstream_read(fd, &type, 1) != 1) + return -1; + + /* positive fixint */ + if (type < 0x80) + { + *out = type; + return 0; + } + + switch (type) + { + case 0xcc: case 0xcd: case 0xce: case 0xcf: + if (rmsgpack_read_uint(fd, &n, (size_t)(1 << (type - 0xcc))) == -1) + return -1; + *out = n; + return 0; + default: + break; + } + + if (intfstream_seek(fd, -1, RETRO_VFS_SEEK_POSITION_CURRENT) < 0) + return -1; + if (rmsgpack_skip_value(fd) < 0) + return -1; + return -1; +} + +int libretrodb_scan_field(libretrodb_t *db, const char *field, + const char *aux_field, libretrodb_scan_cb cb, void *ctx) +{ + intfstream_t *fd; + size_t field_len; + size_t aux_len = 0; + int rv = -1; + + if (!db || !db->path || !*db->path || !field || !*field || !cb) + return -1; + + field_len = strlen(field); + if (aux_field && *aux_field) + aux_len = strlen(aux_field); + + if (!(fd = intfstream_open_buffered(db->path, LIBRETRODB_WINDOW_SIZE))) + if (!(fd = intfstream_open_file(db->path, + RETRO_VFS_FILE_ACCESS_READ, + RETRO_VFS_FILE_ACCESS_HINT_NONE))) + return -1; + + if (intfstream_seek(fd, (int64_t)(db->root + sizeof(libretrodb_header_t)), + RETRO_VFS_SEEK_POSITION_START) < 0) + goto end; + + for (;;) + { + int64_t record_start = intfstream_tell(fd); + int32_t map_len; + int32_t i; + int stop = 0; + int have_key = 0; + int have_aux = 0; + uint8_t key_val[LIBRETRODB_MAX_KEY_SIZE]; + uint64_t key_val_len = 0; + uint64_t aux_val = 0; + + if (record_start < 0) + goto end; + + map_len = rmsgpack_read_map_header(fd); + + if (map_len == -2) /* nil sentinel: end of records */ + { + rv = 0; + goto end; + } + if (map_len < 0) + goto end; + + /* The field and its companion can sit either way round in the + * map, so both are collected before the record is reported. */ + have_key = 0; + have_aux = 0; + + for (i = 0; i < map_len; i++) + { + char key_buf[64]; + int32_t key_len = rmsgpack_read_key_string(fd, key_buf, + sizeof(key_buf)); + + if (key_len < 0) + { + /* Key was unreadable or too long for the buffer; its + * value still has to be stepped over. */ + if (rmsgpack_skip_value(fd) < 0) + goto end; + continue; + } + + if ( !have_key + && (size_t)key_len == field_len + && memcmp(key_buf, field, field_len) == 0) + { + /* Not binary, or wider than any key we index: the reader + * has already stepped past the value. */ + if (rmsgpack_read_bin_inplace(fd, key_val, sizeof(key_val), + &key_val_len) == 0) + have_key = 1; + continue; + } + + if ( aux_len + && !have_aux + && (size_t)key_len == aux_len + && memcmp(key_buf, aux_field, aux_len) == 0) + { + if (rmsgpack_read_uint_inplace(fd, &aux_val) == 0) + have_aux = 1; + continue; + } + + if (rmsgpack_skip_value(fd) < 0) + goto end; + } + + if (have_key) + { + int64_t resume = intfstream_tell(fd); + + if (cb(ctx, key_val, (size_t)key_val_len, + (uint64_t)record_start, have_aux ? &aux_val : NULL)) + stop = 1; + + if (resume < 0 + || intfstream_seek(fd, resume, + RETRO_VFS_SEEK_POSITION_START) < 0) + goto end; + if (stop) + { + rv = 0; + goto end; + } + } + } + +end: + intfstream_close(fd); + free(fd); + return rv; +} + +/** + * libretrodb_read_at: + * + * Read the record beginning at @offset, which must have come from + * libretrodb_scan_field(). Lets a caller that kept only offsets + * fetch a record without walking to it. + */ +int libretrodb_read_at(libretrodb_t *db, uint64_t offset, + struct rmsgpack_dom_value *out) +{ + intfstream_t *fd; + int rv = -1; + + if (!db || !db->path || !*db->path || !out) + return -1; + + if (!(fd = intfstream_open_buffered(db->path, LIBRETRODB_WINDOW_SIZE))) + if (!(fd = intfstream_open_file(db->path, + RETRO_VFS_FILE_ACCESS_READ, + RETRO_VFS_FILE_ACCESS_HINT_NONE))) + return -1; + + if (intfstream_seek(fd, (int64_t)offset, + RETRO_VFS_SEEK_POSITION_START) >= 0) + rv = (rmsgpack_dom_read(fd, out) < 0) ? -1 : 0; + + intfstream_close(fd); + free(fd); + return rv; +} + int libretrodb_cursor_read_item(libretrodb_cursor_t *cursor, struct rmsgpack_dom_value *out) { @@ -727,13 +1079,20 @@ int libretrodb_cursor_open(libretrodb_t *db, libretrodb_query_t *q) { intfstream_t *fd = NULL; + if (!db || !db->path || !*db->path) return -1; - if (!(fd = intfstream_open_file(db->path, - RETRO_VFS_FILE_ACCESS_READ, - RETRO_VFS_FILE_ACCESS_HINT_NONE))) - return -1; + /* Walking the record stream is what a content scan spends its time + * on, and most of that is the per-read trip through + * filestream/VFS/fread rather than parsing. A sliding window turns + * those reads into a memcpy while keeping the resident cost fixed + * at LIBRETRODB_WINDOW_SIZE instead of the size of the database. */ + if (!(fd = intfstream_open_buffered(db->path, LIBRETRODB_WINDOW_SIZE))) + if (!(fd = intfstream_open_file(db->path, + RETRO_VFS_FILE_ACCESS_READ, + RETRO_VFS_FILE_ACCESS_HINT_NONE))) + return -1; cursor->fd = fd; cursor->db = db; @@ -742,11 +1101,30 @@ int libretrodb_cursor_open(libretrodb_t *db, cursor->query = q; if (q) + { libretrodb_query_inc_ref(q); + /* min()/max() accumulate across the rows a cursor yields, so + * their state belongs to the walk, not to the compiled query. + * Reset it here as well as at compile time: a query outlives + * the cursor that references it, and a second walk over the + * same query used to inherit the first walk's extreme. */ + libretrodb_query_reset_accumulator(q); + } return 0; } +/* bintree stores values by pointer and does not own them, so the + * key buffers handed to bintree_insert() have to be released here. + * They were not, leaking (key_size + 8) bytes per indexed record - + * 240 KB for a 20000-record index. */ +static int node_free_iter(void *value, void *ctx) +{ + (void)ctx; + free(value); + return 0; +} + static int node_iter(void *value, void *ctx) { struct node_iter_ctx *nictx = (struct node_iter_ctx*)ctx; @@ -802,9 +1180,15 @@ int libretrodb_create_index(libretrodb_t *db, if (item.type != RDT_MAP) goto clean; - /* Field not found in item? */ + /* Field not found in item? The free at the end of the loop is + * skipped by this continue, so do it here. */ if (!(field = rmsgpack_dom_value_map_value(&item, &key))) - continue; + { + rmsgpack_dom_value_free(&item); + item.type = RDT_NULL; + item_loc = intfstream_tell(cur.fd); + continue; + } /* Field is not binary? */ if (field->type != RDT_BINARY) @@ -872,7 +1256,10 @@ int libretrodb_create_index(libretrodb_t *db, if (cur.is_valid) libretrodb_cursor_close(&cur); if (tree && tree->root) + { + bintree_iterate(tree->root, node_free_iter, NULL); bintree_free(tree->root); + } free(tree); return rval; } @@ -921,3 +1308,15 @@ void libretrodb_free(libretrodb_t *db) if (db) free(db); } + +/* Accessor for query.c, which owns the formatting but not the + * storage. Returns NULL when there is no db handle to borrow from; + * the caller then falls back to a fixed message. */ +char *libretrodb_query_err_buf(libretrodb_t *db, size_t *len) +{ + if (!db) + return NULL; + if (len) + *len = sizeof(db->query_err); + return db->query_err; +} diff --git a/libretro-db/libretrodb.h b/libretro-db/libretrodb.h index 10f5d1ee8d74..bdefb6cf5052 100644 --- a/libretro-db/libretrodb.h +++ b/libretro-db/libretrodb.h @@ -57,8 +57,31 @@ int libretrodb_create_index(libretrodb_t *db, const char *name, int libretrodb_find_entry(libretrodb_t *db, const char *index_name, const void *key, struct rmsgpack_dom_value *out); +/* Reported for each record carrying the scanned field: the field's + * binary value and the offset of the record it came from. Returning + * non-zero stops the scan. */ +/* Reported once per record carrying the scanned field: the field's + * binary value, the offset of the record, and the value of the + * companion numeric field when the record has one (NULL otherwise). + * Returning non-zero stops the scan. */ +typedef int (*libretrodb_scan_cb)(void *ctx, const uint8_t *key, + size_t key_len, uint64_t offset, const uint64_t *aux); + +/* @aux_field may be NULL. Naming one lets a caller collect a numeric + * field in the same pass rather than walking the database again for + * it. */ +int libretrodb_scan_field(libretrodb_t *db, const char *field, + const char *aux_field, libretrodb_scan_cb cb, void *ctx); + +int libretrodb_read_at(libretrodb_t *db, uint64_t offset, + struct rmsgpack_dom_value *out); + libretrodb_t *libretrodb_new(void); +/* Scratch buffer used by libretrodb_query_compile() for error text. + * Owned by the db handle so that concurrent compiles do not share it. */ +char *libretrodb_query_err_buf(libretrodb_t *db, size_t *len); + void libretrodb_free(libretrodb_t *db); libretrodb_cursor_t *libretrodb_cursor_new(void); diff --git a/libretro-db/query.c b/libretro-db/query.c index 35414c40302a..776fba425963 100644 --- a/libretro-db/query.c +++ b/libretro-db/query.c @@ -25,6 +25,7 @@ #else #include #endif +#include #include #include #include @@ -57,7 +58,28 @@ enum argument_type struct argument; +/* Per-query evaluation state. + * + * query_func_min()/query_func_max() report "smaller/larger than + * everything seen so far", so they need somewhere to keep the running + * extreme across the rows of a walk. That used to be a single + * file-scope 'intermediate_res', which two queries evaluated + * concurrently would corrupt for each other: + * + * WARNING: ThreadSanitizer: data race + * Write of size 8 by thread T2: + * #1 query_func_min query.c:310 + * Location is global 'intermediate_res' of size 24 + * + * Give every compiled query its own, and pass it to the evaluation + * functions rather than reaching for a global. */ +struct query_ctx +{ + struct rmsgpack_dom_value intermediate_res; +}; + typedef struct rmsgpack_dom_value (*rarch_query_func)( + struct query_ctx *ctx, struct rmsgpack_dom_value input, unsigned argc, const struct argument *argv); @@ -81,6 +103,7 @@ struct argument struct query { struct invocation root; /* ptr alignment */ + struct query_ctx ctx; unsigned ref_count; }; @@ -90,7 +113,14 @@ struct registered_func rarch_query_func func; }; -struct rmsgpack_dom_value intermediate_res; +void libretrodb_query_reset_accumulator(libretrodb_query_t *q) +{ + struct query *rq = (struct query *)q; + if (!rq) + return; + rq->ctx.intermediate_res.val.int_ = 0; + rq->ctx.intermediate_res.val.uint_ = 0; +} /* Forward declarations */ static struct buffer query_parse_method_call(char *s, size_t len, @@ -100,6 +130,7 @@ static struct buffer query_parse_table(char *s, size_t len, struct buffer buff, /* Errors */ static struct rmsgpack_dom_value query_func_is_true( + struct query_ctx *ctx, struct rmsgpack_dom_value input, unsigned argc, const struct argument *argv) { @@ -115,6 +146,7 @@ static struct rmsgpack_dom_value query_func_is_true( } static struct rmsgpack_dom_value func_equals( + struct query_ctx *ctx, struct rmsgpack_dom_value input, unsigned argc, const struct argument * argv) { @@ -143,6 +175,7 @@ static struct rmsgpack_dom_value func_equals( } static struct rmsgpack_dom_value query_func_operator_or( + struct query_ctx *ctx, struct rmsgpack_dom_value input, unsigned argc, const struct argument * argv) { @@ -155,10 +188,10 @@ static struct rmsgpack_dom_value query_func_operator_or( for (i = 0; i < argc; i++) { if (argv[i].type == AT_VALUE) - res = func_equals(input, 1, &argv[i]); + res = func_equals(ctx, input, 1, &argv[i]); else - res = query_func_is_true( - argv[i].a.invocation.func(input, + res = query_func_is_true(ctx, + argv[i].a.invocation.func(ctx, input, argv[i].a.invocation.argc, argv[i].a.invocation.argv ), 0, NULL); @@ -171,6 +204,7 @@ static struct rmsgpack_dom_value query_func_operator_or( } static struct rmsgpack_dom_value query_func_operator_and( + struct query_ctx *ctx, struct rmsgpack_dom_value input, unsigned argc, const struct argument * argv) { @@ -183,10 +217,10 @@ static struct rmsgpack_dom_value query_func_operator_and( for (i = 0; i < argc; i++) { if (argv[i].type == AT_VALUE) - res = func_equals(input, 1, &argv[i]); + res = func_equals(ctx, input, 1, &argv[i]); else - res = query_func_is_true( - argv[i].a.invocation.func(input, + res = query_func_is_true(ctx, + argv[i].a.invocation.func(ctx, input, argv[i].a.invocation.argc, argv[i].a.invocation.argv ), @@ -199,6 +233,7 @@ static struct rmsgpack_dom_value query_func_operator_and( } static struct rmsgpack_dom_value query_func_between( + struct query_ctx *ctx, struct rmsgpack_dom_value input, unsigned argc, const struct argument * argv) { @@ -236,6 +271,7 @@ static struct rmsgpack_dom_value query_func_between( } static struct rmsgpack_dom_value query_func_glob( + struct query_ctx *ctx, struct rmsgpack_dom_value input, unsigned argc, const struct argument * argv) { @@ -260,6 +296,7 @@ static struct rmsgpack_dom_value query_func_glob( * - in practice, last entry will contain the actual min/max value. * * Empty result means there is no such field. */ static struct rmsgpack_dom_value query_func_min( + struct query_ctx *ctx, struct rmsgpack_dom_value input, unsigned argc, const struct argument * argv) { @@ -273,18 +310,18 @@ static struct rmsgpack_dom_value query_func_min( case RDT_INT: res.val.bool_ = ( (input.val.int_ == 0) - || (input.val.int_ < intermediate_res.val.int_) - || (intermediate_res.val.int_ == 0)); - if (res.val.bool_ || intermediate_res.val.int_ == 0) - memcpy(&intermediate_res, &input, sizeof(intermediate_res)); + || (input.val.int_ < ctx->intermediate_res.val.int_) + || (ctx->intermediate_res.val.int_ == 0)); + if (res.val.bool_ || ctx->intermediate_res.val.int_ == 0) + memcpy(&ctx->intermediate_res, &input, sizeof(ctx->intermediate_res)); break; case RDT_UINT: res.val.bool_ = ( (input.val.uint_ == 0) - || (input.val.uint_ < intermediate_res.val.uint_) - || (intermediate_res.val.uint_ == 0)); - if (res.val.bool_ || intermediate_res.val.uint_ == 0) - memcpy(&intermediate_res, &input, sizeof(intermediate_res)); + || (input.val.uint_ < ctx->intermediate_res.val.uint_) + || (ctx->intermediate_res.val.uint_ == 0)); + if (res.val.bool_ || ctx->intermediate_res.val.uint_ == 0) + memcpy(&ctx->intermediate_res, &input, sizeof(ctx->intermediate_res)); break; default: break; @@ -294,6 +331,7 @@ static struct rmsgpack_dom_value query_func_min( } static struct rmsgpack_dom_value query_func_max( + struct query_ctx *ctx, struct rmsgpack_dom_value input, unsigned argc, const struct argument * argv) { @@ -307,18 +345,18 @@ static struct rmsgpack_dom_value query_func_max( case RDT_INT: res.val.bool_ = ( (input.val.int_ == 0) - || (input.val.int_ > intermediate_res.val.int_) - || (intermediate_res.val.int_ == 0)); - if (res.val.bool_ || intermediate_res.val.int_ == 0) - memcpy(&intermediate_res, &input, sizeof(intermediate_res)); + || (input.val.int_ > ctx->intermediate_res.val.int_) + || (ctx->intermediate_res.val.int_ == 0)); + if (res.val.bool_ || ctx->intermediate_res.val.int_ == 0) + memcpy(&ctx->intermediate_res, &input, sizeof(ctx->intermediate_res)); break; case RDT_UINT: res.val.bool_ = ( (input.val.uint_ == 0) - || (input.val.uint_ > intermediate_res.val.uint_) - || (intermediate_res.val.uint_ == 0)); - if (res.val.bool_ || intermediate_res.val.uint_ == 0) - memcpy(&intermediate_res, &input, sizeof(intermediate_res)); + || (input.val.uint_ > ctx->intermediate_res.val.uint_) + || (ctx->intermediate_res.val.uint_ == 0)); + if (res.val.bool_ || ctx->intermediate_res.val.uint_ == 0) + memcpy(&ctx->intermediate_res, &input, sizeof(ctx->intermediate_res)); break; default: break; @@ -351,8 +389,11 @@ static void query_raise_unknown_function( { size_t remaining = len - off; size_t copy_len = (size_t)name_len < remaining ? (size_t)name_len : remaining - 1; - strlcpy(s + off, name, copy_len + 1); + /* See query_parse_table(): 'name' is a slice of the query, not + * a C string, so it cannot be handed to strlcpy(). */ + memcpy(s + off, name, copy_len); off += copy_len; + s[off] = '\0'; } if (off + 2 <= len) @@ -388,10 +429,11 @@ static struct buffer query_parse_integer( struct rmsgpack_dom_value *value, const char **err) { - int64_t result = 0; - int sign = 1; - bool has_digit = false; - size_t idx = buff.offset; + int64_t result = 0; + int sign = 1; + bool has_digit = false; + bool overflowed = false; + size_t idx = buff.offset; value->type = RDT_INT; @@ -405,11 +447,28 @@ static struct buffer query_parse_integer( while (idx < buff.len && ISDIGIT((int)buff.data[idx])) { + int digit = buff.data[idx] - '0'; has_digit = true; - result = result * 10 + (buff.data[idx] - '0'); + /* Signed overflow is undefined, and the digit count here comes + * from the query, so bound the accumulation rather than letting + * it wrap. */ + if ( result > (INT64_MAX - digit) / 10 + || overflowed) + overflowed = true; + else + result = result * 10 + digit; idx++; } + if (overflowed) + { + snprintf(s, len, + "%" PRIu64 "::Number out of range", + (uint64_t)buff.offset); + *err = s; + return buff; + } + if (!has_digit) { snprintf(s, len, @@ -525,7 +584,11 @@ static struct buffer query_parse_string( count = value->val.string.len + 1; if (is_binstr) - count /= 2; + count /= 2; + /* b"" sizes this to zero, and calloc(0) may return NULL, which + * the check below would report as an allocation failure. */ + if (count == 0) + count = 1; value->val.string.buff = (char*)calloc(count, sizeof(char)); if (!value->val.string.buff) @@ -597,7 +660,8 @@ static struct buffer query_parse_value(char *s, size_t len, || query_peek(buff, "\"", STRLEN_CONST("\"")) || query_peek(buff, "'", STRLEN_CONST("'"))) buff = query_parse_string(s, len, buff, value, err); - else if (ISDIGIT((int)buff.data[buff.offset])) + else if ( (size_t)buff.offset < buff.len + && ISDIGIT((int)buff.data[buff.offset])) buff = query_parse_integer(s, len, buff, value, err); return buff; } @@ -690,7 +754,8 @@ static struct buffer query_parse_argument( buff = query_chomp(buff); if ( - ISALPHA((int)buff.data[buff.offset]) + (size_t)buff.offset < buff.len + && ISALPHA((int)buff.data[buff.offset]) && !( query_peek(buff, "nil", STRLEN_CONST("nil")) || query_peek(buff, "true", STRLEN_CONST("true")) @@ -721,6 +786,7 @@ static struct buffer query_parse_argument( } static struct rmsgpack_dom_value query_func_all_map( + struct query_ctx *ctx, struct rmsgpack_dom_value input, unsigned argc, const struct argument *argv) { @@ -755,10 +821,11 @@ static struct rmsgpack_dom_value query_func_all_map( value = &nil_value; arg = argv[i + 1]; if (arg.type == AT_VALUE) - res = func_equals(*value, 1, &arg); + res = func_equals(ctx, *value, 1, &arg); else { - res = query_func_is_true(arg.a.invocation.func( + res = query_func_is_true(ctx, arg.a.invocation.func( + ctx, *value, arg.a.invocation.argc, arg.a.invocation.argv @@ -811,9 +878,13 @@ static struct buffer query_parse_table( *err = s; goto clean; } - strlcpy( - args[argi].a.value.val.string.buff, - ident_name, _len + 1); + /* strlcpy() would strlen() the source to compute its + * return value, and ident_name points into the query + * buffer, which is a (pointer, length) slice with no + * terminator of its own. Copy exactly the identifier. */ + memcpy(args[argi].a.value.val.string.buff, + ident_name, _len); + args[argi].a.value.val.string.buff[_len] = '\0'; } } else @@ -990,18 +1061,27 @@ void *libretrodb_query_compile(libretrodb_t *db, const char *query, size_t len, const char **err_string) { struct buffer buff; - /* TODO/FIXME - static local variable */ - static char tmp_err_buff [MAX_ERROR_LEN] = {0}; + /* Error text is formatted into storage owned by the db handle. + * This used to be a function-scope static, so the pointer returned + * through err_string was shared process-wide and two concurrent + * compiles clobbered each other's message - the second racy global + * in this file: + * + * WARNING: ThreadSanitizer: data race + * Location is global 'tmp_err_buff.0' of size 256 + * + * When there is no db handle to borrow from, report a fixed + * message rather than writing anywhere. */ + size_t err_buff_len = 0; + char *tmp_err_buff = libretrodb_query_err_buf(db, &err_buff_len); struct query *q = (struct query*)malloc(sizeof(*q)); - size_t err_buff_len = sizeof(tmp_err_buff); - - intermediate_res.val.int_ = 0; - intermediate_res.val.uint_ = 0; if (!q) return NULL; q->ref_count = 1; + q->ctx.intermediate_res.val.int_ = 0; + q->ctx.intermediate_res.val.uint_ = 0; q->root.argc = 0; q->root.func = NULL; q->root.argv = NULL; @@ -1011,6 +1091,16 @@ void *libretrodb_query_compile(libretrodb_t *db, buff.offset = 0; *err_string = NULL; + /* Every in-tree caller compiles against an open handle, and the + * parse helpers below format diagnostics straight into this + * buffer. Fail cleanly rather than teaching ten snprintf() sites + * to tolerate a NULL destination. */ + if (!tmp_err_buff) + { + *err_string = "No database handle"; + goto error; + } + buff = query_chomp(buff); if (query_peek(buff, "{", STRLEN_CONST("{"))) @@ -1020,7 +1110,8 @@ void *libretrodb_query_compile(libretrodb_t *db, if (*err_string) goto error; } - else if (ISALPHA((int)buff.data[buff.offset])) + else if ( (size_t)buff.offset < buff.len + && ISALPHA((int)buff.data[buff.offset])) buff = query_parse_method_call(tmp_err_buff, err_buff_len, buff, &q->root, err_string); @@ -1034,11 +1125,16 @@ void *libretrodb_query_compile(libretrodb_t *db, if (!q->root.func) { - snprintf(tmp_err_buff, err_buff_len, - "%" PRIu64 "::Unexpected EOF", - (uint64_t)buff.offset - ); - *err_string = tmp_err_buff; + if (tmp_err_buff) + { + snprintf(tmp_err_buff, err_buff_len, + "%" PRIu64 "::Unexpected EOF", + (uint64_t)buff.offset + ); + *err_string = tmp_err_buff; + } + else + *err_string = "Unexpected EOF"; goto error; } @@ -1060,8 +1156,9 @@ void libretrodb_query_inc_ref(libretrodb_query_t *q) int libretrodb_query_filter(libretrodb_query_t *q, struct rmsgpack_dom_value *v) { - struct invocation inv = ((struct query *)q)->root; - struct rmsgpack_dom_value res = inv.func(*v, inv.argc, inv.argv); + struct query *rq = (struct query *)q; + struct invocation inv = rq->root; + struct rmsgpack_dom_value res = inv.func(&rq->ctx, *v, inv.argc, inv.argv); return (res.type == RDT_BOOL && res.val.bool_); } @@ -1174,13 +1271,14 @@ int libretrodb_query_eval_field(libretrodb_query_t *q, /* Evaluate the matcher against the value */ if (val_arg->type == AT_VALUE) { - struct rmsgpack_dom_value res = func_equals(*value, 1, val_arg); + struct rmsgpack_dom_value res = func_equals(&rq->ctx, *value, 1, val_arg); return res.val.bool_ ? 1 : 0; } else { - struct rmsgpack_dom_value res = query_func_is_true( + struct rmsgpack_dom_value res = query_func_is_true(&rq->ctx, val_arg->a.invocation.func( + &rq->ctx, *value, val_arg->a.invocation.argc, val_arg->a.invocation.argv), diff --git a/libretro-db/query.h b/libretro-db/query.h index df30f2579fd0..23f768f8bc23 100644 --- a/libretro-db/query.h +++ b/libretro-db/query.h @@ -34,6 +34,10 @@ typedef struct libretrodb_query libretrodb_query_t; void libretrodb_query_inc_ref(libretrodb_query_t *q); +/* Reset @q's min()/max() accumulator. Called when a cursor walk + * begins; see the comment on struct query_ctx in query.c. */ +void libretrodb_query_reset_accumulator(libretrodb_query_t *q); + void libretrodb_query_dec_ref(libretrodb_query_t *q); int libretrodb_query_filter(libretrodb_query_t *q, struct rmsgpack_dom_value *v); diff --git a/libretro-db/rmsgpack.c b/libretro-db/rmsgpack.c index eb9227415fd8..eea9de797459 100644 --- a/libretro-db/rmsgpack.c +++ b/libretro-db/rmsgpack.c @@ -72,6 +72,29 @@ static const uint8_t MPF_FIXARRAY = _MPF_FIXARRAY; static const uint8_t MPF_FIXSTR = _MPF_FIXSTR; static const uint8_t MPF_NIL = _MPF_NIL; +/* Maximum container nesting accepted from a stream. + * + * The readers below recurse once per level of map/array nesting and + * had no limit, while the "len > remaining bytes" guards in + * rmsgpack_read_map()/rmsgpack_read_array() bound only how *wide* a + * container may be. Depth costs one byte per level: a run of 0x91 + * (fixarray of one element) recurses once per byte, so a 200 KB .rdb + * of 0x91 exhausts the stack: + * + * AddressSanitizer: stack-overflow + * rmsgpack_read rmsgpack.c:530 + * rmsgpack_read_array rmsgpack.c:513 + * ... repeated to exhaustion + * + * The DOM reader's own MAX_DEPTH does not catch this - a one-element + * array pops one entry and pushes one, so its stack index never + * grows. + * + * Records written by the converter nest two deep (a map of scalars); + * 32 leaves room for any plausible schema change while keeping worst + * case recursion bounded. */ +#define RMSGPACK_MAX_DEPTH 32 + int rmsgpack_write_array_header(intfstream_t *fd, uint32_t size) { uint8_t buf[5]; @@ -333,11 +356,40 @@ int rmsgpack_write_uint(intfstream_t *fd, uint64_t value) return (int)len; } +/** + * rmsgpack_read_exact: + * + * intfstream_read() reports a hard error as -1, but signals + * end-of-file as a zero-length or short read. Every length in a + * MsgPack stream is exact - a type byte is one byte, a declared + * payload is that many bytes - so a short read is a truncated or + * malformed stream and never a successful parse. + * + * Checking only for -1 (as every read site here used to) makes the + * reader treat EOF as success: rmsgpack_read() would leave its + * zero-initialised type byte untouched, take the 'positive fixint' + * branch and hand the caller a fabricated integer 0, forever. A + * .rdb whose nil sentinel is missing - a truncated download is + * enough - then spins libretrodb_cursor_read_item() without bound + * and the scanner task never completes. + * + * Returns: 0 when @len bytes were read, -1 otherwise. + */ +static int rmsgpack_read_exact(intfstream_t *fd, void *s, size_t len) +{ + if (len == 0) + return 0; + if (intfstream_read(fd, s, len) != (int64_t)len) + return -1; + return 0; +} + int rmsgpack_read_uint(intfstream_t *fd, uint64_t *s, size_t len) { union { uint64_t u64; uint32_t u32; uint16_t u16; uint8_t u8; } tmp; - if (intfstream_read(fd, &tmp, len) == -1) + tmp.u64 = 0; + if (rmsgpack_read_exact(fd, &tmp, len) == -1) return -1; switch (len) @@ -362,7 +414,8 @@ static int rmsgpack_read_int(intfstream_t *fd, int64_t *s, size_t len) { union { uint64_t u64; uint32_t u32; uint16_t u16; uint8_t u8; } tmp; - if (intfstream_read(fd, &tmp, len) == -1) + tmp.u64 = 0; + if (rmsgpack_read_exact(fd, &tmp, len) == -1) return -1; switch (len) @@ -426,22 +479,27 @@ static int rmsgpack_read_buff(intfstream_t *fd, size_t size, char **pbuff, uint6 if (!*pbuff) return -1; - if ((read_len = intfstream_read(fd, *pbuff, (size_t)tmp_len)) == -1) + if (rmsgpack_read_exact(fd, *pbuff, (size_t)tmp_len) == -1) { free(*pbuff); *pbuff = NULL; return -1; } - *len = read_len; + read_len = (ssize_t)tmp_len; + *len = (uint64_t)read_len; (*pbuff)[read_len] = 0; - /* Throw warning on read_len != tmp_len ? */ return 0; } +static int rmsgpack_read_depth(intfstream_t *fd, + struct rmsgpack_read_callbacks *callbacks, void *data, + unsigned depth); + static int rmsgpack_read_map(intfstream_t *fd, uint32_t len, - struct rmsgpack_read_callbacks *callbacks, void *data) + struct rmsgpack_read_callbacks *callbacks, void *data, + unsigned depth) { int rv; unsigned i; @@ -475,9 +533,9 @@ static int rmsgpack_read_map(intfstream_t *fd, uint32_t len, for (i = 0; i < len; i++) { - if ((rv = rmsgpack_read(fd, callbacks, data)) < 0) + if ((rv = rmsgpack_read_depth(fd, callbacks, data, depth)) < 0) return rv; - if ((rv = rmsgpack_read(fd, callbacks, data)) < 0) + if ((rv = rmsgpack_read_depth(fd, callbacks, data, depth)) < 0) return rv; } @@ -485,7 +543,8 @@ static int rmsgpack_read_map(intfstream_t *fd, uint32_t len, } static int rmsgpack_read_array(intfstream_t *fd, uint32_t len, - struct rmsgpack_read_callbacks *callbacks, void *data) + struct rmsgpack_read_callbacks *callbacks, void *data, + unsigned depth) { int rv; unsigned i; @@ -510,7 +569,7 @@ static int rmsgpack_read_array(intfstream_t *fd, uint32_t len, for (i = 0; i < len; i++) { - if ((rv = rmsgpack_read(fd, callbacks, data)) < 0) + if ((rv = rmsgpack_read_depth(fd, callbacks, data, depth)) < 0) return rv; } @@ -519,6 +578,13 @@ static int rmsgpack_read_array(intfstream_t *fd, uint32_t len, int rmsgpack_read(intfstream_t *fd, struct rmsgpack_read_callbacks *callbacks, void *data) +{ + return rmsgpack_read_depth(fd, callbacks, data, 0); +} + +static int rmsgpack_read_depth(intfstream_t *fd, + struct rmsgpack_read_callbacks *callbacks, void *data, + unsigned depth) { int rv; uint64_t tmp_len = 0; @@ -527,7 +593,11 @@ int rmsgpack_read(intfstream_t *fd, uint8_t type = 0; char *buff = NULL; - if (intfstream_read(fd, &type, sizeof(uint8_t)) == -1) + if (depth >= RMSGPACK_MAX_DEPTH) + return -1; + depth++; + + if (rmsgpack_read_exact(fd, &type, sizeof(uint8_t)) == -1) return -1; if (type < MPF_FIXMAP) @@ -539,12 +609,12 @@ int rmsgpack_read(intfstream_t *fd, else if (type < MPF_FIXARRAY) { tmp_len = type - MPF_FIXMAP; - return rmsgpack_read_map(fd, (uint32_t)tmp_len, callbacks, data); + return rmsgpack_read_map(fd, (uint32_t)tmp_len, callbacks, data, depth); } else if (type < MPF_FIXSTR) { tmp_len = type - MPF_FIXARRAY; - return rmsgpack_read_array(fd, (uint32_t)tmp_len, callbacks, data); + return rmsgpack_read_array(fd, (uint32_t)tmp_len, callbacks, data, depth); } else if (type < MPF_NIL) { @@ -552,11 +622,12 @@ int rmsgpack_read(intfstream_t *fd, tmp_len = type - MPF_FIXSTR; if (!(buff = (char *)malloc((size_t)(tmp_len + 1) * sizeof(char)))) return -1; - if ((_len = intfstream_read(fd, buff, (ssize_t)tmp_len)) == -1) + if (rmsgpack_read_exact(fd, buff, (size_t)tmp_len) == -1) { free(buff); return -1; } + _len = (ssize_t)tmp_len; buff[_len] = '\0'; if (callbacks->read_string) return callbacks->read_string(buff, (uint32_t)_len, data); @@ -627,12 +698,12 @@ int rmsgpack_read(intfstream_t *fd, case _MPF_ARRAY16: case _MPF_ARRAY32: if (rmsgpack_read_uint(fd, &tmp_len, 2<<(type - _MPF_ARRAY16)) != -1) - return rmsgpack_read_array(fd, (uint32_t)tmp_len, callbacks, data); + return rmsgpack_read_array(fd, (uint32_t)tmp_len, callbacks, data, depth); return -1; case _MPF_MAP16: case _MPF_MAP32: if (rmsgpack_read_uint(fd, &tmp_len, 2<<(type - _MPF_MAP16)) != -1) - return rmsgpack_read_map(fd, (uint32_t)tmp_len, callbacks, data); + return rmsgpack_read_map(fd, (uint32_t)tmp_len, callbacks, data, depth); return -1; } @@ -655,7 +726,7 @@ static int rmsgpack_skip_bytes(intfstream_t *fd, uint64_t len) while (len > 0) { uint64_t chunk = len > sizeof(tmp) ? sizeof(tmp) : len; - if (intfstream_read(fd, tmp, (size_t)chunk) == -1) + if (rmsgpack_read_exact(fd, tmp, (size_t)chunk) == -1) return -1; len -= chunk; } @@ -672,13 +743,24 @@ static int rmsgpack_skip_bytes(intfstream_t *fd, uint64_t len) * * Returns: 0 on success, -1 on error. */ +static int rmsgpack_skip_value_depth(intfstream_t *fd, unsigned depth); + int rmsgpack_skip_value(intfstream_t *fd) +{ + return rmsgpack_skip_value_depth(fd, 0); +} + +static int rmsgpack_skip_value_depth(intfstream_t *fd, unsigned depth) { uint8_t type = 0; uint64_t len = 0; uint64_t i; - if (intfstream_read(fd, &type, 1) == -1) + if (depth >= RMSGPACK_MAX_DEPTH) + return -1; + depth++; + + if (rmsgpack_read_exact(fd, &type, 1) == -1) return -1; /* positive fixint (0x00..0x7f) — no payload */ @@ -690,7 +772,7 @@ int rmsgpack_skip_value(intfstream_t *fd) { len = type - _MPF_FIXMAP; for (i = 0; i < len * 2; i++) - if (rmsgpack_skip_value(fd) < 0) + if (rmsgpack_skip_value_depth(fd, depth) < 0) return -1; return 0; } @@ -700,7 +782,7 @@ int rmsgpack_skip_value(intfstream_t *fd) { len = type - _MPF_FIXARRAY; for (i = 0; i < len; i++) - if (rmsgpack_skip_value(fd) < 0) + if (rmsgpack_skip_value_depth(fd, depth) < 0) return -1; return 0; } @@ -747,23 +829,23 @@ int rmsgpack_skip_value(intfstream_t *fd) case _MPF_ARRAY16: if (rmsgpack_read_uint(fd, &len, 2) == -1) return -1; for (i = 0; i < len; i++) - if (rmsgpack_skip_value(fd) < 0) return -1; + if (rmsgpack_skip_value_depth(fd, depth) < 0) return -1; return 0; case _MPF_ARRAY32: if (rmsgpack_read_uint(fd, &len, 4) == -1) return -1; for (i = 0; i < len; i++) - if (rmsgpack_skip_value(fd) < 0) return -1; + if (rmsgpack_skip_value_depth(fd, depth) < 0) return -1; return 0; case _MPF_MAP16: if (rmsgpack_read_uint(fd, &len, 2) == -1) return -1; for (i = 0; i < len * 2; i++) - if (rmsgpack_skip_value(fd) < 0) return -1; + if (rmsgpack_skip_value_depth(fd, depth) < 0) return -1; return 0; case _MPF_MAP32: if (rmsgpack_read_uint(fd, &len, 4) == -1) return -1; for (i = 0; i < len * 2; i++) - if (rmsgpack_skip_value(fd) < 0) return -1; + if (rmsgpack_skip_value_depth(fd, depth) < 0) return -1; return 0; } diff --git a/libretro-db/rmsgpack_dom.c b/libretro-db/rmsgpack_dom.c index 95a7b1f7eb1d..ebc19ea20135 100644 --- a/libretro-db/rmsgpack_dom.c +++ b/libretro-db/rmsgpack_dom.c @@ -152,14 +152,37 @@ static int dom_read_map_start(uint32_t len, void *data) struct rmsgpack_dom_value *v = rmsgpack_dom_reader_state_pop(dom_state); v->type = RDT_MAP; - v->val.map.len = len; + v->val.map.len = 0; v->val.map.items = NULL; + /* An empty map is legal MsgPack, and calloc(0, n) is permitted to + * return NULL, which the old code could not tell from failure. */ + if (len == 0) + return 0; + + /* 'len' is only published once storage backs it. It used to be + * assigned before the calloc(), so a failed allocation left the + * value as an RDT_MAP of len pairs with a NULL items pointer. The + * error then propagates to rmsgpack_dom_read_with(), whose cleanup + * calls rmsgpack_dom_value_free() - which walks items[0..len) and + * dereferences NULL. + * + * The allocation is attacker-influenced: len is bounded only by + * half the bytes remaining in the file, so a large .rdb with a + * MAP32 header sized to exceed the process' remaining memory + * reaches it. Reproduced with a 2 MB .rdb claiming 1000000 pairs + * under a constrained allocator: + * + * 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 */ if (!(items = (struct rmsgpack_dom_pair *) calloc(len, sizeof(struct rmsgpack_dom_pair)))) return -1; v->val.map.items = items; + v->val.map.len = len; for (i = 0; i < len; i++) { @@ -180,14 +203,21 @@ static int dom_read_array_start(uint32_t len, void *data) struct rmsgpack_dom_value *items = NULL; v->type = RDT_ARRAY; - v->val.array.len = len; + v->val.array.len = 0; v->val.array.items = NULL; + /* See dom_read_map_start(): empty arrays are legal, and 'len' must + * not be published until storage backs it or the cleanup path + * walks a NULL items pointer. */ + if (len == 0) + return 0; + if (!(items = (struct rmsgpack_dom_value *) calloc(len, sizeof(*items)))) return -1; v->val.array.items = items; + v->val.array.len = len; for (i = 0; i < len; i++) { @@ -205,9 +235,13 @@ void rmsgpack_dom_value_free(struct rmsgpack_dom_value *v) { case RDT_STRING: free(v->val.string.buff); + v->val.string.buff = NULL; + v->val.string.len = 0; break; case RDT_BINARY: free(v->val.binary.buff); + v->val.binary.buff = NULL; + v->val.binary.len = 0; break; case RDT_MAP: for (i = 0; i < v->val.map.len; i++) @@ -216,11 +250,15 @@ void rmsgpack_dom_value_free(struct rmsgpack_dom_value *v) rmsgpack_dom_value_free(&v->val.map.items[i].value); } free(v->val.map.items); + v->val.map.items = NULL; + v->val.map.len = 0; break; case RDT_ARRAY: for (i = 0; i < v->val.array.len; i++) rmsgpack_dom_value_free(&v->val.array.items[i]); free(v->val.array.items); + v->val.array.items = NULL; + v->val.array.len = 0; break; case RDT_NULL: case RDT_INT: @@ -229,6 +267,13 @@ void rmsgpack_dom_value_free(struct rmsgpack_dom_value *v) /* Do nothing */ break; } + + /* Leave the value in the same state a fresh RDT_NULL has, so that + * a second free of the same object - which the reader below can + * ask for when a record fails to parse - is a no-op rather than a + * double free. libretrodb_create() already open-coded this + * assignment after every call; it belongs here. */ + v->type = RDT_NULL; } struct rmsgpack_dom_value *rmsgpack_dom_value_map_value( @@ -432,6 +477,26 @@ static struct rmsgpack_read_callbacks dom_reader_callbacks = { int rmsgpack_dom_read_with(intfstream_t *fd, struct rmsgpack_dom_value *out, struct rmsgpack_dom_reader_state *s) { int rv; + + /* 'out' is uninitialised on entry - 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 it once a callback fires, so a stream that fails before + * the first callback left the error path below freeing whatever + * happened to be in 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 was already freed. Reading a truncated .rdb + * therefore ran rmsgpack_dom_value_free() over a dangling map - + * a use-after-free walking freed pairs, then a double free of + * the pair array itself. + * + * Claim the object before handing it to the reader so the error + * path always has a well-defined value to release. */ + out->type = RDT_NULL; + s->i = 0; s->stack[0] = out; if ((rv = rmsgpack_read(fd, &dom_reader_callbacks, s)) < 0) @@ -486,11 +551,52 @@ int rmsgpack_dom_read(intfstream_t *fd, struct rmsgpack_dom_value *out) return rmsgpack_dom_read_with(fd, out, &s); } +/** + * rmsgpack_dom_read_into: + * + * Read a map from @fd and extract the requested keys into + * caller-supplied storage. Varargs are triples of + * + * const char *key, enum rmsgpack_dom_field_type type, + * + * terminated by a NULL key. RDF_UINT / RDF_INT / RDF_BOOL take one + * output pointer; RDF_STRING / RDF_BINARY take a buffer plus a + * uint64_t* holding the buffer capacity on entry and the number of + * bytes written on exit. + * + * The type argument is what makes this safe. This function used to + * switch on the type tag found in the *file* and consume a different + * number of va_args per case - one for RDT_UINT, two for RDT_STRING + * and RDT_BINARY. A .rdb that declared a key with an unexpected + * type therefore slid the whole va_list out of step, and every + * subsequent key wrote a file-controlled uint64 through a pointer + * belonging to a different field. libretrodb_find_index() passes + * five outputs, so the desync there is an arbitrary write rather + * than merely a crash. + * + * A missing key was equally unsafe: rmsgpack_dom_value_map_value() + * returns NULL and the result went straight into "switch + * (value->type)". Reproduced with a 26-byte .rdb whose metadata map + * is keyed "kount" instead of "count": + * + * rmsgpack_dom.c:526: member access within null pointer + * #1 libretrodb_open libretrodb.c:248 + * + * Reading the caller's expectation first fixes both: the va_list + * advances by a fixed amount per key regardless of what the file + * says, an absent key leaves the output untouched, and a key whose + * stored type disagrees with the requested one is a parse failure. + * + * Returns: 0 on success, -1 if the stream is not a map, if a + * requested key is absent, or if a key's stored type does not match + * the requested type. + */ int rmsgpack_dom_read_into(intfstream_t *fd, ...) { - int rv; + int rv = 0; va_list ap; struct rmsgpack_dom_value map; + struct rmsgpack_dom_value key; va_start(ap, fd); @@ -500,79 +606,138 @@ int rmsgpack_dom_read_into(intfstream_t *fd, ...) return rv; } - if (map.type == RDT_MAP) + if (map.type != RDT_MAP) { - int *bool_value; - char *buff_value; - uint64_t min_len; - int64_t *int_value; - uint64_t *uint_value; - struct rmsgpack_dom_value key; - - for (;;) - { - struct rmsgpack_dom_value *value; - const char *key_name = va_arg(ap, const char *); + va_end(ap); + rmsgpack_dom_value_free(&map); + return -1; + } - if (!key_name) - break; + for (;;) + { + struct rmsgpack_dom_value *value; + enum rmsgpack_dom_field_type ftype; + char *buff_value; + uint64_t *uint_value; + uint64_t capacity; + uint64_t min_len; + const char *key_name = va_arg(ap, const char *); + + if (!key_name) + break; - key.type = RDT_STRING; - key.val.string.len = (uint32_t)strlen(key_name); - key.val.string.buff = (char *)key_name; + /* Read the caller's expected type before touching the file's, + * so the number of va_args consumed for this key is fixed. */ + ftype = (enum rmsgpack_dom_field_type) + va_arg(ap, int); - value = rmsgpack_dom_value_map_value(&map, &key); + key.type = RDT_STRING; + key.val.string.len = (uint32_t)strlen(key_name); + key.val.string.buff = (char *)key_name; - switch (value->type) - { - case RDT_INT: - int_value = va_arg(ap, int64_t *); - *int_value = value->val.int_; - break; - case RDT_BOOL: - bool_value = va_arg(ap, int *); - *bool_value = value->val.bool_; - break; - case RDT_UINT: - uint_value = va_arg(ap, uint64_t *); - *uint_value = value->val.uint_; - break; - case RDT_BINARY: - buff_value = va_arg(ap, char *); - uint_value = va_arg(ap, uint64_t *); - /* *uint_value is the caller's buffer capacity on entry - * and the number of bytes copied on exit. Compute the - * clamped copy length BEFORE overwriting *uint_value, - * otherwise the bound check collapses to len > len and - * the memcpy can overrun the caller's buffer with - * attacker-controlled length from the db file. */ - min_len = (value->val.binary.len > *uint_value) ? - *uint_value : value->val.binary.len; - *uint_value = min_len; - - memcpy(buff_value, value->val.binary.buff, (size_t)min_len); + value = rmsgpack_dom_value_map_value(&map, &key); + + switch (ftype) + { + case RDF_INT: + { + int64_t *int_value = va_arg(ap, int64_t *); + /* 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. Accept either spelling as long + * as the value is representable, so that requiring a + * type does not reject a validly encoded file. */ + if (value && value->type == RDT_INT) + *int_value = value->val.int_; + else if ( value + && value->type == RDT_UINT + && value->val.uint_ <= (uint64_t)INT64_MAX) + *int_value = (int64_t)value->val.uint_; + else + rv = -1; + } + break; + case RDF_BOOL: + { + int *bool_value = va_arg(ap, int *); + if (!value || value->type != RDT_BOOL) + { + rv = -1; + break; + } + *bool_value = value->val.bool_; + } + break; + case RDF_UINT: + { + uint64_t *u = va_arg(ap, uint64_t *); + /* See RDF_INT above: a non-negative RDT_INT is a valid + * encoding of a small unsigned value. */ + if (value && value->type == RDT_UINT) + *u = value->val.uint_; + else if ( value + && value->type == RDT_INT + && value->val.int_ >= 0) + *u = (uint64_t)value->val.int_; + else + rv = -1; + } + break; + case RDF_BINARY: + buff_value = va_arg(ap, char *); + uint_value = va_arg(ap, uint64_t *); + if (!value || value->type != RDT_BINARY) + { + rv = -1; break; - case RDT_STRING: - buff_value = va_arg(ap, char *); - uint_value = va_arg(ap, uint64_t *); - /* Cast to uint64_t before adding 1 to avoid uint32_t - * overflow when string.len == UINT32_MAX, which would - * wrap the sum to 0 and collapse the bounds check. */ - min_len = ((uint64_t)value->val.string.len + 1 > *uint_value) ? - *uint_value : (uint64_t)value->val.string.len + 1; - *uint_value = min_len; - - memcpy(buff_value, value->val.string.buff, (size_t)min_len); + } + /* *uint_value is the caller's buffer capacity on entry + * and the number of bytes copied on exit. Compute the + * clamped copy length BEFORE overwriting *uint_value, + * otherwise the bound check collapses to len > len and + * the memcpy can overrun the caller's buffer with + * attacker-controlled length from the db file. */ + capacity = *uint_value; + min_len = (value->val.binary.len > capacity) + ? capacity : value->val.binary.len; + *uint_value = min_len; + memcpy(buff_value, value->val.binary.buff, (size_t)min_len); + break; + case RDF_STRING: + buff_value = va_arg(ap, char *); + uint_value = va_arg(ap, uint64_t *); + if (!value || value->type != RDT_STRING) + { + rv = -1; break; - default: - va_end(ap); - rmsgpack_dom_value_free(&map); - return 0; - } + } + /* Cast to uint64_t before adding 1 to avoid uint32_t + * overflow when string.len == UINT32_MAX, which would + * wrap the sum to 0 and collapse the bounds check. */ + capacity = *uint_value; + min_len = ((uint64_t)value->val.string.len + 1 > capacity) + ? capacity : (uint64_t)value->val.string.len + 1; + *uint_value = min_len; + memcpy(buff_value, value->val.string.buff, (size_t)min_len); + /* memcpy above may have stopped short of the terminator + * when the stored string does not fit. Callers treat + * this as a C string (libretrodb_find_index() calls + * strlen() on idx->name), so terminate unconditionally + * rather than handing back 50 unterminated bytes. */ + if (min_len > 0) + buff_value[min_len - 1] = '\0'; + break; + default: + rv = -1; + break; } + + if (rv < 0) + break; } va_end(ap); rmsgpack_dom_value_free(&map); - return 0; + return rv; } diff --git a/libretro-db/rmsgpack_dom.h b/libretro-db/rmsgpack_dom.h index 539032f3b37f..53f41658612f 100644 --- a/libretro-db/rmsgpack_dom.h +++ b/libretro-db/rmsgpack_dom.h @@ -99,6 +99,21 @@ void rmsgpack_dom_reader_state_free(struct rmsgpack_dom_reader_state *state); int rmsgpack_dom_write(intfstream_t *stream, const struct rmsgpack_dom_value *obj); +/* Field descriptors for rmsgpack_dom_read_into(). The caller states + * the type it expects so that the number of varargs consumed for a + * key is fixed by the call site rather than by the file being read. + * See the comment above rmsgpack_dom_read_into() in rmsgpack_dom.c. */ +enum rmsgpack_dom_field_type +{ + RDF_UINT = 0, /* uint64_t *out */ + RDF_INT, /* int64_t *out */ + RDF_BOOL, /* int *out */ + RDF_STRING, /* char *buf, uint64_t *inout_capacity */ + RDF_BINARY /* void *buf, uint64_t *inout_capacity */ +}; + +/* varargs: (const char *key, enum rmsgpack_dom_field_type, ) + * repeated, terminated by a NULL key. */ int rmsgpack_dom_read_into(intfstream_t *stream, ...); RETRO_END_DECLS diff --git a/libretro-db/samples/libretrodb/Makefile b/libretro-db/samples/libretrodb/Makefile index 6117a6296c0f..b3dd0d3962ae 100644 --- a/libretro-db/samples/libretrodb/Makefile +++ b/libretro-db/samples/libretrodb/Makefile @@ -1,10 +1,9 @@ -TARGET := libretrodb_leak_test +TARGETS := libretrodb_leak_test libretrodb_parser_test LIBRETRODB_DIR := ../.. LIBRETRO_COMM_DIR := ../../../libretro-common -SOURCES := \ - libretrodb_leak_test.c \ +COMMON_SOURCES := \ $(LIBRETRODB_DIR)/libretrodb.c \ $(LIBRETRODB_DIR)/rmsgpack.c \ $(LIBRETRODB_DIR)/rmsgpack_dom.c \ @@ -25,7 +24,7 @@ SOURCES := \ $(LIBRETRO_COMM_DIR)/time/rtime.c \ $(LIBRETRO_COMM_DIR)/vfs/vfs_implementation.c -OBJS := $(SOURCES:.c=.o) +COMMON_OBJS := $(COMMON_SOURCES:.c=.o) CFLAGS += -Wall -pedantic -std=gnu99 -g \ -I$(LIBRETRO_COMM_DIR)/include \ @@ -41,15 +40,30 @@ ifneq ($(SANITIZER),) LDFLAGS := -fsanitize=$(SANITIZER) $(LDFLAGS) endif -all: $(TARGET) +all: $(TARGETS) %.o: %.c $(CC) -c -o $@ $< $(CFLAGS) -$(TARGET): $(OBJS) +libretrodb_leak_test: libretrodb_leak_test.o $(COMMON_OBJS) $(CC) -o $@ $^ $(LDFLAGS) +libretrodb_parser_test: libretrodb_parser_test.o $(COMMON_OBJS) + $(CC) -o $@ $^ $(LDFLAGS) + +# Run both under the sanitizers the tests are written for. +check: all + ./libretrodb_leak_test + ./libretrodb_parser_test + +# Build and run everything under ASan, UBSan, LeakSan and TSan. +# Set SWEEP_CORPUS to add the checks that need .rdb data too large +# to keep in the repository; see the header of sweep.sh. +sweep: + ./sweep.sh + clean: - rm -f $(TARGET) $(OBJS) + rm -f $(TARGETS) $(COMMON_OBJS) libretrodb_leak_test.o \ + libretrodb_parser_test.o -.PHONY: clean +.PHONY: clean check sweep diff --git a/libretro-db/samples/libretrodb/libretrodb_parser_test.c b/libretro-db/samples/libretrodb/libretrodb_parser_test.c new file mode 100644 index 000000000000..943102532b3d --- /dev/null +++ b/libretro-db/samples/libretrodb/libretrodb_parser_test.c @@ -0,0 +1,890 @@ +/* Regression test for the libretrodb reader: malformed-input + * handling and the sliding-window cursor stream. + * + * Every case here corresponds to a defect that was live at some + * point. A .rdb is content the frontend downloads, so the reader + * has to survive anything in that file rather than trusting it. + * + * missing metadata key rmsgpack_dom_read_into() fed the NULL + * from a failed map lookup straight into + * "switch (value->type)". + * metadata key mistyped the same function 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. + * deep nesting the readers recursed once per level of + * container nesting with no depth limit, + * so a run of 0x91 exhausted the stack. + * no nil sentinel intfstream_read() reports EOF 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. + * truncated record 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. + * empty containers a zero-length map or array is legal + * MsgPack, but calloc(0) may return NULL + * and that was read as failure. + * index headers binsearch() read before its count == 0 + * test, recursed without shrinking on a + * one-element range, and loaded the + * record offset with an unaligned cast; + * nothing cross-checked count against the + * payload the header reserved. + * window spanning the cursor walks through a fixed + * sliding buffer; records have to survive + * crossing its boundary. + * oversized field a field wider than the window cannot be + * served from it, and returning failure + * silently dropped the record. + * index round trip libretrodb_create_index() leaked every + * key buffer it handed to the tree, and + * bintree reported a failed grow as a + * successful insert; nothing exercised + * create-then-look-up at all. + * field scan libretrodb_scan_field() walks a database + * once reporting a field, a companion + * numeric field, and the offset of the + * record carrying them, and + * libretrodb_read_at() fetches a record + * back from such an offset. The two have + * to agree with what a query returns, + * including for keys that repeat. + * query slices libretrodb_query_compile() takes a + * (pointer, length) pair, but the parser + * handed identifier slices to strlcpy(), + * which strlen()s its source, and indexed + * buff.data[buff.offset] after chomping + * trailing space without checking the + * offset against the length. + * + * Self-contained: builds each .rdb in a temp directory, exercises + * it, and reports. Exits non-zero if any case regresses. + * + * Run with: make SANITIZER=address,undefined && ./libretrodb_parser_test + */ + +#include +#include +#include +#include + +#include "../../libretrodb.h" +#include "../../rmsgpack_dom.h" +#include "../../query.h" + +/* A record count no legitimate test file reaches; hitting it means + * the cursor is not terminating. */ +#define RUNAWAY_LIMIT 100000 + +static int failures = 0; +static int checks = 0; + +/* Printed before the case runs, so a crash still says which one. */ +static void begin(const char *what) +{ + printf(" .... %-34s", what); +} + +static void check(int ok, const char *what, const char *detail) +{ + checks++; + printf("\r"); + if (ok) + printf(" ok %-34s %s\n", what, detail ? detail : ""); + else + { + printf(" FAIL %-34s %s\n", what, detail ? detail : ""); + failures++; + } +} + +/* ------------------------------------------------------------------ + * Minimal MsgPack writers, so the test does not depend on the + * library it is testing to produce its inputs. + * ------------------------------------------------------------------ */ + +typedef struct +{ + uint8_t *data; + size_t len; + size_t cap; +} buf_t; + +static void bput(buf_t *b, const void *p, size_t n) +{ + if (b->len + n > b->cap) + { + size_t want = (b->cap ? b->cap * 2 : 256); + while (want < b->len + n) + want *= 2; + b->data = (uint8_t*)realloc(b->data, want); + b->cap = want; + } + memcpy(b->data + b->len, p, n); + b->len += n; +} + +static void bbyte(buf_t *b, uint8_t v) { bput(b, &v, 1); } +static void bfixmap(buf_t *b, unsigned n) { bbyte(b, (uint8_t)(0x80 | n)); } +static void bfixstr(buf_t *b, const char *s) +{ + size_t n = strlen(s); + bbyte(b, (uint8_t)(0xa0 | n)); + bput(b, s, n); +} +static void bstr32(buf_t *b, const void *s, uint32_t n) +{ + uint8_t h[5]; + h[0] = 0xdb; + h[1] = (uint8_t)(n >> 24); h[2] = (uint8_t)(n >> 16); + h[3] = (uint8_t)(n >> 8); h[4] = (uint8_t)n; + bput(b, h, 5); + bput(b, s, n); +} +static void bbin(buf_t *b, const void *s, uint8_t n) +{ + bbyte(b, 0xc4); + bbyte(b, n); + bput(b, s, n); +} +static void buint8(buf_t *b, uint8_t v) { bbyte(b, 0xcc); bbyte(b, v); } +static void bnil(buf_t *b) { bbyte(b, 0xc0); } + +/* Header is "RARCHDB\0" plus a big-endian metadata offset. */ +static int write_rdb(const char *path, const buf_t *body, const buf_t *meta) +{ + FILE *f; + uint8_t hdr[16]; + uint64_t off = 16 + (uint64_t)body->len; + int i; + + memcpy(hdr, "RARCHDB", 7); + hdr[7] = 0; + for (i = 0; i < 8; i++) + hdr[8 + i] = (uint8_t)(off >> (56 - 8 * i)); + + if (!(f = fopen(path, "wb"))) + return 0; + fwrite(hdr, 1, sizeof(hdr), f); + if (body->len) + fwrite(body->data, 1, body->len, f); + if (meta && meta->len) + fwrite(meta->data, 1, meta->len, f); + fclose(f); + return 1; +} + +static void meta_count(buf_t *m, uint8_t n) +{ + bfixmap(m, 1); + bfixstr(m, "count"); + buint8(m, n); +} + +static void bfree(buf_t *b) { free(b->data); memset(b, 0, sizeof(*b)); } + +/* ------------------------------------------------------------------ + * Harness + * ------------------------------------------------------------------ */ + +/* Walk a database. Returns record count, -1 if it could not be + * opened, -2 if the cursor would not open, -3 if it ran away. */ +static long walk(const char *path) +{ + libretrodb_t *db = libretrodb_new(); + libretrodb_cursor_t *cur = libretrodb_cursor_new(); + struct rmsgpack_dom_value item; + long n = 0; + long rv; + + if (!db || !cur) + return -1; + + if (libretrodb_open(path, db, false) != 0) + { + rv = -1; + goto done_nocursor; + } + if (libretrodb_cursor_open(db, cur, NULL) != 0) + { + rv = -2; + goto done_db; + } + + while (libretrodb_cursor_read_item(cur, &item) == 0) + { + rmsgpack_dom_value_free(&item); + if (++n > RUNAWAY_LIMIT) + { + rv = -3; + libretrodb_cursor_close(cur); + goto done_db; + } + } + libretrodb_cursor_close(cur); + rv = n; + +done_db: + libretrodb_close(db); +done_nocursor: + libretrodb_free(db); + libretrodb_cursor_free(cur); + return rv; +} + +/* Collect the "name" of every record, joined by '|', so a walk can + * be compared byte for byte against what was written. */ +static char *walk_names(const char *path) +{ + libretrodb_t *db = libretrodb_new(); + libretrodb_cursor_t *cur = libretrodb_cursor_new(); + struct rmsgpack_dom_value item; + buf_t out; + unsigned i; + + memset(&out, 0, sizeof(out)); + + if (!db || !cur || libretrodb_open(path, db, false) != 0) + { + libretrodb_free(db); + libretrodb_cursor_free(cur); + return NULL; + } + if (libretrodb_cursor_open(db, cur, NULL) != 0) + { + libretrodb_close(db); + libretrodb_free(db); + libretrodb_cursor_free(cur); + return NULL; + } + + while (libretrodb_cursor_read_item(cur, &item) == 0) + { + if (item.type == RDT_MAP) + { + for (i = 0; i < item.val.map.len; i++) + { + struct rmsgpack_dom_value *k = &item.val.map.items[i].key; + struct rmsgpack_dom_value *v = &item.val.map.items[i].value; + if ( k->type == RDT_STRING && k->val.string.buff + && !strcmp(k->val.string.buff, "name") + && v->type == RDT_STRING && v->val.string.buff) + { + bput(&out, v->val.string.buff, + strlen(v->val.string.buff)); + bbyte(&out, '|'); + } + } + } + rmsgpack_dom_value_free(&item); + } + bbyte(&out, 0); + + libretrodb_cursor_close(cur); + libretrodb_close(db); + libretrodb_free(db); + libretrodb_cursor_free(cur); + return (char*)out.data; +} + +/* ------------------------------------------------------------------ + * Cases + * ------------------------------------------------------------------ */ + +static void case_metadata(const char *dir) +{ + char path[512]; + buf_t body, meta; + + /* metadata map keyed "kount" instead of "count" */ + memset(&body, 0, sizeof(body)); memset(&meta, 0, sizeof(meta)); + bnil(&body); + bfixmap(&meta, 1); bfixstr(&meta, "kount"); buint8(&meta, 1); + sprintf(path, "%s/meta_missing_key.rdb", dir); + write_rdb(path, &body, &meta); + begin("metadata key absent"); + check(walk(path) == -1, "metadata key absent", "rejected, no NULL deref"); + bfree(&body); bfree(&meta); + + /* "count" present but stored as a string */ + memset(&body, 0, sizeof(body)); memset(&meta, 0, sizeof(meta)); + bnil(&body); + bfixmap(&meta, 1); bfixstr(&meta, "count"); bfixstr(&meta, "AAAAAAAA"); + sprintf(path, "%s/meta_wrong_type.rdb", dir); + write_rdb(path, &body, &meta); + begin("metadata key mistyped"); + check(walk(path) == -1, "metadata key mistyped", "rejected, va_list intact"); + bfree(&body); bfree(&meta); + + /* "count" as a positive fixint rather than uint8: a different but + * legal encoding, and must still be accepted */ + memset(&body, 0, sizeof(body)); memset(&meta, 0, sizeof(meta)); + bfixmap(&body, 1); bfixstr(&body, "name"); bfixstr(&body, "One"); + bnil(&body); + bfixmap(&meta, 1); bfixstr(&meta, "count"); bbyte(&meta, 1); + sprintf(path, "%s/meta_fixint.rdb", dir); + write_rdb(path, &body, &meta); + begin("metadata count as fixint"); + check(walk(path) == 1, "metadata count as fixint", "accepted"); + bfree(&body); bfree(&meta); +} + +static void case_nesting(const char *dir) +{ + char path[512]; + buf_t body, meta; + int i; + + /* shallow nesting stays legal */ + memset(&body, 0, sizeof(body)); memset(&meta, 0, sizeof(meta)); + for (i = 0; i < 8; i++) + bbyte(&body, 0x91); /* fixarray of one */ + bbyte(&body, 0x00); + bnil(&body); + meta_count(&meta, 1); + sprintf(path, "%s/nest_shallow.rdb", dir); + write_rdb(path, &body, &meta); + begin("nesting within the limit"); + check(walk(path) == 1, "nesting within the limit", "accepted"); + bfree(&body); bfree(&meta); + + /* a long run of container headers must be refused rather than + * recursed into until the stack runs out */ + memset(&body, 0, sizeof(body)); memset(&meta, 0, sizeof(meta)); + for (i = 0; i < 100000; i++) + bbyte(&body, 0x91); + bbyte(&body, 0x00); + bnil(&body); + meta_count(&meta, 1); + sprintf(path, "%s/nest_deep.rdb", dir); + write_rdb(path, &body, &meta); + begin("nesting past the limit"); + check(walk(path) == 0, "nesting past the limit", "refused, stack intact"); + bfree(&body); bfree(&meta); +} + +static void case_truncation(const char *dir) +{ + char path[512]; + buf_t body, meta; + + /* no trailing nil: the walk has to end at EOF, not spin */ + memset(&body, 0, sizeof(body)); memset(&meta, 0, sizeof(meta)); + bfixmap(&body, 1); bfixstr(&body, "name"); bfixstr(&body, "One"); + meta_count(&meta, 1); + sprintf(path, "%s/no_sentinel.rdb", dir); + write_rdb(path, &body, &meta); + begin("sentinel missing"); + check(walk(path) >= 0, "sentinel missing", "terminates at EOF"); + bfree(&body); bfree(&meta); + + /* a complete record followed by a bare map header: the second + * read fails, and the value it fails on is the one the previous + * iteration left behind */ + memset(&body, 0, sizeof(body)); memset(&meta, 0, sizeof(meta)); + bfixmap(&body, 1); bfixstr(&body, "name"); bfixstr(&body, "AAAAAAAAAAAAAAAA"); + bbyte(&body, 0x81); + meta_count(&meta, 2); + sprintf(path, "%s/trunc_after_record.rdb", dir); + write_rdb(path, &body, &meta); + begin("record truncated mid-map"); + check(walk(path) >= 0, "record truncated mid-map", "no use-after-free"); + bfree(&body); bfree(&meta); +} + +static void case_empty_containers(const char *dir) +{ + char path[512]; + buf_t body, meta; + + memset(&body, 0, sizeof(body)); memset(&meta, 0, sizeof(meta)); + bbyte(&body, 0x80); /* empty map */ + bfixmap(&body, 1); bfixstr(&body, "tags"); + bbyte(&body, 0x90); /* empty array */ + bfixmap(&body, 1); bfixstr(&body, "name"); bfixstr(&body, "Real"); + bnil(&body); + meta_count(&meta, 3); + sprintf(path, "%s/empty_containers.rdb", dir); + write_rdb(path, &body, &meta); + begin("empty map and array records"); + check(walk(path) == 3, "empty map and array records", "all three read"); + bfree(&body); bfree(&meta); +} + +/* The cursor reads through a fixed sliding buffer. Build a database + * comfortably larger than it, and one carrying a field wider than + * it, and require both to read back exactly. */ +static void case_window(const char *dir) +{ + char path[512]; + buf_t body, meta, expect; + char *got; + char name[64]; + int i; + const int records = 4000; + + memset(&body, 0, sizeof(body)); + memset(&meta, 0, sizeof(meta)); + memset(&expect, 0, sizeof(expect)); + + for (i = 0; i < records; i++) + { + sprintf(name, "Record %06d With Padding To Widen The Entry", i); + bfixmap(&body, 2); + bfixstr(&body, "name"); + bstr32(&body, name, (uint32_t)strlen(name)); + bfixstr(&body, "crc"); + bbin(&body, "\xde\xad\xbe\xef", 4); + bput(&expect, name, strlen(name)); + bbyte(&expect, '|'); + } + bnil(&body); + bbyte(&expect, 0); + meta_count(&meta, 1); + + sprintf(path, "%s/window_span.rdb", dir); + write_rdb(path, &body, &meta); + + begin("database spanning many windows"); + check(walk(path) == records, "database spanning many windows", + "record count matches"); + begin("records across window edges"); + got = walk_names(path); + check(got && !strcmp(got, (char*)expect.data), + "records across window edges", "names byte-identical"); + free(got); + bfree(&body); bfree(&meta); bfree(&expect); + + /* One field larger than any plausible window. Reading it needs a + * single call wider than the buffer; failing that call drops the + * record without any error reaching the caller. */ + { + char *big = (char*)malloc(200000); + memset(big, 'A', 200000); + memset(&body, 0, sizeof(body)); + memset(&meta, 0, sizeof(meta)); + bfixmap(&body, 2); + bfixstr(&body, "crc"); + bbin(&body, "\xde\xad\xbe\xef", 4); + bfixstr(&body, "name"); + bstr32(&body, big, 200000); + bnil(&body); + meta_count(&meta, 1); + sprintf(path, "%s/window_bigfield.rdb", dir); + write_rdb(path, &body, &meta); + begin("field wider than the window"); + check(walk(path) == 1, "field wider than the window", + "record still read"); + free(big); + bfree(&body); bfree(&meta); + } +} + +/* Index headers are three independent file-supplied numbers with no + * relationship enforced between them. */ +static void case_index(const char *dir) +{ + char path[512]; + buf_t body, meta, tail; + struct { const char *name; uint8_t key_size; uint8_t next; + uint8_t count; int payload; int expect_found; + const char *what; } cases[] = { + { "crc", 4, 12, 1, 12, 1, "index well formed" }, + { "crc", 4, 12, 200, 12, 0, "index count exceeds payload" }, + { "crc", 4, 0, 50, 0, 0, "index payload empty" }, + { "crc", 0, 64, 8, 64, 0, "index key_size zero" }, + { "zzz", 4, 0, 1, 0, 0, "index chain does not advance" } + }; + unsigned c; + + for (c = 0; c < sizeof(cases) / sizeof(cases[0]); c++) + { + libretrodb_t *db; + struct rmsgpack_dom_value out; + unsigned char key[4]; + int found; + + key[0] = 0xde; key[1] = 0xad; key[2] = 0xbe; key[3] = 0xef; + + memset(&body, 0, sizeof(body)); + memset(&meta, 0, sizeof(meta)); + memset(&tail, 0, sizeof(tail)); + + bfixmap(&body, 2); + bfixstr(&body, "name"); bfixstr(&body, "Rec"); + bfixstr(&body, "crc"); bbin(&body, "\xde\xad\xbe\xef", 4); + bnil(&body); + meta_count(&meta, 1); + + /* index header, then its payload */ + bfixmap(&tail, 4); + bfixstr(&tail, "name"); bfixstr(&tail, cases[c].name); + bfixstr(&tail, "key_size"); buint8(&tail, cases[c].key_size); + bfixstr(&tail, "next"); buint8(&tail, cases[c].next); + bfixstr(&tail, "count"); buint8(&tail, cases[c].count); + if (cases[c].payload) + { + int j; + bput(&tail, "\xde\xad\xbe\xef", 4); + for (j = 0; j < 8; j++) + bbyte(&tail, j == 0 ? 16 : 0); + for (j = 12; j < cases[c].payload; j++) + bbyte(&tail, 0); + } + /* metadata and the index chain both live past the records */ + bput(&meta, tail.data, tail.len); + + sprintf(path, "%s/index_%u.rdb", dir, c); + write_rdb(path, &body, &meta); + + begin(cases[c].what); + db = libretrodb_new(); + found = 0; + if (db && libretrodb_open(path, db, false) == 0) + { + if (libretrodb_find_entry(db, "crc", key, &out) == 0) + { + found = 1; + rmsgpack_dom_value_free(&out); + } + libretrodb_close(db); + } + libretrodb_free(db); + + check(found == cases[c].expect_found, cases[c].what, + cases[c].expect_found ? "entry located" : "rejected cleanly"); + + bfree(&body); bfree(&meta); bfree(&tail); + } +} + +/* libretrodb_query_compile() is documented by its signature to take + * a pointer and a length. Feed it exactly-sized heap buffers with no + * terminator so that any read past the length is a heap overflow the + * sanitizer can see, rather than landing on a NUL that happens to be + * there. */ +static void case_query_slices(const char *dir) +{ + static const struct + { + const char *text; + const char *what; + } queries[] = { + { "{crc:or(b\"DEADBEEF\",b\"00000000\")}", "query with identifiers" }, + { "{'serial': b'414243'}", "query with binary string" }, + { "{'a':", "query ending at a value" }, + { "{'a': ", "query ending after space" }, + { " ", "query that is all space" }, + { "{crc:99999999999999999999999}", "query with huge integer" }, + { "{'a': b\"\"}", "query with empty binary" }, + { "{nosuchfunction(1)}", "query naming unknown func" } + }; + char path[512]; + buf_t body, meta; + unsigned i; + libretrodb_t *db = libretrodb_new(); + + memset(&body, 0, sizeof(body)); + memset(&meta, 0, sizeof(meta)); + bfixmap(&body, 1); bfixstr(&body, "name"); bfixstr(&body, "One"); + bnil(&body); + meta_count(&meta, 1); + sprintf(path, "%s/query_host.rdb", dir); + write_rdb(path, &body, &meta); + bfree(&body); bfree(&meta); + + if (!db || libretrodb_open(path, db, false) != 0) + { + check(0, "query host database", "could not be opened"); + libretrodb_free(db); + return; + } + + for (i = 0; i < sizeof(queries) / sizeof(queries[0]); i++) + { + size_t n = strlen(queries[i].text); + char *exact = (char*)malloc(n ? n : 1); + const char *err = NULL; + void *q; + + memcpy(exact, queries[i].text, n); + begin(queries[i].what); + q = libretrodb_query_compile(db, exact, n, &err); + /* Compiling or rejecting are both fine; reading out of bounds + * while deciding is not, and that is what the sanitizer sees. */ + check(1, queries[i].what, (q && !err) ? "compiled" : "rejected"); + if (q && !err) + libretrodb_query_free((libretrodb_query_t*)q); + free(exact); + } + + libretrodb_close(db); + libretrodb_free(db); +} + +/* Build an index over a database with unique keys, then look an + * entry up through it. This is the only path that reaches + * bintree_insert() and binsearch(), and until now nothing exercised + * the two together. */ +static void case_index_round_trip(const char *dir) +{ + char path[512]; + buf_t body, meta; + int i; + const int records = 600; + libretrodb_t *db; + + memset(&body, 0, sizeof(body)); + memset(&meta, 0, sizeof(meta)); + + for (i = 0; i < records; i++) + { + char name[32]; + uint8_t crc[4]; + sprintf(name, "G%05d", i); + crc[0] = (uint8_t)(i >> 24); crc[1] = (uint8_t)(i >> 16); + crc[2] = (uint8_t)(i >> 8); crc[3] = (uint8_t)i; + bfixmap(&body, 2); + bfixstr(&body, "name"); bfixstr(&body, name); + bfixstr(&body, "crc"); bbin(&body, crc, 4); + } + bnil(&body); + meta_count(&meta, 1); + + sprintf(path, "%s/index_round_trip.rdb", dir); + write_rdb(path, &body, &meta); + bfree(&body); bfree(&meta); + + begin("index creation"); + db = libretrodb_new(); + if (!db || libretrodb_open(path, db, true) != 0) + { + check(0, "index creation", "database would not open for write"); + libretrodb_free(db); + return; + } + check(libretrodb_create_index(db, "crc", "crc") == 0, + "index creation", "built over unique keys"); + libretrodb_close(db); + libretrodb_free(db); + + /* Look up a key that is neither first nor last, so the search has + * to actually descend. */ + begin("index lookup"); + db = libretrodb_new(); + if (db && libretrodb_open(path, db, false) == 0) + { + struct rmsgpack_dom_value out; + unsigned char key[4]; + unsigned v = 337; + char found[64]; + int ok = 0; + key[0] = (unsigned char)(v >> 24); key[1] = (unsigned char)(v >> 16); + key[2] = (unsigned char)(v >> 8); key[3] = (unsigned char)v; + found[0] = '\0'; + if (libretrodb_find_entry(db, "crc", key, &out) == 0) + { + if (out.type == RDT_MAP) + { + unsigned j; + for (j = 0; j < out.val.map.len; j++) + { + struct rmsgpack_dom_value *k = &out.val.map.items[j].key; + struct rmsgpack_dom_value *w = &out.val.map.items[j].value; + if ( k->type == RDT_STRING && k->val.string.buff + && !strcmp(k->val.string.buff, "name") + && w->type == RDT_STRING && w->val.string.buff) + strncpy(found, w->val.string.buff, sizeof(found) - 1); + } + } + ok = !strcmp(found, "G00337"); + rmsgpack_dom_value_free(&out); + } + check(ok, "index lookup", ok ? "resolved to the right record" + : "wrong or missing record"); + libretrodb_close(db); + } + else + check(0, "index lookup", "database would not reopen"); + libretrodb_free(db); +} + +/* Collector for the scan test below. */ +typedef struct +{ + uint32_t key; + uint64_t off; + uint64_t aux; + int have_aux; +} scan_ent_t; +static scan_ent_t scan_ents[64]; +static size_t scan_n; + +static int scan_collect(void *ctx, const uint8_t *key, size_t key_len, + uint64_t offset, const uint64_t *aux) +{ + (void)ctx; + if (key_len != 4 || scan_n >= sizeof(scan_ents) / sizeof(scan_ents[0])) + return 0; + scan_ents[scan_n].have_aux = (aux != NULL); + scan_ents[scan_n].aux = aux ? *aux : 0; + scan_ents[scan_n].key = ((uint32_t)key[0] << 24) | ((uint32_t)key[1] << 16) + | ((uint32_t)key[2] << 8) | (uint32_t)key[3]; + scan_ents[scan_n].off = offset; + scan_n++; + return 0; +} + +/* Walk a database with libretrodb_scan_field(), then read each record + * back by offset. Includes a repeated key, because the scanner + * iterates every record sharing a crc and the offsets have to + * distinguish them. */ +static void case_field_scan(const char *dir) +{ + char path[512]; + buf_t body, meta; + size_t i; + int ok; + libretrodb_t *db; + /* Third and fourth records deliberately share a key. */ + static const uint32_t keys[] = { 0x11111111u, 0x22222222u, + 0x33333333u, 0x33333333u, + 0x44444444u }; + const size_t nkeys = sizeof(keys) / sizeof(keys[0]); + + memset(&body, 0, sizeof(body)); + memset(&meta, 0, sizeof(meta)); + + for (i = 0; i < nkeys; i++) + { + char name[32]; + uint8_t crc[4]; + sprintf(name, "Rec%02u", (unsigned)i); + crc[0] = (uint8_t)(keys[i] >> 24); crc[1] = (uint8_t)(keys[i] >> 16); + crc[2] = (uint8_t)(keys[i] >> 8); crc[3] = (uint8_t)keys[i]; + /* The last record deliberately carries no size, so the + * companion field has to be reported as absent rather than + * carrying the previous record's value. */ + if (i + 1 < nkeys) + { + bfixmap(&body, 3); + bfixstr(&body, "name"); bfixstr(&body, name); + bfixstr(&body, "crc"); bbin(&body, crc, 4); + bfixstr(&body, "size"); buint8(&body, (uint8_t)(10 * (i + 1))); + } + else + { + bfixmap(&body, 2); + bfixstr(&body, "name"); bfixstr(&body, name); + bfixstr(&body, "crc"); bbin(&body, crc, 4); + } + } + bnil(&body); + meta_count(&meta, 1); + sprintf(path, "%s/field_scan.rdb", dir); + write_rdb(path, &body, &meta); + bfree(&body); bfree(&meta); + + scan_n = 0; + db = libretrodb_new(); + + begin("field scan reports every record"); + if (!db || libretrodb_open(path, db, false) != 0) + { + check(0, "field scan reports every record", "open failed"); + libretrodb_free(db); + return; + } + ok = (libretrodb_scan_field(db, "crc", "size", scan_collect, NULL) == 0) + && (scan_n == nkeys); + check(ok, "field scan reports every record", + ok ? "all keys seen, repeats included" : "wrong count"); + + begin("scanned keys are in file order"); + ok = 1; + for (i = 0; i < scan_n && i < nkeys; i++) + if (scan_ents[i].key != keys[i]) + ok = 0; + check(ok, "scanned keys are in file order", ok ? "match" : "reordered"); + + begin("companion field follows the key"); + ok = 1; + for (i = 0; i < scan_n && i < nkeys; i++) + { + if (i + 1 < nkeys) + { + if (!scan_ents[i].have_aux || scan_ents[i].aux != 10 * (i + 1)) + ok = 0; + } + else if (scan_ents[i].have_aux) + ok = 0; /* no size on the last record */ + } + check(ok, "companion field follows the key", + ok ? "values match, absence reported" : "wrong or leaked value"); + + begin("offsets resolve to their own record"); + ok = 1; + for (i = 0; i < scan_n; i++) + { + struct rmsgpack_dom_value v; + char want[32], got[64]; + unsigned j; + sprintf(want, "Rec%02u", (unsigned)i); + got[0] = '\0'; + if (libretrodb_read_at(db, scan_ents[i].off, &v) != 0) + { + ok = 0; + break; + } + if (v.type == RDT_MAP) + for (j = 0; j < v.val.map.len; j++) + { + struct rmsgpack_dom_value *k = &v.val.map.items[j].key; + struct rmsgpack_dom_value *w = &v.val.map.items[j].value; + if ( k->type == RDT_STRING && k->val.string.buff + && !strcmp(k->val.string.buff, "name") + && w->type == RDT_STRING && w->val.string.buff) + strncpy(got, w->val.string.buff, sizeof(got) - 1); + } + rmsgpack_dom_value_free(&v); + if (strcmp(want, got)) + { + ok = 0; + break; + } + } + check(ok, "offsets resolve to their own record", + ok ? "including the repeated key" : "wrong record"); + + libretrodb_close(db); + libretrodb_free(db); +} + +int main(int argc, char **argv) +{ + const char *dir = (argc > 1) ? argv[1] : "/tmp"; + + /* Unbuffered: several of these cases crash outright when the + * defect they cover is present, and the line already printed is + * what identifies which one. */ + setvbuf(stdout, NULL, _IONBF, 0); + + printf("libretrodb parser regression test (dir: %s)\n\n", dir); + + case_metadata(dir); + case_nesting(dir); + case_truncation(dir); + case_empty_containers(dir); + case_window(dir); + case_index(dir); + case_query_slices(dir); + case_index_round_trip(dir); + case_field_scan(dir); + + printf("\n%d checks, %d failures\n", checks, failures); + return failures ? 1 : 0; +} diff --git a/libretro-db/samples/libretrodb/sweep.sh b/libretro-db/samples/libretrodb/sweep.sh new file mode 100755 index 000000000000..554b97b6f400 --- /dev/null +++ b/libretro-db/samples/libretrodb/sweep.sh @@ -0,0 +1,193 @@ +#!/bin/sh +# Build and run the libretrodb tests under AddressSanitizer, +# UndefinedBehaviorSanitizer, LeakSanitizer and ThreadSanitizer. +# +# ./sweep.sh [tree] +# +# With no argument the tree is inferred from this script's location. +# Exits non-zero if any run reports, so it can gate a series. +# +# The in-tree tests always run. Setting SWEEP_CORPUS to a directory +# holding .rdb files and the matching harness sources adds the +# larger checks - real databases, a malformed corpus, and the +# threaded runs - which need data too big to keep in the repository. +# +# SWEEP_CORPUS=~/rdb-corpus ./sweep.sh + +set -u +TREE="${1:-$(cd "$(dirname "$0")/../../.." && pwd)}" +CORPUS="${SWEEP_CORPUS:-}" +WORK="$(mktemp -d)" +FAIL=0 + +DB="$TREE/libretro-db" +LC="$TREE/libretro-common" + +LIB="$DB/libretrodb.c $DB/rmsgpack.c $DB/rmsgpack_dom.c $DB/query.c \ +$DB/bintree.c $LC/streams/interface_stream.c $LC/streams/file_stream.c \ +$LC/streams/memory_stream.c $LC/vfs/vfs_implementation.c \ +$LC/compat/compat_strl.c $LC/string/stdstring.c \ +$LC/encodings/encoding_utf.c $LC/file/file_path.c \ +$LC/compat/fopen_utf8.c $LC/time/rtime.c $LC/lists/string_list.c \ +$LC/compat/compat_strcasestr.c $LC/compat/compat_fnmatch.c \ +$LC/encodings/encoding_crc32.c $LC/features/features_cpu.c" + +INC="-I$LC/include -I$DB" + +say() { printf ' %-46s %s\n' "$1" "$2"; } +bad() { FAIL=1; printf ' %-46s %s\n' "$1" "$2"; } + +# $1 = label, $2 = output file. Reports if any sanitizer fired. +judge() +{ + n=$(grep -cE 'ERROR: (Address|Thread|Leak)Sanitizer|runtime error|SUMMARY: .*Sanitizer' "$2" 2>/dev/null || true) + if [ "$n" -eq 0 ]; then say "$1" "clean"; else bad "$1" "$n REPORTS"; fi +} + +echo "sweep: $TREE" +if [ -z "$CORPUS" ]; then + echo " (no SWEEP_CORPUS set: running the in-tree tests only)" +fi + +echo "=== build ===" +for h in t dumpall dump q qoob; do + [ -n "$CORPUS" ] && [ -f "$CORPUS/$h.c" ] || continue + gcc -g -O1 -fsanitize=address,undefined -fno-omit-frame-pointer $INC \ + -o "$WORK/a_$h" "$CORPUS/$h.c" $LIB 2>"$WORK/build_$h.log" \ + || { bad "build $h (asan)" "FAILED"; sed -n '1,3p' "$WORK/build_$h.log"; } +done +for h in race_win race_err; do + [ -n "$CORPUS" ] && [ -f "$CORPUS/$h.c" ] || continue + gcc -g -O1 -fsanitize=thread -fno-omit-frame-pointer -pthread $INC \ + -o "$WORK/t_$h" "$CORPUS/$h.c" $LIB 2>"$WORK/build_$h.log" \ + || bad "build $h (tsan)" "FAILED" +done +say "harnesses" "built" + +echo "=== in-tree regression tests (ASan+UBSan+LeakSan) ===" +( cd "$DB/samples/libretrodb" && make clean >/dev/null 2>&1 + make SANITIZER=address,undefined >"$WORK/mk.log" 2>&1 ) \ + || bad "sample build" "FAILED" +mkdir -p "$WORK/td" +if [ -x "$DB/samples/libretrodb/libretrodb_parser_test" ]; then + ASAN_OPTIONS=detect_leaks=1 \ + "$DB/samples/libretrodb/libretrodb_parser_test" "$WORK/td" \ + >"$WORK/parser.out" 2>&1 + res=$(tr '\r' '\n' < "$WORK/parser.out" | grep -oE '[0-9]+ checks, [0-9]+ failures') + case "$res" in + *", 0 failures") judge "parser test ($res)" "$WORK/parser.out" ;; + *) bad "parser test" "${res:-no result}" ;; + esac +else + bad "parser test" "not built" +fi +if [ -x "$DB/samples/libretrodb/libretrodb_leak_test" ]; then + ASAN_OPTIONS=detect_leaks=1 \ + "$DB/samples/libretrodb/libretrodb_leak_test" >"$WORK/leak.out" 2>&1 + judge "leak test" "$WORK/leak.out" +else + bad "leak test" "not built" +fi +( cd "$DB/samples/libretrodb" && make clean >/dev/null 2>&1 ) + +echo "=== archive and lzma tests (ASan+UBSan+LeakSan) ===" +# Both cover defects that stopped a scan dead rather than failing: +# an lc=4 stream the decoder refused, and a lookup for a member that +# is not there spinning instead of returning. +for d in "$LC/samples/file/archive_file" "$LC/samples/formats/r7z"; do + [ -f "$d/Makefile" ] || continue + name=$(basename "$d") + ( cd "$d" && make clean >/dev/null 2>&1 + make check SANITIZER=address,undefined ) >"$WORK/$name.log" 2>&1 + rc=$? + ( cd "$d" && make clean >/dev/null 2>&1 ) + if [ $rc -eq 0 ]; then + judge "$name" "$WORK/$name.log" + else + bad "$name" "exit $rc (a hang here reads as a timeout)" + grep -E "FAIL|error:" "$WORK/$name.log" | head -3 + fi +done + +echo "=== scanner tests (ASan+UBSan+LeakSan) ===" +# These link most of the frontend, so they are slower to build than +# everything else here; they are the only coverage of the scan as a +# whole, including the task teardown and the playlist write. +SCAN_DIR="$TREE/samples/tasks/database" +if [ -f "$SCAN_DIR/Makefile" ]; then + ( cd "$SCAN_DIR" && make clean >/dev/null 2>&1 + make check SANITIZER=address,undefined >"$WORK/scan.log" 2>&1 ) + res=$(grep -oE '[0-9]+ checks, [0-9]+ failures' "$WORK/scan.log" | tr '\n' ' ') + if grep -q "0 failures" "$WORK/scan.log" \ + && ! grep -qE '[1-9][0-9]* failures' "$WORK/scan.log"; then + judge "scanner tests ($res)" "$WORK/scan.log" + else + bad "scanner tests" "${res:-did not run}" + grep -E "FAIL|error:" "$WORK/scan.log" | head -3 + fi + ( cd "$SCAN_DIR" && make clean >/dev/null 2>&1 ) +else + say "scanner tests" "skipped (sample not present)" +fi + +echo "=== real databases (ASan+UBSan+LeakSan) ===" +if [ -n "$CORPUS" ] && [ -d "$CORPUS/real" ] && [ -x "$WORK/a_dumpall" ]; then + for f in "$CORPUS"/real/*.rdb; do + [ -e "$f" ] || continue + ASAN_OPTIONS=detect_leaks=1 "$WORK/a_dumpall" "$f" \ + >"$WORK/rec.out" 2>"$WORK/rec.err" + judge "$(basename "$f" .rdb) ($(wc -l < "$WORK/rec.out") records)" "$WORK/rec.err" + done +else + say "real databases" "skipped (no corpus)" +fi + +echo "=== malformed corpus (ASan+UBSan+LeakSan) ===" +if [ -x "$WORK/a_t" ]; then + n=0; r=0 + for f in "$CORPUS"/rdb/*.rdb "$CORPUS"/idx/*.rdb "$CORPUS"/typeconf*.rdb \ + "$CORPUS"/bigfield.rdb "$CORPUS"/valid/valid.rdb; do + [ -e "$f" ] || continue + n=$((n+1)) + ASAN_OPTIONS=detect_leaks=1 timeout 90 "$WORK/a_t" "$f" \ + >"$WORK/c.out" 2>&1 + st=$? + if [ $st -eq 124 ]; then r=$((r+1)); bad "hang" "$(basename "$f")"; fi + grep -qE 'ERROR: |runtime error' "$WORK/c.out" && { r=$((r+1)); bad "report" "$(basename "$f")"; } + done + [ "$r" -eq 0 ] && say "$n files" "clean" +fi + +echo "=== query parser, unterminated buffers (ASan+UBSan) ===" +if [ -n "$CORPUS" ] && [ -x "$WORK/a_qoob" ] && [ -f "$CORPUS/valid/valid.rdb" ]; then + ASAN_OPTIONS=detect_leaks=1 "$WORK/a_qoob" "$CORPUS/valid/valid.rdb" \ + >"$WORK/q.out" 2>&1 + judge "slice handling" "$WORK/q.out" +fi + +echo "=== ThreadSanitizer ===" +if [ -x "$WORK/t_race_win" ]; then + for f in "$CORPUS"/real/*.rdb "$CORPUS"/valid/valid.rdb; do + [ -e "$f" ] || continue + TSAN_OPTIONS=exitcode=0 timeout 400 "$WORK/t_race_win" "$f" \ + >"$WORK/tw.out" 2>&1 + judge "concurrent scans: $(basename "$f" .rdb)" "$WORK/tw.out" + done +fi +if [ -x "$WORK/t_race_err" ]; then + # any real database will do; fall back to the small built one + f=$(ls "$CORPUS"/real/*.rdb 2>/dev/null | head -1) + [ -n "$f" ] && [ -e "$f" ] || f="$CORPUS/valid/valid.rdb" + TSAN_OPTIONS=exitcode=0 timeout 400 "$WORK/t_race_err" "$f" \ + >"$WORK/te.out" 2>&1 + judge "concurrent failing-query compiles" "$WORK/te.out" +fi + +rm -rf "$WORK" +echo +if [ $FAIL -eq 0 ]; then + echo "SWEEP CLEAN" +else + echo "SWEEP FAILED" +fi +exit $FAIL diff --git a/manual_content_scan.c b/manual_content_scan.c index 17321fd3b135..7fad1b3b4b5d 100644 --- a/manual_content_scan.c +++ b/manual_content_scan.c @@ -326,20 +326,55 @@ bool manual_content_scan_set_menu_content_dir(const char *content_dir) { size_t _len; const char *dir_name = NULL; - char _tmpbuf[PATH_MAX_LENGTH]; + + /* Sanity check before the expansion. It used to sit after + * content_dir had been reassigned to _tmpbuf, by which point it + * could never fire - and fill_pathname_expand_special() had + * already dereferenced the caller's pointer. */ + if (!content_dir || !*content_dir) + goto error; + fill_pathname_expand_special(_tmpbuf, content_dir, sizeof(_tmpbuf)); content_dir = _tmpbuf; - /* Sanity check */ - if (!content_dir || !*content_dir) + if (!*content_dir) goto error; /* Copy directory path to settings struct. - * Remove trailing slash, if required */ - if ((_len = strlcpy( - scan_content_dir, content_dir, - sizeof(scan_content_dir))) <= 0) + * Remove trailing slash, if required. + * + * strlcpy() returns the length of its source, not the number of + * bytes it wrote, and the source can be longer than the + * destination here: _tmpbuf is PATH_MAX_LENGTH while + * scan_content_dir is DIR_MAX_LENGTH, which is half of it on + * every platform. Indexing with the return value therefore ran + * off the end of the buffer for any content directory longer than + * DIR_MAX_LENGTH - a deeply nested folder picked in the file + * browser is enough: + * + * AddressSanitizer: heap-buffer-overflow + * READ of size 1 ... located 1022 bytes after 1024-byte region + */ + _len = strlcpy(scan_content_dir, content_dir, + sizeof(scan_content_dir)); + + if (_len == 0) + goto error; + + /* A path that did not fit is rejected rather than kept truncated. + * Truncation cuts mid-component, so what remains is a different + * directory: scanning ".../subdirectory_number_041/subdirectory_n" + * either finds nothing, with no indication why, or finds whatever + * happens to sit at that shortened path. The tail also becomes + * the content-directory system name, so a scan that did produce + * entries would file them 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 instead surfaces it as + * an invalid content directory, which is what the menu already + * reports for the other rejections above. */ + if (_len >= sizeof(scan_content_dir)) goto error; if (scan_content_dir[_len - 1] == PATH_DEFAULT_SLASH_C()) diff --git a/playlist.c b/playlist.c index 040595e73a05..0bdafb27e02b 100644 --- a/playlist.c +++ b/playlist.c @@ -3088,6 +3088,7 @@ playlist_t *playlist_init(const playlist_config_t *config) playlist->scan_record.search_recursively = false; playlist->scan_record.search_archives = false; playlist->scan_record.filter_dat_content = false; + playlist->scan_record.overwrite_playlist = false; playlist->scan_record.omit_db_ref = false; playlist->scan_record.content_dir = NULL; playlist->scan_record.file_exts = NULL; diff --git a/samples/tasks/database/Makefile b/samples/tasks/database/Makefile index 6aa7b0dddc98..2457a9aca85b 100644 --- a/samples/tasks/database/Makefile +++ b/samples/tasks/database/Makefile @@ -174,6 +174,18 @@ CXXFLAGS += $(DEFINES) OBJECTS = $(SOURCES_C:.c=.o) +# The index pairing test links the same objects but its own entry +# point, so the scanner's main() is swapped out for it. +SCAN_TEST := database_scan_test +SCAN_TEST_MAIN := $(CORE_DIR)/samples/tasks/database/database_scan_test.o +SCAN_TEST_OBJECTS = $(filter-out $(CORE_DIR)/samples/tasks/database/main.o,$(OBJECTS)) \ + $(SCAN_TEST_MAIN) + +INDEX_TEST := database_index_test +INDEX_TEST_MAIN := $(CORE_DIR)/samples/tasks/database/database_index_test.o +INDEX_TEST_OBJECTS = $(filter-out $(CORE_DIR)/samples/tasks/database/main.o,$(OBJECTS)) \ + $(INDEX_TEST_MAIN) + OBJOUT = -o LINKOUT = -o @@ -193,6 +205,17 @@ all: $(TARGET)$(EXE_EXT) $(TARGET)$(EXE_EXT): $(OBJECTS) $(LD) $(LINKOUT)$@ $(SHARED) $(OBJECTS) $(LDFLAGS) $(LIBS) +$(INDEX_TEST)$(EXE_EXT): $(INDEX_TEST_OBJECTS) + $(LD) $(LINKOUT)$@ $(SHARED) $(INDEX_TEST_OBJECTS) $(LDFLAGS) $(LIBS) + +# Builds its own databases, so it needs nothing but a writable dir. +$(SCAN_TEST)$(EXE_EXT): $(SCAN_TEST_OBJECTS) + $(LD) $(LINKOUT)$@ $(SHARED) $(SCAN_TEST_OBJECTS) $(LDFLAGS) $(LIBS) + +check: $(INDEX_TEST)$(EXE_EXT) $(SCAN_TEST)$(EXE_EXT) + ./$(INDEX_TEST)$(EXE_EXT) + ./$(SCAN_TEST)$(EXE_EXT) + %.o: %.c $(CC) $(INCFLAGS) $(CFLAGS) -c $(OBJOUT)$@ $< @@ -200,4 +223,7 @@ $(TARGET)$(EXE_EXT): $(OBJECTS) $(CXX) $(INCFLAGS) $(CXXFLAGS) -c $(OBJOUT)$@ $< clean: - rm -f $(OBJECTS) + rm -f $(OBJECTS) $(INDEX_TEST_MAIN) $(SCAN_TEST_MAIN) \ + $(TARGET)$(EXE_EXT) $(INDEX_TEST)$(EXE_EXT) $(SCAN_TEST)$(EXE_EXT) + +.PHONY: all check clean diff --git a/samples/tasks/database/database_index_test.c b/samples/tasks/database/database_index_test.c new file mode 100644 index 000000000000..182771fcb2c8 --- /dev/null +++ b/samples/tasks/database/database_index_test.c @@ -0,0 +1,410 @@ +/* Regression test for the crc and serial index caches. + * + * The scanner keeps one index per database, keyed by the database's + * position in its list, and promotes a database to the front of that + * list whenever it matches. Anything else keyed by the same position + * has to be promoted with it. + * + * When the index caches were first added they were not, so after the + * first match the index at a given slot described a different + * database than the entry at that slot. A lookup then answered with + * records belonging to another system, and the scanner filed the + * content under whichever database happened to be in the slot - a + * a title recorded under a wholly unrelated system, with nothing + * reporting an error. + * + * Two things are checked here: + * + * - a lookup against the index built for that database returns the + * record the database actually holds; + * - a lookup against an index built for a *different* database is + * refused rather than answered, so that if the pairing is ever + * broken again the result is the slow path rather than a wrong + * one. + * + * Builds its own databases so it depends on nothing but the library. + * + * Run with: make database_index_test SANITIZER=address,undefined + */ + +#include +#include +#include +#include + +#include + +#include +#include +#include +#include +#include "../../../core_info.h" +#include "../../../tasks/tasks_internal.h" +#include +#include +#include "../../../manual_content_scan.h" +#include "../../../configuration.h" +#include "../../../verbosity.h" +#include "../../../database_info.h" + +/* The scanner objects this links against expect these; lifted from + * main.c in this directory, which provides the same set. */ + + + +/* Stubs for symbols referenced by the retroarch-tree sources we pull + * in. The real definitions live in intl/msg_hash_us.c and + * configuration.c, but those files transitively require RARCH_INTERNAL + * which drags in the entire frontend subsystem. This sample only + * exercises task_push_dbscan; none of these symbols are actually + * invoked on the path through task_push_dbscan / task_queue_check. + * + * These take enum msg_hash_enums now that configuration.h - included + * for settings_t - brings msg_hash.h with it. They used to be + * declared with int, which matched at the link level but conflicts + * once the real prototypes are visible. */ +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 ""; +} + +/* The string-table index builder (added by the msg_hash strtab refactor). + * With msg_hash_to_str_us() stubbed above, the index is never consulted, so + * this can be an empty stub rather than linking intl/msg_hash_us.c. */ +void msg_hash_us_index_init(void) +{ +} + +settings_t *config_get_ptr(void) +{ + /* A real settings_t, not a zeroed blob. + * + * The scan reads settings->paths.directory_playlist before it does + * anything else and refuses to start if it is empty - so a stub that + * returned zeros meant no task was ever created, the completion + * callback never fired, and this sample sat in its loop until the CI + * runner killed it. The playlist directory argument it accepts on + * the command line went nowhere. + * + * Static so it is zero-initialised; main() fills in the one field + * the scan actually consults. */ + static settings_t settings; + return &settings; +} + +/* Additional stubs for retroarch-core symbols referenced transitively. + * None of these are exercised on the dbscan path; they're link-time + * stubs to avoid pulling in retroarch.c, runloop.c, frontend drivers, + * and the UI/video subsystems. */ +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; +} + +void ui_companion_driver_notify_refresh(void) +{ +} + +void video_display_server_set_window_progress(int progress, bool finished) +{ + (void)progress; (void)finished; +} + +/* task_database.c's progress_cb is now the shared task_window_progress_cb, + * whose definition lives in tasks/task_file_transfer.c. Pulling that + * file in would drag in nbio, the audio mixer, and the image-task + * machinery, none of which the dbscan path exercises. Stub it here + * to satisfy the linker; the function is never called on this code + * path because no progress_cb is invoked unless a worker thread + * publishes progress, and this sample never reaches that state. */ +void task_window_progress_cb(retro_task_t *task) +{ + (void)task; +} + +/* dir_list_new_special lives in retroarch.c, which we cannot link + * without dragging in the world. manual_content_scan calls this to + * walk the scan directory; returning NULL causes the scan to bail + * without producing results, which is fine for a standalone demo. */ +void *dir_list_new_special(const char *input_dir, unsigned type, + const char *filter, bool show_hidden_files) +{ + (void)input_dir; (void)type; (void)filter; (void)show_hidden_files; + return NULL; +} + +static int failures = 0; +static int checks = 0; + +static void check(int ok, const char *what, const char *detail) +{ + checks++; + printf(" %-5s %-40s %s\n", ok ? "ok" : "FAIL", what, + detail ? detail : ""); + if (!ok) + failures++; +} + +/* ------------------------------------------------------------------ + * Minimal MsgPack writer, so the test does not build its inputs with + * the library it is testing. + * ------------------------------------------------------------------ */ + +typedef struct { uint8_t *data; size_t len, cap; } buf_t; + +static void bput(buf_t *b, const void *p, size_t n) +{ + if (b->len + n > b->cap) + { + size_t want = b->cap ? b->cap * 2 : 256; + while (want < b->len + n) + want *= 2; + b->data = (uint8_t*)realloc(b->data, want); + b->cap = want; + } + memcpy(b->data + b->len, p, n); + b->len += n; +} +static void bbyte(buf_t *b, uint8_t v) { bput(b, &v, 1); } +static void bfixmap(buf_t *b, unsigned n) { bbyte(b, (uint8_t)(0x80 | n)); } +static void bfixstr(buf_t *b, const char *s) +{ + size_t n = strlen(s); + bbyte(b, (uint8_t)(0xa0 | n)); + bput(b, s, n); +} +static void bbin(buf_t *b, const void *s, uint8_t n) +{ + bbyte(b, 0xc4); + bbyte(b, n); + bput(b, s, n); +} +static void buint8(buf_t *b, uint8_t v) { bbyte(b, 0xcc); bbyte(b, v); } + +/* One database: @count records named " NNN", carrying crcs from + * @crc_base upwards and a serial built from @tag. */ +static int write_db(const char *path, const char *tag, uint32_t crc_base, + int count) +{ + buf_t body, meta; + FILE *f; + uint8_t hdr[16]; + uint64_t off; + int i; + + memset(&body, 0, sizeof(body)); + memset(&meta, 0, sizeof(meta)); + + for (i = 0; i < count; i++) + { + char name[64], serial[64]; + uint8_t crc[4]; + uint32_t c = crc_base + (uint32_t)i; + + sprintf(name, "%s %03d", tag, i); + sprintf(serial, "%s-%03d", tag, i); + crc[0] = (uint8_t)(c >> 24); crc[1] = (uint8_t)(c >> 16); + crc[2] = (uint8_t)(c >> 8); crc[3] = (uint8_t)c; + + bfixmap(&body, 4); + bfixstr(&body, "name"); bfixstr(&body, name); + bfixstr(&body, "crc"); bbin(&body, crc, 4); + bfixstr(&body, "serial"); bbin(&body, serial, (uint8_t)strlen(serial)); + bfixstr(&body, "size"); buint8(&body, (uint8_t)(16 + i)); + } + bbyte(&body, 0xc0); + + bfixmap(&meta, 1); + bfixstr(&meta, "count"); + buint8(&meta, 1); + + off = 16 + (uint64_t)body.len; + memcpy(hdr, "RARCHDB", 7); + hdr[7] = 0; + for (i = 0; i < 8; i++) + hdr[8 + i] = (uint8_t)(off >> (56 - 8 * i)); + + if (!(f = fopen(path, "wb"))) + return 0; + fwrite(hdr, 1, sizeof(hdr), f); + fwrite(body.data, 1, body.len, f); + fwrite(meta.data, 1, meta.len, f); + fclose(f); + + free(body.data); + free(meta.data); + return 1; +} + +/* Name of the first record a lookup returns, or "" for none. */ +static void crc_lookup(const database_info_crc_index_t *idx, + const char *path, uint32_t crc, char *out, size_t len) +{ + database_info_list_t *l = database_info_list_new_crc(idx, path, crc, 0, + DB_EXTRACT_SCAN_FIELDS); + out[0] = '\0'; + if (l) + { + if (l->count && l->list[0].name) + strncpy(out, l->list[0].name, len - 1); + database_info_list_free(l); + free(l); + } + else + strncpy(out, "(refused)", len - 1); + out[len - 1] = '\0'; +} + +static void serial_lookup(const database_info_serial_index_t *idx, + const char *path, const char *serial, char *out, size_t len) +{ + database_info_list_t *l = database_info_list_new_serial(idx, path, + serial, DB_EXTRACT_SCAN_FIELDS); + out[0] = '\0'; + if (l) + { + if (l->count && l->list[0].name) + strncpy(out, l->list[0].name, len - 1); + database_info_list_free(l); + free(l); + } + else + strncpy(out, "(refused)", len - 1); + out[len - 1] = '\0'; +} + +int main(int argc, char **argv) +{ + const char *dir = (argc > 1) ? argv[1] : "/tmp"; + char path_a[512], path_b[512]; + char got[256]; + database_info_crc_index_t *cia, *cib; + database_info_serial_index_t *sia, *sib; + + setvbuf(stdout, NULL, _IONBF, 0); + printf("database index pairing test (dir: %s)\n\n", dir); + + sprintf(path_a, "%s/index_pair_a.rdb", dir); + sprintf(path_b, "%s/index_pair_b.rdb", dir); + + if ( !write_db(path_a, "Alpha", 0x1000, 40) + || !write_db(path_b, "Beta", 0x2000, 40)) + { + check(0, "test databases", "could not be written"); + return 1; + } + + cia = database_info_crc_index_new(path_a, 0); + cib = database_info_crc_index_new(path_b, 0); + sia = database_info_serial_index_new(path_a, 0); + sib = database_info_serial_index_new(path_b, 0); + + if (!cia || !cib || !sia || !sib) + { + check(0, "index construction", "failed"); + return 1; + } + + check( database_info_crc_index_count(cia) == 40 + && database_info_crc_index_count(cib) == 40, + "indexes cover every record", "40 each"); + + /* Correct pairing resolves. */ + crc_lookup(cia, path_a, 0x1005, got, sizeof(got)); + check(!strcmp(got, "Alpha 005"), "crc lookup, index matches database", + got); + crc_lookup(cib, path_b, 0x2007, got, sizeof(got)); + check(!strcmp(got, "Beta 007"), "crc lookup, other database", got); + + serial_lookup(sia, path_a, "Alpha-012", got, sizeof(got)); + check(!strcmp(got, "Alpha 012"), "serial lookup, index matches database", + got); + + /* The scanner compares db_info->serial against the serial it read + * off the disc, so that field has to survive extraction. Every + * shipped database stores serial as binary rather than string - + * 27056 binary and 0 string in Sony - PlayStation 2 - and a type + * check that accepted only RDT_STRING left it NULL, which silently + * stopped every disc system matching while crc-based ones kept + * working. */ + { + database_info_list_t *l = database_info_list_new_serial(sia, path_a, + "Alpha-012", DB_EXTRACT_SCAN_FIELDS); + const char *got_serial = (l && l->count) ? l->list[0].serial : NULL; + check(got_serial && !strcmp(got_serial, "Alpha-012"), + "binary serial survives extraction", + got_serial ? got_serial : "(NULL)"); + if (l) { database_info_list_free(l); free(l); } + } + + /* Mismatched pairing must be refused, not answered. Without the + * check these return the other database's record, which is how the + * scanner came to file content under the wrong system. */ + crc_lookup(cib, path_a, 0x2007, got, sizeof(got)); + check(!strcmp(got, "(refused)"), + "crc lookup, index from another database", got); + + serial_lookup(sib, path_a, "Beta-012", got, sizeof(got)); + check(!strcmp(got, "(refused)"), + "serial lookup, index from another database", got); + + /* A crc that only the other database holds must not resolve even + * with the pairing correct. */ + crc_lookup(cia, path_a, 0x2007, got, sizeof(got)); + check(got[0] == '\0', "crc absent from this database", + got[0] ? got : "no match"); + + /* A budget too small to hold the table must yield nothing rather + * than a short one: a partial index misses matches silently, which + * is the failure the whole fallback design exists to avoid. */ + { + database_info_crc_index_t *tiny = + database_info_crc_index_new(path_a, 64); + check(tiny == NULL, "index refused when over budget", + tiny ? "built anyway" : "refused"); + database_info_crc_index_free(tiny); + } + { + database_info_crc_index_t *ample = + database_info_crc_index_new(path_a, 1024 * 1024); + char tmp[256]; + check(ample != NULL, "index built when within budget", + ample ? "built" : "refused"); + if (ample) + { + crc_lookup(ample, path_a, 0x1005, tmp, sizeof(tmp)); + check(!strcmp(tmp, "Alpha 005"), "budgeted index still complete", + tmp); + check(database_info_crc_index_bytes(ample) > 0, + "index reports its size", + database_info_crc_index_bytes(ample) ? "non-zero" : "zero"); + } + database_info_crc_index_free(ample); + } + + database_info_crc_index_free(cia); + database_info_crc_index_free(cib); + database_info_serial_index_free(sia); + database_info_serial_index_free(sib); + + printf("\n%d checks, %d failures\n", checks, failures); + return failures ? 1 : 0; +} diff --git a/samples/tasks/database/database_scan_test.c b/samples/tasks/database/database_scan_test.c new file mode 100644 index 000000000000..939955eacb6a --- /dev/null +++ b/samples/tasks/database/database_scan_test.c @@ -0,0 +1,625 @@ +/* End-to-end regression test for the content scanner. + * + * Drives task_push_dbscan() over content that is 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, the database iteration, the crc lookup, the + * promotion of a matched database to the front of the list, the + * playlist write and the teardown. Two defects in this tree went + * undetected because nothing exercised that: + * + * - the index caches were keyed by a database's position in the + * list but not moved when a match promoted it, so content ended + * up filed under whichever database took the vacated slot; + * - playlist_init() left scan_record.overwrite_playlist + * uninitialised, and it is written into the playlist and read + * back on the next scan to decide whether to overwrite. + * + * The second content file therefore matches the *second* database on + * purpose. A match in the first would never trigger the promotion, + * and the pairing bug would pass unnoticed again. + * + * Everything is built here - databases, content, core info - so the + * test needs no fixture beyond a writable directory. Content is made + * to match by forcing its crc32: four bytes appended to a buffer can + * bring the checksum to any value, which is cheaper and more precise + * than searching for content that happens to collide. + * + * Run with: make scan_test SANITIZER=address,undefined + */ + +#include +#include +#include +#include +#include + +#include +#include +#include +#include + +#include "../../../core_info.h" +#include "../../../tasks/tasks_internal.h" +#include "../../../manual_content_scan.h" +#include "../../../list_special.h" +#include "../../../configuration.h" +#include "../../../verbosity.h" + +#define SCAN_TIMEOUT_SECONDS 120 + + +/* Stubs for symbols referenced by the retroarch-tree sources we pull + * in. The real definitions live in intl/msg_hash_us.c and + * configuration.c, but those files transitively require RARCH_INTERNAL + * which drags in the entire frontend subsystem. This sample only + * exercises task_push_dbscan; none of these symbols are actually + * invoked on the path through task_push_dbscan / task_queue_check. + * + * These take enum msg_hash_enums now that configuration.h - included + * for settings_t - brings msg_hash.h with it. They used to be + * declared with int, which matched at the link level but conflicts + * once the real prototypes are visible. */ +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 ""; +} + +/* The string-table index builder (added by the msg_hash strtab refactor). + * With msg_hash_to_str_us() stubbed above, the index is never consulted, so + * this can be an empty stub rather than linking intl/msg_hash_us.c. */ +void msg_hash_us_index_init(void) +{ +} + +settings_t *config_get_ptr(void) +{ + /* A real settings_t, not a zeroed blob. + * + * The scan reads settings->paths.directory_playlist before it does + * anything else and refuses to start if it is empty - so a stub that + * returned zeros meant no task was ever created, the completion + * callback never fired, and this sample sat in its loop until the CI + * runner killed it. The playlist directory argument it accepts on + * the command line went nowhere. + * + * Static so it is zero-initialised; main() fills in the one field + * the scan actually consults. */ + static settings_t settings; + return &settings; +} + +/* Additional stubs for retroarch-core symbols referenced transitively. + * None of these are exercised on the dbscan path; they're link-time + * stubs to avoid pulling in retroarch.c, runloop.c, frontend drivers, + * and the UI/video subsystems. */ +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; +} + +void ui_companion_driver_notify_refresh(void) +{ +} + +void video_display_server_set_window_progress(int progress, bool finished) +{ + (void)progress; (void)finished; +} + +/* task_database.c's progress_cb is now the shared task_window_progress_cb, + * whose definition lives in tasks/task_file_transfer.c. Pulling that + * file in would drag in nbio, the audio mixer, and the image-task + * machinery, none of which the dbscan path exercises. Stub it here + * to satisfy the linker; the function is never called on this code + * path because no progress_cb is invoked unless a worker thread + * publishes progress, and this sample never reaches that state. */ +void task_window_progress_cb(retro_task_t *task) +{ + (void)task; +} + +/* dir_list_new_special lives in retroarch.c, which we cannot link + * without dragging in the world. + * + * Returning NULL here meant the scan never enumerated its databases, + * so it could not finish, and everything downstream of a completed + * scan - notably the task teardown - went unexercised. The scanner + * only asks for DIR_LIST_DATABASES, which retroarch.c answers with a + * plain "rdb" listing, so that much is worth providing. */ +struct string_list *dir_list_new_special(const char *input_dir, + enum dir_list_type type, const char *filter, bool show_hidden_files) +{ + (void)filter; + + if (type == DIR_LIST_DATABASES) + return dir_list_new(input_dir, "rdb", false, show_hidden_files, + false, false); + + return NULL; +} + +static int failures = 0; +static int checks = 0; + +static void check(int ok, const char *what, const char *detail) +{ + checks++; + printf(" %-5s %-42s %s\n", ok ? "ok" : "FAIL", what, + detail ? detail : ""); + if (!ok) + failures++; +} + +/* ------------------------------------------------------------------ + * crc32, and forcing a buffer to a chosen one + * ------------------------------------------------------------------ */ + +static uint32_t crc_table[256]; +static uint8_t crc_top[256]; /* table index by top byte */ + +static void crc_init(void) +{ + unsigned i, j; + for (i = 0; i < 256; i++) + { + uint32_t c = i; + for (j = 0; j < 8; j++) + c = (c >> 1) ^ ((c & 1) ? 0xEDB88320u : 0u); + crc_table[i] = c; + } + for (i = 0; i < 256; i++) + crc_top[crc_table[i] >> 24] = (uint8_t)i; +} + +static uint32_t crc_of(const uint8_t *data, size_t len) +{ + uint32_t reg = 0xFFFFFFFFu; + size_t i; + for (i = 0; i < len; i++) + reg = (reg >> 8) ^ crc_table[(reg ^ data[i]) & 0xFF]; + return reg ^ 0xFFFFFFFFu; +} + +/* Four bytes that, appended to @data, bring its crc32 to @target. + * + * Each step of the checksum is reversible: the top byte of the + * register after a step identifies the table entry used, which gives + * both the register before it and the byte that was fed in. Walking + * four steps back from the wanted value and then forward again from + * the current one yields the bytes. */ +static void crc_force(const uint8_t *data, size_t len, uint32_t target, + uint8_t out[4]) +{ + uint32_t reg = crc_of(data, len) ^ 0xFFFFFFFFu; + uint32_t r = target ^ 0xFFFFFFFFu; + uint8_t idx[4]; + int i; + + for (i = 0; i < 4; i++) + { + idx[i] = crc_top[r >> 24]; + r = (r ^ crc_table[idx[i]]) << 8; + } + for (i = 3; i >= 0; i--) + { + out[3 - i] = (uint8_t)((reg & 0xFF) ^ idx[i]); + reg = (reg >> 8) ^ crc_table[idx[i]]; + } +} + +/* ------------------------------------------------------------------ + * A minimal .rdb, so the test does not depend on shipped data + * ------------------------------------------------------------------ */ + +typedef struct { uint8_t *d; size_t len, cap; } buf_t; + +static void bput(buf_t *b, const void *p, size_t n) +{ + if (b->len + n > b->cap) + { + size_t want = b->cap ? b->cap * 2 : 256; + while (want < b->len + n) + want *= 2; + b->d = (uint8_t*)realloc(b->d, want); + b->cap = want; + } + memcpy(b->d + b->len, p, n); + b->len += n; +} +static void bbyte(buf_t *b, uint8_t v) { bput(b, &v, 1); } +static void bfixmap(buf_t *b, unsigned n) { bbyte(b, (uint8_t)(0x80 | n)); } +static void bfixstr(buf_t *b, const char *s) +{ + size_t n = strlen(s); + bbyte(b, (uint8_t)(0xa0 | n)); + bput(b, s, n); +} +static void bbin4(buf_t *b, uint32_t v) +{ + uint8_t c[4]; + c[0] = (uint8_t)(v >> 24); c[1] = (uint8_t)(v >> 16); + c[2] = (uint8_t)(v >> 8); c[3] = (uint8_t)v; + bbyte(b, 0xc4); bbyte(b, 4); bput(b, c, 4); +} +static void buint32(buf_t *b, uint32_t v) +{ + uint8_t c[4]; + c[0] = (uint8_t)(v >> 24); c[1] = (uint8_t)(v >> 16); + c[2] = (uint8_t)(v >> 8); c[3] = (uint8_t)v; + bbyte(b, 0xce); bput(b, c, 4); +} + +/* One database holding a single named record. */ +static int write_db(const char *path, const char *game, uint32_t crc, + uint32_t size) +{ + buf_t body, meta; + FILE *f; + uint8_t hdr[16]; + uint64_t off; + int i; + + memset(&body, 0, sizeof(body)); + memset(&meta, 0, sizeof(meta)); + + bfixmap(&body, 3); + bfixstr(&body, "name"); bfixstr(&body, game); + bfixstr(&body, "crc"); bbin4(&body, crc); + bfixstr(&body, "size"); buint32(&body, size); + bbyte(&body, 0xc0); + + bfixmap(&meta, 1); + bfixstr(&meta, "count"); + bbyte(&meta, 0x01); + + off = 16 + (uint64_t)body.len; + memcpy(hdr, "RARCHDB", 7); + hdr[7] = 0; + for (i = 0; i < 8; i++) + hdr[8 + i] = (uint8_t)(off >> (56 - 8 * i)); + + if (!(f = fopen(path, "wb"))) + return 0; + fwrite(hdr, 1, sizeof(hdr), f); + fwrite(body.d, 1, body.len, f); + fwrite(meta.d, 1, meta.len, f); + fclose(f); + free(body.d); + free(meta.d); + return 1; +} + +/* Content of @size whose crc32 is @crc. */ +static int write_content(const char *path, uint32_t crc, uint32_t size) +{ + uint8_t *d = (uint8_t*)malloc(size); + uint32_t i; + FILE *f; + + if (!d || size < 8) + { + free(d); + return 0; + } + for (i = 0; i < size - 4; i++) + d[i] = (uint8_t)((i * 37 + 11) & 0xFF); + crc_force(d, size - 4, crc, d + size - 4); + + if (crc_of(d, size) != crc) + { + free(d); + return 0; + } + if (!(f = fopen(path, "wb"))) + { + free(d); + return 0; + } + fwrite(d, 1, size, f); + fclose(f); + free(d); + return 1; +} + +/* A one-entry zip using the stored method, so no compressor is needed + * and the bytes are entirely predictable. Written by hand because + * the point is to exercise the scanner's archive handling, not to + * depend on a fixture checked into the tree. */ +static int write_zip(const char *path, const char *member, + const uint8_t *data, uint32_t len, uint32_t crc) +{ + FILE *f = fopen(path, "wb"); + uint16_t nlen = (uint16_t)strlen(member); + uint8_t h[46]; + uint32_t local_off = 0; + + if (!f) + return 0; + +#define PUT16(p, v) do { (p)[0] = (uint8_t)(v); (p)[1] = (uint8_t)((v) >> 8); } while (0) +#define PUT32(p, v) do { (p)[0] = (uint8_t)(v); (p)[1] = (uint8_t)((v) >> 8); (p)[2] = (uint8_t)((v) >> 16); (p)[3] = (uint8_t)((v) >> 24); } while (0) + + /* local file header */ + memset(h, 0, 30); + PUT32(h, 0x04034b50); PUT16(h + 4, 20); PUT16(h + 6, 0); + PUT16(h + 8, 0); /* stored */ + PUT16(h + 10, 0); PUT16(h + 12, 0); + PUT32(h + 14, crc); PUT32(h + 18, len); PUT32(h + 22, len); + PUT16(h + 26, nlen); PUT16(h + 28, 0); + fwrite(h, 1, 30, f); + fwrite(member, 1, nlen, f); + fwrite(data, 1, len, f); + + /* central directory */ + local_off = 0; + memset(h, 0, 46); + PUT32(h, 0x02014b50); PUT16(h + 4, 20); PUT16(h + 6, 20); + PUT16(h + 8, 0); PUT16(h + 10, 0); + PUT16(h + 12, 0); PUT16(h + 14, 0); + PUT32(h + 16, crc); PUT32(h + 20, len); PUT32(h + 24, len); + PUT16(h + 28, nlen); PUT16(h + 30, 0); PUT16(h + 32, 0); + PUT16(h + 34, 0); PUT16(h + 36, 0); PUT32(h + 38, 0); + PUT32(h + 42, local_off); + fwrite(h, 1, 46, f); + fwrite(member, 1, nlen, f); + + /* end of central directory */ + memset(h, 0, 22); + PUT32(h, 0x06054b50); + PUT16(h + 8, 1); PUT16(h + 10, 1); + PUT32(h + 12, 46 + nlen); + PUT32(h + 16, 30 + nlen + len); + fwrite(h, 1, 22, f); + fclose(f); +#undef PUT16 +#undef PUT32 + return 1; +} + +static int file_contains(const char *path, const char *needle) +{ + int64_t len = 0; + char *data = NULL; + int hit; + + if (!filestream_read_file(path, (void**)&data, &len) || !data) + return 0; + hit = (strstr(data, needle) != NULL); + free(data); + return hit; +} + +/* ------------------------------------------------------------------ */ + +static bool loop_active = true; +static bool scan_completed = false; + +static void scan_cb(retro_task_t *task, void *task_data, + void *user_data, const char *err) +{ + (void)task; (void)task_data; (void)user_data; + if (err && *err) + fprintf(stderr, " scan reported an error: %s\n", err); + scan_completed = true; + loop_active = false; +} + +static void msgq_push(retro_task_t *task, const char *msg, + unsigned prio, unsigned duration, bool flush) +{ + (void)task; (void)msg; (void)prio; (void)duration; (void)flush; +} + +int main(int argc, char **argv) +{ + const char *root = (argc > 1) ? argv[1] : "/tmp"; + char db_dir[512], core_dir[512], info_dir[512]; + char in_dir[512], pl_dir[512], p[1024]; + bool cache_supported = false; + /* Alpha lives in the first database, Beta in the second, so the + * second match promotes a database that was not already at the + * front. */ + const uint32_t crc_a = 0xA1B2C3D4u, crc_b = 0x5E6F7A8Bu; + const uint32_t sz_a = 4096, sz_b = 8192; + /* A cue sheet does not carry the checksum itself; the scanner + * parses it, finds the track it names and checksums that. This + * is the only coverage of that path. */ + const uint32_t crc_c = 0x0C0FFEE0u; + const uint32_t sz_c = 6144; + /* Inside an archive, so the scanner has to expand it and build a + * "archive#member" path for the entry. */ + const uint32_t crc_d = 0xD15CD15Cu; + const uint32_t sz_d = 2048; + + setvbuf(stdout, NULL, _IONBF, 0); + crc_init(); + verbosity_disable(); + + printf("scanner end-to-end test (dir: %s)\n\n", root); + + sprintf(db_dir, "%s/scan_db", root); + sprintf(core_dir, "%s/scan_cores", root); + sprintf(info_dir, "%s/scan_info", root); + sprintf(in_dir, "%s/scan_content", root); + sprintf(pl_dir, "%s/scan_playlist", root); + path_mkdir(db_dir); path_mkdir(core_dir); path_mkdir(info_dir); + path_mkdir(in_dir); path_mkdir(pl_dir); + + sprintf(p, "%s/Test Alpha.rdb", db_dir); + if (!write_db(p, "Alpha The Game", crc_a, sz_a)) + { check(0, "fixture", "could not write database"); return 1; } + sprintf(p, "%s/Test Beta.rdb", db_dir); + if (!write_db(p, "Beta The Game", crc_b, sz_b)) + { check(0, "fixture", "could not write database"); return 1; } + sprintf(p, "%s/Test Disc.rdb", db_dir); + if (!write_db(p, "Disc The Game", crc_c, sz_c)) + { check(0, "fixture", "could not write database"); return 1; } + sprintf(p, "%s/Test Zip.rdb", db_dir); + if (!write_db(p, "Zipped The Game", crc_d, sz_d)) + { check(0, "fixture", "could not write database"); return 1; } + + sprintf(p, "%s/02_alpha.bin", in_dir); + if (!write_content(p, crc_a, sz_a)) + { check(0, "fixture", "crc forcing failed"); return 1; } + sprintf(p, "%s/01_beta.bin", in_dir); + if (!write_content(p, crc_b, sz_b)) + { check(0, "fixture", "crc forcing failed"); return 1; } + /* The track the cue names, and the sheet itself. The scanner + * reads the sheet, resolves the track beside it and checksums + * that, so only the track carries the matching crc. */ + sprintf(p, "%s/03_disc.bin", in_dir); + if (!write_content(p, crc_c, sz_c)) + { check(0, "fixture", "crc forcing failed"); return 1; } + sprintf(p, "%s/03_disc.cue", in_dir); + { + FILE *f = fopen(p, "w"); + if (!f) + { check(0, "fixture", "could not write cue"); return 1; } + fprintf(f, + "FILE \"03_disc.bin\" BINARY\n" + " TRACK 01 MODE1/2352\n" + " INDEX 01 00:00:00\n"); + fclose(f); + } + + /* The archive itself checksums to something else entirely, so a + * match can only come from expanding it. */ + { + uint8_t *d = (uint8_t*)malloc(sz_d); + uint32_t i; + for (i = 0; i < sz_d - 4; i++) + d[i] = (uint8_t)((i * 53 + 7) & 0xFF); + crc_force(d, sz_d - 4, crc_d, d + sz_d - 4); + sprintf(p, "%s/04_bundle.zip", in_dir); + if (!write_zip(p, "inner.bin", d, sz_d, crc_d)) + { free(d); check(0, "fixture", "could not write zip"); return 1; } + free(d); + } + + check(1, "content forced to the databases' crcs", + "alpha, beta, a cue track and an archive member"); + + /* The scanner skips any database no installed core claims, so the + * core info has to name both databases as well as the extension - + * see core_info_database_supports_content_path(). Without the + * database line the scan reports no match for content whose crc is + * certainly present, which is easy to mistake for a lookup bug. */ + sprintf(p, "%s/test_libretro.info", info_dir); + { + FILE *f = fopen(p, "w"); + if (!f) + { check(0, "fixture", "could not write core info"); return 1; } + fprintf(f, + "display_name = \"Scan Test\"\n" + "corename = \"ScanTest\"\n" + "supported_extensions = \"bin|cue|zip\"\n" + "database = \"Test Alpha|Test Beta|Test Disc|Test Zip\"\n"); + fclose(f); + } + sprintf(p, "%s/test_libretro.so", core_dir); + { FILE *f = fopen(p, "wb"); if (f) { fputs("\177ELF", f); fclose(f); } } + +#ifdef HAVE_THREADS + task_queue_init(true, msgq_push); +#else + task_queue_init(false, msgq_push); +#endif + core_info_init_list(info_dir, core_dir, "so", true, false, + &cache_supported); + + strlcpy(config_get_ptr()->paths.directory_playlist, pl_dir, + sizeof(config_get_ptr()->paths.directory_playlist)); + strlcpy(config_get_ptr()->paths.path_content_database, db_dir, + sizeof(config_get_ptr()->paths.path_content_database)); + + /* Without this the scanner never opens an archive, and the entry + * inside is invisible. */ + { + bool *search_archives = manual_content_scan_get_search_archives_ptr(); + if (search_archives) + *search_archives = true; + } + + if (!manual_content_scan_set_menu_system_name( + MANUAL_CONTENT_SCAN_SYSTEM_NAME_CONTENT_DIR, NULL)) + check(0, "system name", "could not be set"); + + if (!task_push_dbscan(pl_dir, db_dir, in_dir, true, false, scan_cb)) + { + check(0, "scan started", "task_push_dbscan refused"); + goto done; + } + + { + time_t started = time(NULL); + while (loop_active) + { + task_queue_check(); + if (difftime(time(NULL), started) > SCAN_TIMEOUT_SECONDS) + break; + retro_sleep(1); + } + } + check(scan_completed, "scan ran to completion", + scan_completed ? "callback fired" : "timed out"); + + /* Each game must appear in its own database's playlist. Getting + * this wrong is not a crash - the entry is a real record from a + * real database, just the wrong one. */ + sprintf(p, "%s/Test Alpha.lpl", pl_dir); + check(file_contains(p, "Alpha The Game"), + "first database's game in its playlist", "Alpha The Game"); + check(!file_contains(p, "Beta The Game"), + "and not the other database's game", "no Beta in Alpha"); + + sprintf(p, "%s/Test Beta.lpl", pl_dir); + check(file_contains(p, "Beta The Game"), + "promoted database's game in its playlist", "Beta The Game"); + check(!file_contains(p, "Alpha The Game"), + "and not the other database's game", "no Alpha in Beta"); + + /* The cue is the entry that lands in the playlist, not the track + * it names: the scanner checksums the track but records the sheet, + * which is what a frontend would load. */ + sprintf(p, "%s/Test Zip.lpl", pl_dir); + check(file_contains(p, "Zipped The Game"), + "archive member matched", "Zipped The Game"); + /* The member is what gets checksummed and matched, but the entry + * records the archive: that is the path a frontend loads. Assert + * it rather than the "archive#member" form the scan builds + * internally, which does not reach the playlist. */ + check( file_contains(p, "04_bundle.zip") + && !file_contains(p, "#inner.bin"), + "entry records the archive, not the member", "04_bundle.zip"); + + sprintf(p, "%s/Test Disc.lpl", pl_dir); + check(file_contains(p, "Disc The Game"), + "cue resolved through its track", "Disc The Game"); + check(file_contains(p, ".cue"), + "playlist records the sheet, not the track", "path ends .cue"); + +done: + printf("\n%d checks, %d failures\n", checks, failures); + return failures ? 1 : 0; +} diff --git a/samples/tasks/database/main.c b/samples/tasks/database/main.c index 1084b57d2847..1b6a6afcc5ac 100644 --- a/samples/tasks/database/main.c +++ b/samples/tasks/database/main.c @@ -3,6 +3,7 @@ #include #include +#include #include "../../../core_info.h" #include "../../../tasks/tasks_internal.h" @@ -13,6 +14,7 @@ #include +#include "../../../list_special.h" #include "../../../manual_content_scan.h" #include "../../../configuration.h" #include "../../../verbosity.h" @@ -108,13 +110,22 @@ void task_window_progress_cb(retro_task_t *task) } /* dir_list_new_special lives in retroarch.c, which we cannot link - * without dragging in the world. manual_content_scan calls this to - * walk the scan directory; returning NULL causes the scan to bail - * without producing results, which is fine for a standalone demo. */ -void *dir_list_new_special(const char *input_dir, unsigned type, - const char *filter, bool show_hidden_files) + * without dragging in the world. + * + * Returning NULL here meant the scan never enumerated its databases, + * so it could not finish, and everything downstream of a completed + * scan - notably the task teardown - went unexercised. The scanner + * only asks for DIR_LIST_DATABASES, which retroarch.c answers with a + * plain "rdb" listing, so that much is worth providing. */ +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; + (void)filter; + + if (type == DIR_LIST_DATABASES) + return dir_list_new(input_dir, "rdb", false, show_hidden_files, + false, false); + return NULL; } @@ -194,12 +205,28 @@ int main(int argc, char *argv[]) #else task_queue_init(false /* threaded enable */, main_msg_queue_push); #endif - core_info_init_list(core_info_dir, core_dir, exts, true, false, NULL); + { + /* core_info_list_new() writes through this unconditionally, so + * it cannot be NULL - passing one crashed the sample before it + * reached the scan. */ + bool cache_supported = false; + core_info_init_list(core_info_dir, core_dir, exts, true, false, + &cache_supported); + } - /* The scan reads its playlist directory from settings, not from the - * argument below, so put it where the scan will look. */ + /* task_push_dbscan() ignores its playlist_directory and + * content_database arguments - both are marked "always from + * settings" and the scan reads them from there. Passing them and + * not setting the settings left the database path empty, so the + * scan had nothing to match against and never produced a result: + * + * [Scanner] ""... + * + * is that empty path being logged. */ strlcpy(config_get_ptr()->paths.directory_playlist, playlist_dir, sizeof(config_get_ptr()->paths.directory_playlist)); + strlcpy(config_get_ptr()->paths.path_content_database, db_dir, + sizeof(config_get_ptr()->paths.path_content_database)); /* A scan needs a system name to build its playlist from. Without * one manual_content_scan_get_task_config refuses and no task is diff --git a/tasks/task_database.c b/tasks/task_database.c index d403181be620..268e61466844 100644 --- a/tasks/task_database.c +++ b/tasks/task_database.c @@ -21,6 +21,7 @@ #include #include #include +#include #include #include #include @@ -46,8 +47,6 @@ #include "../verbosity.h" #include "task_database_cue.h" -#define MAX_DATABASE_COUNT 256 - /* Scan result structure for accumulating identification results */ typedef struct scan_result { @@ -162,9 +161,61 @@ enum db_state_flags_enum DB_STATE_FLAG_HAS_SERIAL = (1 << 0), DB_STATE_FLAG_HAS_CRC = (1 << 1), DB_STATE_FLAG_HAS_SIZE = (1 << 2), - DB_STATE_FLAG_MATCHED = (1 << 3) + DB_STATE_FLAG_MATCHED = (1 << 3), + /* Set once the size range for a database has been queried, + * whatever the answer was. The probe used to key off + * "min_sizes[i] == 0", which is also what an unqueried slot holds + * and what a database whose smallest record is zero-sized + * legitimately produces - so such a database was re-queried for + * every content file, at two full walks a time. */ + DB_STATE_FLAG_SIZE_CHECKED = (1 << 4) }; +/* Ceiling on the crc and serial indexes a single scan may hold. + * + * Taken as a share of what is actually free rather than from a + * platform list: mem_stats_free() has implementations for the 3DS and + * the GameCube/Wii among others, which are the platforms where this + * matters, and task_audio_mixer_threshold() already sizes a + * task-lifetime allocation the same way. + * + * An eighth is deliberately more conservative than the quarter the + * mixer uses, because these indexes are held for the whole scan + * rather than consulted once. The cap keeps a large-memory host from + * hoarding: the biggest database shipped today indexes to 237 KB + * across roughly 30000 records, so 32 MB covers far more databases + * than exist. Below the + * floor there is no point starting, and the query path is only + * slower, never wrong. */ +#define DB_STATE_INDEX_BUDGET_SHARE 8 +#define DB_STATE_INDEX_BUDGET_CAP (32 * 1024 * 1024) +#define DB_STATE_INDEX_BUDGET_FLOOR (256 * 1024) + +/* Used when mem_stats_free() has no implementation for the platform + * and returns 0. Small enough to be harmless where memory is tight, + * since the only cost of running out is falling back to querying. */ +#ifndef DB_STATE_INDEX_BUDGET_UNKNOWN +#define DB_STATE_INDEX_BUDGET_UNKNOWN (2 * 1024 * 1024) +#endif + +static size_t task_database_index_budget(void) +{ + uint64_t free_mem = mem_stats_free(); + uint64_t share; + + if (!free_mem) + return (size_t)DB_STATE_INDEX_BUDGET_UNKNOWN; + + share = free_mem / DB_STATE_INDEX_BUDGET_SHARE; + + if (share > (uint64_t)DB_STATE_INDEX_BUDGET_CAP) + share = (uint64_t)DB_STATE_INDEX_BUDGET_CAP; + if (share < (uint64_t)DB_STATE_INDEX_BUDGET_FLOOR) + return 0; + + return (size_t)share; +} + typedef struct database_state_handle { database_info_list_t *info; @@ -178,9 +229,29 @@ typedef struct database_state_handle uint64_t archive_size; char archive_name[512]; /* TODO/FIXME - check size */ char serial[4096]; /* TODO/FIXME - check size */ - int64_t min_sizes[MAX_DATABASE_COUNT]; - int64_t max_sizes[MAX_DATABASE_COUNT]; - uint8_t flags[MAX_DATABASE_COUNT]; + /* One entry per database in 'list'. These used to be + * [MAX_DATABASE_COUNT] arrays indexed by list_index, which is + * bounded only by list->size - the number of .rdb files in the + * database directory. Nothing clamped it, so a database + * directory with more than MAX_DATABASE_COUNT entries wrote past + * all three arrays, and the shuffle in + * database_info_list_iterate_found_match() memmove()d past them + * as well. The shipped set is already over half that limit and + * grows every release. Allocated by + * task_database_state_alloc_arrays(). */ + int64_t *min_sizes; + int64_t *max_sizes; + uint8_t *flags; + /* One crc index per database, built the first time that database + * is probed and kept for the rest of the scan. Without it every + * (content file, database) pair is a full walk of the database. */ + database_info_crc_index_t **crc_index; + /* Likewise for the serial lookup disc content uses. */ + database_info_serial_index_t **serial_index; + /* Bytes of index the scan may still allocate. Indexes are held + * for the whole scan, so without a ceiling a large database set + * costs tens of megabytes. See task_database_index_budget(). */ + size_t index_budget; } database_state_handle_t; enum db_flags_enum @@ -488,29 +559,49 @@ static bool is_valid_disc_indicator(const char *str, size_t len) static void remove_disc_indicators(char *title, size_t len) { + size_t i; char *disc_pos = NULL; + size_t prefix_len = 0; + /* Tape and floppy releases usually do not follow the naming + * convention, so their prefixes skip the leading space - which + * makes them six characters rather than seven. The old code + * skipped a hard-coded seven for all of them, so for "(Tape 1)" + * the indicator was taken to start at the ')' and came out empty: + * is_valid_disc_indicator() rejected it and no tape or side + * indicator was ever stripped. Carry each prefix's own length. */ + static const struct + { + const char *pat; + size_t len; + } patterns[] = { + { " (Disc ", 7 }, { " (disc ", 7 }, + { " (Disk ", 7 }, { " (disk ", 7 }, + { "(Tape ", 6 }, { "(tape ", 6 }, + { "(Side ", 6 }, { "(side ", 6 } + }; + + for (i = 0; i < ARRAY_SIZE(patterns); i++) + { + if ((disc_pos = strstr(title, patterns[i].pat))) + { + prefix_len = patterns[i].len; + break; + } + } - /* Search for common disc patterns */ - if ( (disc_pos = strstr(title, " (Disc ")) - || (disc_pos = strstr(title, " (disc ")) - || (disc_pos = strstr(title, " (Disk ")) - || (disc_pos = strstr(title, " (disk ")) - /* Tape and floppy releases usually do not follow naming convention, so skip the space */ - || (disc_pos = strstr(title, "(Tape ")) - || (disc_pos = strstr(title, "(tape ")) - || (disc_pos = strstr(title, "(Side ")) - || (disc_pos = strstr(title, "(side "))) + if (disc_pos) { /* Find the closing parenthesis */ char *end_pos = strchr(disc_pos, ')'); if (end_pos) { - /* Extract the disc indicator text (between " (Disc " and ")") */ - const char *indicator_start = disc_pos + 7; /* Skip " (Disc " */ - size_t indicator_len = end_pos - indicator_start; + /* Extract the indicator text between the prefix and ")" */ + const char *indicator_start = disc_pos + prefix_len; + size_t indicator_len = (size_t)(end_pos - indicator_start); /* Validate this is actually a disc indicator, not arbitrary text */ - if (is_valid_disc_indicator(indicator_start, indicator_len)) + if ( end_pos >= indicator_start + && is_valid_disc_indicator(indicator_start, indicator_len)) { /* Truncate at the disc indicator */ *disc_pos = '\0'; @@ -682,6 +773,11 @@ static void gdi_prune(struct string_list *list, const char *name) } } + /* task_database_cue_prune() above closes the stream before freeing + * the handle; this one only freed the handle, leaking the OS file + * descriptor and the inner stream object for every .gdi in the + * scan. A large GDI collection exhausts the descriptor table. */ + intfstream_close(fd); free(fd); } @@ -691,30 +787,36 @@ static enum msg_file_type extension_to_file_type(const char *ext) strlcpy(ext_lower, ext, sizeof(ext_lower)); string_to_lower(ext_lower); + /* These were memcmp() against a fixed count, which is specified to + * read all n bytes. For an extension shorter than the count - an + * empty extension being the common case - those bytes are + * uninitialised stack, which MSan reports and which vectorised + * memcmp() implementations really do load. string_is_equal() + * stops at the terminator. */ if ( - memcmp(ext_lower, "7z", 3) == 0 - || memcmp(ext_lower, "zip", 4) == 0 - || memcmp(ext_lower, "apk", 4) == 0 - || memcmp(ext_lower, "zst", 4) == 0 + string_is_equal(ext_lower, "7z") + || string_is_equal(ext_lower, "zip") + || string_is_equal(ext_lower, "apk") + || string_is_equal(ext_lower, "zst") ) return FILE_TYPE_COMPRESSED; - if (memcmp(ext_lower, "cue", 4) == 0) + if (string_is_equal(ext_lower, "cue")) return FILE_TYPE_CUE; - if (memcmp(ext_lower, "gdi", 4) == 0) + if (string_is_equal(ext_lower, "gdi")) return FILE_TYPE_GDI; - if (memcmp(ext_lower, "iso", 4) == 0) + if (string_is_equal(ext_lower, "iso")) return FILE_TYPE_ISO; - if (memcmp(ext_lower, "chd", 4) == 0) + if (string_is_equal(ext_lower, "chd")) return FILE_TYPE_CHD; - if (memcmp(ext_lower, "wbfs", 5) == 0) + if (string_is_equal(ext_lower, "wbfs")) return FILE_TYPE_WBFS; - if (memcmp(ext_lower, "rvz", 4) == 0) + if (string_is_equal(ext_lower, "rvz")) return FILE_TYPE_RVZ; - if (memcmp(ext_lower, "wia", 4) == 0) + if (string_is_equal(ext_lower, "wia")) return FILE_TYPE_WIA; - if (memcmp(ext_lower, "pbp", 4) == 0) + if (string_is_equal(ext_lower, "pbp")) return FILE_TYPE_PBP; - if (memcmp(ext_lower, "lutro", 6) == 0) + if (string_is_equal(ext_lower, "lutro")) return FILE_TYPE_LUTRO; return FILE_TYPE_NONE; } @@ -835,9 +937,14 @@ static bool add_files_from_archive(manual_scan_handle_t *_db, char new_path[PATH_MAX_LENGTH]; strlcpy(new_path, path, sizeof(new_path)); new_path[_len] = '#'; + /* The copy starts at _len + 1, so the space left is + * sizeof(new_path) - _len - 1. The bound said - _len, + * which permits one byte past the end. The enclosing + * length test happens to keep that unreachable today; + * the two must agree regardless. */ strlcpy(new_path + _len + 1, archive_list->elems[i].data, - sizeof(new_path) - _len); + sizeof(new_path) - _len - 1); string_list_append(_db->content_list, new_path, archive_list->elems[i].attr); } @@ -1064,6 +1171,27 @@ static enum scan_verdict database_info_list_iterate_found_match( &db_state->flags[0], sizeof(flag) * db_state->list_index); + /* The index caches are keyed by the same position, so they have + * to travel with the entry. Leaving them behind pairs a + * database with another database's index, and the lookup then + * answers with records that belong to a different system. */ + { + database_info_crc_index_t *ci = + db_state->crc_index[db_state->list_index]; + database_info_serial_index_t *si = + db_state->serial_index[db_state->list_index]; + + memmove(&db_state->crc_index[1], + &db_state->crc_index[0], + sizeof(ci) * db_state->list_index); + memmove(&db_state->serial_index[1], + &db_state->serial_index[0], + sizeof(si) * db_state->list_index); + + db_state->crc_index[0] = ci; + db_state->serial_index[0] = si; + } + db_state->list->elems[0] = entry; db_state->min_sizes[0] = min; db_state->max_sizes[0] = max; @@ -1092,21 +1220,130 @@ static enum scan_verdict database_info_list_iterate_next( return SCAN_VERDICT_CONTINUE; } +/* Allocate the per-database size/flag caches once the database list + * is known. Returns false on OOM; the caller aborts the scan. */ +static bool task_database_state_alloc_arrays( + database_state_handle_t *db_state) +{ + size_t count; + + if (!db_state || !db_state->list) + return false; + + count = db_state->list->size; + + /* An empty database directory is not an error: every lookup path + * bails on "list_index == list->size" before touching these. */ + if (count == 0) + return true; + + db_state->min_sizes = (int64_t*)calloc(count, sizeof(int64_t)); + db_state->max_sizes = (int64_t*)calloc(count, sizeof(int64_t)); + db_state->flags = (uint8_t*)calloc(count, sizeof(uint8_t)); + db_state->crc_index = (database_info_crc_index_t**) + calloc(count, sizeof(*db_state->crc_index)); + db_state->serial_index = (database_info_serial_index_t**) + calloc(count, sizeof(*db_state->serial_index)); + db_state->index_budget = task_database_index_budget(); + + if ( !db_state->min_sizes + || !db_state->max_sizes + || !db_state->flags + || !db_state->crc_index + || !db_state->serial_index) + return false; + + return true; +} + static void task_database_fill_db_min_max(database_state_handle_t *db_state) { char query[50]; query[0] = '\0'; + /* Marked up front so that every exit below - including the ones + * that record a placeholder range - counts as having been asked. + * This is what stops the two walks repeating per content file. */ + db_state->flags[db_state->list_index] |= DB_STATE_FLAG_SIZE_CHECKED; + + /* The crc index collects the size range while it walks, so build + * it here and take the range from it: one walk instead of this + * function's two, and the crc lookup then finds the index already + * built. The queries below still run for a database the index + * cannot cover. */ + { + const char *rdb = db_state->list->elems[db_state->list_index].data; + int64_t lo = 0; + int64_t hi = 0; + + if (rdb && *rdb) + { + if ( !db_state->crc_index[db_state->list_index] + && db_state->index_budget) + { + database_info_crc_index_t *ci = + database_info_crc_index_new(rdb, db_state->index_budget); + if (ci) + { + size_t used = database_info_crc_index_bytes(ci); + db_state->index_budget = (used < db_state->index_budget) + ? db_state->index_budget - used : 0; + db_state->crc_index[db_state->list_index] = ci; + } + } + + if ( db_state->crc_index[db_state->list_index] + && database_info_crc_index_size_range( + db_state->crc_index[db_state->list_index], &lo, &hi)) + { + db_state->min_sizes[db_state->list_index] = lo; + db_state->max_sizes[db_state->list_index] = hi; + db_state->flags[db_state->list_index] |= + DB_STATE_FLAG_HAS_SIZE; + + /* HAS_SERIAL gates a skip in the serial lookup, and this + * walk does not observe serial keys - it scans crc with + * size alongside. Set it rather than leave it clear: + * clear would skip every database for disc content, which + * is a missed match, while set costs at most one wasted + * serial-index build for a database that carries none. + * Every database shipped today carries serials. + * + * HAS_CRC is set for symmetry; nothing reads it beyond a + * debug line. */ + db_state->flags[db_state->list_index] |= + DB_STATE_FLAG_HAS_CRC | DB_STATE_FLAG_HAS_SERIAL; + + db_state->entry_index = 0; + return; + } + } + } + snprintf(query, sizeof(query), "{size:min(0)}"); database_info_list_iterate_new(db_state, query); + /* database_info_list_new_filtered() returns NULL when the .rdb + * cannot be opened, when the query fails to compile, or on OOM, so + * db_state->info is not guaranteed here. Treat a failed query the + * same way an empty result is treated below: mark the database as + * having no usable size range and move on. */ + if (!db_state->info) + { + db_state->min_sizes[db_state->list_index] = -1; + db_state->max_sizes[db_state->list_index] = -1; + db_state->entry_index = 0; + return; + } + if (db_state->info->count > 0) { db_state->min_sizes[db_state->list_index] = db_state->info->list[db_state->info->count-1].size; snprintf(query, sizeof(query), "{size:max(0)}"); database_info_list_iterate_new(db_state, query); - if (db_state->info->count > 0) + /* Second query, same failure modes as the first. */ + if (db_state->info && db_state->info->count > 0) { size_t i; db_state->max_sizes[db_state->list_index] = db_state->info->list[db_state->info->count-1].size; @@ -1176,7 +1413,7 @@ static enum scan_verdict task_database_iterate_crc_lookup( } /* If size boundaries are not filled for this DB, run the queries */ - if (db_state->min_sizes[db_state->list_index] == 0) + if (!(db_state->flags[db_state->list_index] & DB_STATE_FLAG_SIZE_CHECKED)) task_database_fill_db_min_max(db_state); if (db_state->min_sizes[db_state->list_index] > 0) @@ -1219,6 +1456,9 @@ static enum scan_verdict task_database_iterate_crc_lookup( if (db_state->entry_index == 0) { char query[50]; + const char *rdb = db_state->list->elems[ + db_state->list_index].data; + database_info_list_t *hits = NULL; query[0] = '\0'; @@ -1241,20 +1481,69 @@ static enum scan_verdict task_database_iterate_crc_lookup( } } - snprintf(query, sizeof(query), - "{crc:or(b\"%08lX\",b\"%08lX\")}", - (unsigned long)db_state->crc, (unsigned long)db_state->archive_crc); + /* Answer from this database's crc index when we can. Building + * it costs one walk - about what a single probe used to cost - + * and every later content file is then a binary search instead + * of another walk. The index is only a faster route to the + * same records: it reports them in file order with the same + * fields extracted, so the matching below is unchanged. + * + * Anything the index cannot serve falls through to the query, + * which stays the reference path. */ + if (rdb && *rdb) + { + if ( !db_state->crc_index[db_state->list_index] + && db_state->index_budget) + { + database_info_crc_index_t *ci = + database_info_crc_index_new(rdb, db_state->index_budget); + if (ci) + { + size_t used = database_info_crc_index_bytes(ci); + db_state->index_budget = (used < db_state->index_budget) + ? db_state->index_budget - used : 0; + db_state->crc_index[db_state->list_index] = ci; + } + } + + if (db_state->crc_index[db_state->list_index]) + hits = database_info_list_new_crc( + db_state->crc_index[db_state->list_index], rdb, + db_state->crc, db_state->archive_crc, + DB_EXTRACT_SCAN_FIELDS); + } + + if (hits) + { + if (db_state->info) + { + database_info_list_free(db_state->info); + free(db_state->info); + } + db_state->info = hits; + } + else + { + snprintf(query, sizeof(query), + "{crc:or(b\"%08lX\",b\"%08lX\")}", + (unsigned long)db_state->crc, + (unsigned long)db_state->archive_crc); - database_info_list_iterate_new(db_state, query); + database_info_list_iterate_new(db_state, query); + } } - if (db_state->info) + /* Same shape as the serial lookup below: entry_index was used to + * index the list without checking it against count, so a query + * that matched nothing (count == 0, list either empty or NULL) + * still had list[0] dereferenced. */ + if (db_state->info && db_state->entry_index < db_state->info->count) { database_info_t *db_info_entry = &db_state->info->list[db_state->entry_index]; /* When scanning an archive, "first" file crc32 is also checked. */ - if (db_info_entry && db_info_entry->crc32) + if (db_info_entry->crc32) { if (db_state->archive_crc == db_info_entry->crc32) return database_info_list_iterate_found_match( @@ -1360,7 +1649,7 @@ static int task_database_iterate_serial_lookup( path_contains_compressed_file); /* If size boundaries are not filled for this DB, run the queries */ - if (db_state->min_sizes[db_state->list_index] == 0) + if (!(db_state->flags[db_state->list_index] & DB_STATE_FLAG_SIZE_CHECKED)) task_database_fill_db_min_max(db_state); if (db_state->min_sizes[db_state->list_index] > 0) @@ -1411,36 +1700,120 @@ static int task_database_iterate_serial_lookup( if (db_state->entry_index == 0) { - size_t _len; - char query[50]; - char *serial_buf = bin_to_hex_alloc( + size_t query_len; + char query_buf[128]; + char *query = query_buf; + char *serial_buf; + const char *rdb = db_state->list->elems[db_state->list_index].data; + database_info_list_t *hits = NULL; + + /* Same treatment as the crc path: one walk builds this + * database's serial index, and every content file after that is + * a lookup instead of another walk. The index reports the same + * records in the same order with the same fields extracted, and + * returns NULL for anything it cannot serve so the query below + * still runs. */ + if (rdb && *rdb) + { + if ( !db_state->serial_index[db_state->list_index] + && db_state->index_budget) + { + database_info_serial_index_t *si = + database_info_serial_index_new(rdb, db_state->index_budget); + if (si) + { + size_t used = database_info_serial_index_bytes(si); + db_state->index_budget = (used < db_state->index_budget) + ? db_state->index_budget - used : 0; + db_state->serial_index[db_state->list_index] = si; + } + } + + if (db_state->serial_index[db_state->list_index]) + hits = database_info_list_new_serial( + db_state->serial_index[db_state->list_index], rdb, + db_state->serial, DB_EXTRACT_SCAN_FIELDS); + } + + if (hits) + { + if (db_state->info) + { + database_info_list_free(db_state->info); + free(db_state->info); + } + db_state->info = hits; + goto serial_query_done; + } + + serial_buf = bin_to_hex_alloc( (uint8_t*)db_state->serial, strlen(db_state->serial) * sizeof(uint8_t)); if (!serial_buf) return SCAN_VERDICT_ERROR; - _len = strlcpy(query, "{'serial': b'", sizeof(query)); - _len += strlcpy(query + _len, serial_buf, sizeof(query) - _len); - query[ _len] = '\''; - query[++_len] = '}'; - query[++_len] = '\0'; + /* strlcpy() returns the length of its *source*, not the number + * of bytes it copied, so the previous form advanced _len by the + * full hex expansion even when the copy had been truncated to + * fit. The three stores that follow then landed at + * query[13 + 2 * strlen(serial)], past the end of a 50 byte + * buffer for any serial longer than 17 characters: + * + * AddressSanitizer: stack-buffer-overflow + * WRITE of size 1 + * + * db_state->serial is 4 KiB, and detect_dc_game() alone + * concatenates lgame_id[20] and rgame_id[20] before appending a + * disc suffix, so it can hand us close to forty characters. + * + * Size the buffer from the string being built instead, keeping + * the common case on the stack and falling back to the heap for + * a serial that does not fit - the same pattern + * database_info_list_iterate_found_match() uses for db_crc. */ + query_len = STRLEN_CONST("{'serial': b'") + + strlen(serial_buf) + + STRLEN_CONST("'}") + 1; + + if (query_len > sizeof(query_buf)) + query = (char*)malloc(query_len); + + if (!query) + { + free(serial_buf); + return SCAN_VERDICT_ERROR; + } + + snprintf(query, query_len, "{'serial': b'%s'}", serial_buf); #ifdef DEBUG RARCH_DBG("[Scanner] Serial orig / decoded: \"%s\" / \"%s\".\n", db_state->serial, serial_buf); #endif database_info_list_iterate_new(db_state, query); + if (query != query_buf) + free(query); free(serial_buf); + +serial_query_done: + ; } if (db_state->info) { - while (db_state->entry_index <= db_state->info->count) + /* "<=" walked one element past the end of the list and then + * dereferenced it: db_info_entry->serial reads a pointer out of + * whatever follows the allocation and hands it to + * string_is_equal(). The list is empty on the common no-match + * path, so this fired on ordinary scans, not just crafted ones. + * + * The "db_info_entry &&" test below was dead - the address of an + * array element is never NULL - and is dropped with it. */ + while (db_state->entry_index < db_state->info->count) { database_info_t *db_info_entry = &db_state->info->list[ db_state->entry_index]; - if (db_info_entry && db_info_entry->serial) + if (db_info_entry->serial) { if (string_is_equal(db_state->serial, db_info_entry->serial)) { @@ -1570,12 +1943,29 @@ static void scan_results_batch_update_playlists(scan_results_t *sr, { scan_result_t *result = &sr->results[i]; char db_name_noext[PATH_MAX_LENGTH]; + /* The key identifying which playlist this result belongs to. + * With a fixed playlist file every result goes to the same + * playlist, so the key is that path; otherwise results are + * grouped by database name. + * + * This used to compare current_playlist against result->db_name + * unconditionally, but the fixed-file branch below assigns + * task_config->playlist_file to current_playlist - a path like + * ".../MyList.lpl", which never equals a db_name, those being + * the database's own file name. The test was therefore true on every + * iteration, and each result closed the playlist (a full + * playlist_write_file()) and reopened it (a full + * playlist_init() parse from disk). N results meant N complete + * loads and N complete writes of the same file. */ + const char *group_key = (*manual_scan->task_config->playlist_file) + ? manual_scan->task_config->playlist_file + : result->db_name; strlcpy(db_name_noext, result->db_name, sizeof(db_name_noext)); path_remove_extension(db_name_noext); /* Check if we need to switch to a different playlist */ - if (!single_playlist && (!current_playlist || !string_is_equal(current_playlist, result->db_name))) + if (!single_playlist && (!current_playlist || !string_is_equal(current_playlist, group_key))) { /* Write and close previous playlist if any */ if (playlist) @@ -1605,6 +1995,18 @@ static void scan_results_batch_update_playlists(scan_results_t *sr, playlist = playlist_init(&manual_scan->playlist_config); + /* Check before use: the playlist_set_scan_* calls below are + * not all NULL-guarded (playlist_set_scan_search_recursively, + * playlist_set_sort_mode, playlist_qsort, playlist_write_file + * and several others dereference unconditionally). The test + * used to sit after all of them. */ + if (!playlist) + { + RARCH_ERR("[Scanner] Failed to open playlist: \"%s\".\n", result->db_name); + current_playlist = NULL; + continue; + } + /* Set default core, if required */ if (manual_scan->task_config->core_set) { @@ -1640,12 +2042,6 @@ static void scan_results_batch_update_playlists(scan_results_t *sr, playlist_set_scan_omit_db_ref(playlist, manual_scan->task_config->omit_db_reference); - if (!playlist) - { - RARCH_ERR("[Scanner] Failed to open playlist: \"%s\".\n", result->db_name); - continue; - } - RARCH_LOG("[Scanner] Processing playlist: \"%s\".\n", result->db_name); } @@ -1706,7 +2102,12 @@ static void scan_results_batch_update_playlists(scan_results_t *sr, playlist_set_sort_mode(playlist, PLAYLIST_SORT_MODE_DEFAULT); playlist_qsort(playlist); playlist_write_file(playlist); - if (!string_is_equal(current_playlist, manual_scan->task_config->playlist_file)) + /* Free whatever this function opened. Only the single-playlist + * path borrows manual_scan->playlist, which the handle teardown + * owns; comparing the handles says that exactly, where the + * previous string compare against playlist_file also matched + * the playlist this function had opened itself and leaked it. */ + if (playlist != manual_scan->playlist) playlist_free(playlist); } @@ -1785,8 +2186,52 @@ static void free_manual_content_scan_handle(manual_scan_handle_t *manual_scan) if (dbstate) { + /* The per-database arrays are sized from the list, so the + * count has to be taken before the list goes away. Reading + * it afterwards is a use-after-free, and the value it + * returns then drives the loops below. */ + size_t db_count = dbstate->list ? dbstate->list->size : 0; + if (dbstate->list) + { dir_list_free(dbstate->list); + dbstate->list = NULL; + } + /* db_state->info is only released along the iterate paths, + * so cancelling a scan while a database query result was + * live leaked the whole database_info_list_t - every + * strdup'd field of every matched record. */ + if (dbstate->info) + { + database_info_list_free(dbstate->info); + free(dbstate->info); + dbstate->info = NULL; + } + if (dbstate->crc_index) + { + size_t ci; + for (ci = 0; ci < db_count; ci++) + database_info_crc_index_free(dbstate->crc_index[ci]); + free(dbstate->crc_index); + dbstate->crc_index = NULL; + } + if (dbstate->serial_index) + { + size_t si; + for (si = 0; si < db_count; si++) + database_info_serial_index_free(dbstate->serial_index[si]); + free(dbstate->serial_index); + dbstate->serial_index = NULL; + } + if (dbstate->min_sizes) + free(dbstate->min_sizes); + if (dbstate->max_sizes) + free(dbstate->max_sizes); + if (dbstate->flags) + free(dbstate->flags); + dbstate->min_sizes = NULL; + dbstate->max_sizes = NULL; + dbstate->flags = NULL; } if ( manual_scan->content_database_path @@ -1856,14 +2301,21 @@ static void cb_task_manual_content_scan( #if defined(HAVE_MENU) end: +#endif /* The caller's callback, if it gave one. Read before the handle is - * released below; the menu refresh that follows is the task's own - * and is deliberately kept, because three of the four in-tree - * callers rely on it happening whether or not they pass a callback - * of their own. */ + * released below. + * + * This used to sit inside the HAVE_MENU block along with the menu + * refresh, so a build without menu support ran the scan and then + * dropped the callback: a caller waiting on it waited forever. + * The in-tree callers only supply one under HAVE_MENU themselves, + * which is why nothing noticed, but the parameter is not + * documented as menu-only and the sample in samples/tasks/database + * hangs on exactly this. */ if (manual_scan && manual_scan->user_cb) manual_scan->user_cb(task, task_data, user_data, err); +#if defined(HAVE_MENU) /* When creating playlists, the playlist tabs of * any active menu driver must be refreshed */ if ( @@ -2027,6 +2479,16 @@ static void task_manual_content_scan_handler(retro_task_t *task) manual_scan->flags & DB_HANDLE_FLAG_SHOW_HIDDEN_FILES, false, false); } + + /* Size the per-database size/flag caches to the + * database list we just built. Both branches + * above land here. */ + if ( dbstate->list + && !task_database_state_alloc_arrays(dbstate)) + { + RARCH_ERR("[Scanner] Out of memory allocating database state\n"); + goto task_finished; + } } RARCH_LOG("[Scanner] %s\"%s\"...\n", msg_hash_to_str(MSG_MANUAL_CONTENT_SCAN_START), manual_scan->content_database_path); @@ -2162,7 +2624,8 @@ static void task_manual_content_scan_handler(retro_task_t *task) msg_hash_to_str(MSG_MANUAL_CONTENT_SCAN_PLAYLIST_CLEANUP), sizeof(task_title)); - if ( (entry->path && *entry->path) + if ( _len < sizeof(task_title) + && (entry->path && *entry->path) && (entry_file = path_basename(entry->path))) strlcpy(task_title + _len, entry_file, @@ -2326,7 +2789,7 @@ static void task_manual_content_scan_handler(retro_task_t *task) msg_hash_to_str(MSG_MANUAL_CONTENT_SCAN_IN_PROGRESS), sizeof(task_title)); - if (content_file && *content_file) + if (_len < sizeof(task_title) && content_file && *content_file) strlcpy(task_title + _len, content_file, sizeof(task_title) - _len); @@ -2443,7 +2906,7 @@ static void task_manual_content_scan_handler(retro_task_t *task) msg_hash_to_str(MSG_MANUAL_CONTENT_SCAN_M3U_CLEANUP), sizeof(task_title)); - if (m3u_name && *m3u_name) + if (_len < sizeof(task_title) && m3u_name && *m3u_name) strlcpy(task_title + _len, m3u_name, sizeof(task_title) - _len); @@ -2644,13 +3107,23 @@ bool task_push_manual_content_scan( if (!(task = task_init())) goto error; - /* > Get task title */ + /* > Get task title + * + * strlcpy() returns the length of its source, so _len can exceed + * the buffer when the (translated) message does not fit. The + * append would then form an out-of-bounds pointer and pass a + * wrapped size_t as the bound. The shipped strings leave a wide + * margin - the longest of these four is 26 bytes into 128 - so + * this is hardening rather than a live overflow, but it is the + * same misuse of the return value that overflowed the serial + * query buffer, and translations come from Crowdin. */ _len = strlcpy( task_title, msg_hash_to_str(MSG_MANUAL_CONTENT_SCAN_START), sizeof(task_title)); - strlcpy(task_title + _len, - manual_scan->task_config->system_name, - sizeof(task_title) - _len); + if (_len < sizeof(task_title)) + strlcpy(task_title + _len, + manual_scan->task_config->system_name, + sizeof(task_title) - _len); /* > Configure task */ task->handler = task_manual_content_scan_handler; diff --git a/tasks/task_database_cue.c b/tasks/task_database_cue.c index bb7c41b97763..2b25f39613f3 100644 --- a/tasks/task_database_cue.c +++ b/tasks/task_database_cue.c @@ -113,7 +113,13 @@ static int cue_find_disc_number(const char* str1, char disc) /** * Given a title and filename, append the appropriate disc number to it. */ -static void cue_append_multi_disc_suffix(char * str1, const char *filename) +/* @len is the size of the @str1 buffer. This used to bound the + * append with "PATH_MAX_LENGTH - __len" regardless of what was + * actually passed: detect_gc_game() hands it a char[20], so the + * snprintf() was told it had four kilobytes of a twenty byte buffer. + * Only the single-character disc index kept that from overflowing. */ +static void cue_append_multi_disc_suffix(char * str1, size_t len, + const char *filename) { /* Check multi-disc and insert suffix */ int result = string_find_index_substring_string(filename, "(Disc "); @@ -130,7 +136,8 @@ static void cue_append_multi_disc_suffix(char * str1, const char *filename) { char *dest = str1; size_t __len = strlen(dest); - snprintf(dest + __len, PATH_MAX_LENGTH - __len, "-%i", disc_number - 1); + if (__len < len) + snprintf(dest + __len, len - __len, "-%i", disc_number - 1); } } } @@ -240,7 +247,7 @@ int detect_ps1_game(intfstream_t *fd, char *s, size_t len, } raw_game_id[10] = '\0'; string_remove_all_whitespace(s, raw_game_id); - cue_append_multi_disc_suffix(s, filename); + cue_append_multi_disc_suffix(s, len, filename); free(disc_data); return 1; } @@ -249,7 +256,7 @@ int detect_ps1_game(intfstream_t *fd, char *s, size_t len, { raw_game_id[10] = '\0'; string_remove_all_whitespace(s, raw_game_id); - cue_append_multi_disc_suffix(s, filename); + cue_append_multi_disc_suffix(s, len, filename); free(disc_data); return 1; } @@ -257,7 +264,7 @@ int detect_ps1_game(intfstream_t *fd, char *s, size_t len, { raw_game_id[7] = '\0'; string_remove_all_whitespace(s, raw_game_id); - cue_append_multi_disc_suffix(s, filename); + cue_append_multi_disc_suffix(s, len, filename); free(disc_data); return 0; } @@ -274,7 +281,7 @@ int detect_ps1_game(intfstream_t *fd, char *s, size_t len, s[8 ] = 'X'; s[9 ] = 'X'; s[10] = '\0'; - cue_append_multi_disc_suffix(s, filename); + cue_append_multi_disc_suffix(s, len, filename); free(disc_data); return 0; } @@ -389,7 +396,7 @@ int detect_ps2_game(intfstream_t *fd, char *s, size_t len, raw_game_id[9] = '3'; string_remove_all_whitespace(s, raw_game_id); - cue_append_multi_disc_suffix(s, filename); + cue_append_multi_disc_suffix(s, len, filename); free(disc_data); return 1; } @@ -406,7 +413,7 @@ int detect_ps2_game(intfstream_t *fd, char *s, size_t len, s[8 ] = 'X'; s[9 ] = 'X'; s[10] = '\0'; - cue_append_multi_disc_suffix(s, filename); + cue_append_multi_disc_suffix(s, len, filename); free(disc_data); return 0; } @@ -467,7 +474,7 @@ int detect_psp_game(intfstream_t *fd, char *s, size_t len, ) { strlcpy(s, p, MIN(len, 11)); - cue_append_multi_disc_suffix(s, filename); + cue_append_multi_disc_suffix(s, len, filename); free(disc_data); return 1; } @@ -630,7 +637,7 @@ int detect_pbp_game(intfstream_t *fd, char *s, size_t len, raw_id[10] = '\0'; string_remove_all_whitespace(s, raw_id); - cue_append_multi_disc_suffix(s, filename); + cue_append_multi_disc_suffix(s, len, filename); found = 1; } @@ -691,7 +698,7 @@ size_t detect_gc_game(intfstream_t *fd, char *s, size_t len, region_id = pre_game_id[10]; /** check multi-disc and insert suffix **/ - cue_append_multi_disc_suffix(pre_game_id, filename); + cue_append_multi_disc_suffix(pre_game_id, sizeof(pre_game_id), filename); _len = strlcpy(s, pre_game_id, len); switch (region_id) @@ -782,7 +789,7 @@ int detect_scd_game(intfstream_t *fd, char *s, size_t len, if ((index = string_index_last_occurance(pre_game_id, '-')) == -1) return 0; strlcpy(s, pre_game_id, (size_t)(index + 1)); - cue_append_multi_disc_suffix(s, filename); + cue_append_multi_disc_suffix(s, len, filename); return 1; } if ((index = string_index_last_occurance(pre_game_id, '-')) == -1) @@ -793,7 +800,7 @@ int detect_scd_game(intfstream_t *fd, char *s, size_t len, s[++_len] = '5'; s[++_len] = '0'; s[++_len] = '\0'; - cue_append_multi_disc_suffix(s, filename); + cue_append_multi_disc_suffix(s, len, filename); } else if (pre_game_id[0] == 'G' && pre_game_id[1] == '-') @@ -801,7 +808,7 @@ int detect_scd_game(intfstream_t *fd, char *s, size_t len, if ((index = string_index_last_occurance(pre_game_id, '-')) == -1) return 0; strlcpy(s, pre_game_id, (size_t)(index + 1)); - cue_append_multi_disc_suffix(s, filename); + cue_append_multi_disc_suffix(s, len, filename); } else if (pre_game_id[0] == 'M' && pre_game_id[1] == 'K' @@ -819,7 +826,7 @@ int detect_scd_game(intfstream_t *fd, char *s, size_t len, } else strlcpy(s, &pre_game_id[3], 5); - cue_append_multi_disc_suffix(s, filename); + cue_append_multi_disc_suffix(s, len, filename); } else { @@ -900,7 +907,7 @@ int detect_sat_game(intfstream_t *fd, char *s, size_t len, } else strlcpy(s, raw_game_id, len); - cue_append_multi_disc_suffix(s, filename); + cue_append_multi_disc_suffix(s, len, filename); break; case 'E': @@ -932,12 +939,12 @@ int detect_sat_game(intfstream_t *fd, char *s, size_t len, else s[_len] = '\0'; - cue_append_multi_disc_suffix(s, filename); + cue_append_multi_disc_suffix(s, len, filename); break; case 'J': strlcpy(s, raw_game_id, len); - cue_append_multi_disc_suffix(s, filename); + cue_append_multi_disc_suffix(s, len, filename); break; default: @@ -1019,7 +1026,7 @@ int detect_dc_game(intfstream_t *fd, char *s, size_t len, s[++_len] = '\0'; strlcpy(s + _len, rgame_id, len - _len); } - cue_append_multi_disc_suffix(s, filename); + cue_append_multi_disc_suffix(s, len, filename); } else if (raw_game_id[0] == 'T') { @@ -1045,7 +1052,7 @@ int detect_dc_game(intfstream_t *fd, char *s, size_t len, if (___len <= 8) { strlcpy(s, pre_game_id, 9); - cue_append_multi_disc_suffix(s, filename); + cue_append_multi_disc_suffix(s, len, filename); return 1; } strlcpy(lgame_id, pre_game_id, 8); @@ -1055,7 +1062,7 @@ int detect_dc_game(intfstream_t *fd, char *s, size_t len, s[ _len] = '-'; s[++_len] = '\0'; strlcpy(s + _len, rgame_id, len - _len); - cue_append_multi_disc_suffix(s, filename); + cue_append_multi_disc_suffix(s, len, filename); } else if (raw_game_id[0] == 'H' && raw_game_id[1] == 'D' @@ -1076,7 +1083,7 @@ int detect_dc_game(intfstream_t *fd, char *s, size_t len, } else strlcpy(s, raw_game_id, len); - cue_append_multi_disc_suffix(s, filename); + cue_append_multi_disc_suffix(s, len, filename); } else if (raw_game_id[0] == 'M' && raw_game_id[1] == 'K' @@ -1116,7 +1123,7 @@ int detect_dc_game(intfstream_t *fd, char *s, size_t len, s[++_len] = '\0'; strlcpy(s + _len, rgame_id, len - _len); } - cue_append_multi_disc_suffix(s, filename); + cue_append_multi_disc_suffix(s, len, filename); } else strlcpy(s, raw_game_id, len); @@ -1165,8 +1172,14 @@ size_t detect_wii_game(intfstream_t *fd, char *s, size_t len, return 0; } - cue_append_multi_disc_suffix(s, filename); - return strlcpy(s, raw_game_id, len); + /* The suffix was appended to 's' and then immediately destroyed by + * the strlcpy() below, so Wii multi-disc titles never received one. + * Every other detector writes the serial first and appends after. */ + { + size_t _len = strlcpy(s, raw_game_id, len); + cue_append_multi_disc_suffix(s, len, filename); + return _len; + } } int detect_system(intfstream_t *fd, const char **system_name,