Skip to content

Commit 55bd49c

Browse files
committed
libretro-db: fix OOM bugs in rmsgpack_dom reader state and query parser
Five related fixes across libretro-db's runtime code (the subset linked into RetroArch itself - c_converter.c and similar offline tools are out of scope for this patch). === rmsgpack_dom.c: rmsgpack_dom_reader_state_new unchecked callocs === struct rmsgpack_dom_reader_state *s = calloc(1, ...); s->i = 0; /* NULL-deref on OOM */ s->capacity = 1024; s->growable = true; s->stack = calloc(1024, ...); return s; Both callocs unchecked. The outer calloc's failure NULL-derefs on the immediate field writes. The inner (stack) calloc's failure is caught later when callers iterate - rmsgpack_dom_read at line ~422 does 's->stack[0] = out' unconditionally. Fix: NULL-check the outer calloc and return NULL. NULL-check the inner calloc and free the outer struct + return NULL on failure. Callers must now tolerate NULL return. === rmsgpack_dom.c: rmsgpack_dom_reader_state_free NULL-deref === free(state->stack); /* NULL-deref if state is NULL */ free(state); state could not be NULL pre-patch (construction couldn't fail) but with reader_state_new now able to return NULL, callers that defer cleanup (like input/bsv/bsvmovie.c's decompress path) would hit this free with NULL. Fix: early return on NULL state. === rmsgpack_dom.c: rmsgpack_dom_reader_state_push realloc-assign-self === s->capacity *= 2; s->stack = realloc(s->stack, s->capacity * ...); if (!s->stack) return -1; Two stacked bugs: 1. Classic realloc-assign-self: on OOM s->stack goes NULL, leaking the old buffer (the only reference). 2. 'capacity *= 2' happens BEFORE the realloc, so on failure the capacity claimed the new size while the allocation is still the old size (or NULL). The -1 return is checked by the direct caller, but the out-of-sync state is a heap- buffer-overflow trap if anything later ignored the error return. Fix: realloc-to-tmp. Compute new_capacity into a local, realloc into new_stack, commit both only on success. Preserves the -1 OOM signal semantics. === query.c: query_parse_method_call zero-arg false-OOM === invocation->argv = (argi > 0) ? malloc(...) : NULL; if (!invocation->argv) { /* emit 'OOM' error */ goto clean; } argv is legitimately NULL when argi == 0 (a zero-argument function call like 'foo()') but the next block treated that as OOM and raised the 'OOM' error string. This was a pre-existing logic bug tangentially exposed by OOM auditing. The observable symptom pre-patch was that any genuinely-zero-argument function call would fail query parsing with a bogus 'OOM' error. Fix: gate the OOM branch on 'argi > 0 && !invocation->argv'. Skip the memcpy when argv is NULL (nothing to copy when argi==0). === input/bsv/bsvmovie.c: bsv_movie_read_deduped_state consumer === struct rmsgpack_dom_reader_state *reader_state = rmsgpack_dom_reader_state_new(); ... rmsgpack_dom_read_with(read_mem, &item, reader_state); With rmsgpack_dom_reader_state_new now able to return NULL, this caller needs to NULL-check before passing reader_state into rmsgpack_dom_read_with (which dereferences it freely). Fix: early bail to the existing 'exit' cleanup label on NULL. The cleanup already calls rmsgpack_dom_reader_state_free (now NULL-tolerant), intfstream_close (already NULL-tolerant), and sets ret=false, so the user-visible outcome on OOM is the same as the existing 'malformed frame' error paths. === Scope notes === Other libretro-db files checked during this pass: * bintree.c - the single alloc site at line 35 is already NULL-checked. * libretrodb.c - two sites at lines 771 (buff in create_index) and 831 (cursor_new) both use the NULL-check idiom. * rmsgpack.c - zero raw alloc sites. * Inner callocs in rmsgpack_dom.c:145 (dom_read_map_start) and :173 (dom_read_array_start) use the 'if (!(items = ...))' idiom and propagate -1 up the rmsgpack callback chain. Not in scope (standalone offline utilities, not linked into RetroArch runtime - see libretro-db/Makefile TARGETS): * c_converter.c - 7 alloc sites * libretrodb_tool.c * rmsgpack_test.c === Thread-safety === All touched paths run synchronously on whichever thread invoked the db query. In practice RetroArch accesses libretro-db from the main thread (menu content scanning, explore-by-metadata) and from task threads (background scan). No shared-state changes; no lock discipline changes. === Reachability === * rmsgpack_dom_reader_state_new/free: the allocating variant is only called from input/bsv/bsvmovie.c's STATESTREAM decode path, used during savestate runahead on cores that support bsv movie recording. * rmsgpack_dom_reader_state_push: called recursively while parsing deep msgpack maps/arrays inside a libretrodb .rdb query result. Capacity doublings trigger on deeply nested data (the default capacity is 1024 for the allocating variant, MAX_DEPTH stack-alloca for rmsgpack_dom_read - the stack variant sets growable=false so never reaches this realloc path). * query_parse_method_call: every libretrodb query string parse - menu-driven 'Explore' category navigation and any core's database-backed metadata lookup.
1 parent 3785495 commit 55bd49c

3 files changed

Lines changed: 57 additions & 7 deletions

File tree

input/bsv/bsvmovie.c

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1658,6 +1658,17 @@ bool bsv_movie_read_deduped_state(bsv_movie_t *movie, uint8_t *encoded, size_t e
16581658
size_t superblock_byte_size = movie->superblocks->object_size*block_byte_size;
16591659
intfstream_t *read_mem = intfstream_open_memory(encoded,
16601660
RETRO_VFS_FILE_ACCESS_READ, RETRO_VFS_FILE_ACCESS_HINT_NONE, encoded_size);
1661+
/* NULL-check reader_state: rmsgpack_dom_reader_state_new can
1662+
* now return NULL on OOM. Bailing to 'exit' hits the
1663+
* rmsgpack_dom_reader_state_free call at the cleanup label
1664+
* (which now tolerates NULL) and returns false; the user-
1665+
* visible outcome is 'STATESTREAM decode failed' which
1666+
* matches the existing error paths for a malformed frame. */
1667+
if (!reader_state)
1668+
{
1669+
RARCH_ERR("[STATESTREAM] failed to allocate reader state\n");
1670+
goto exit;
1671+
}
16611672
if (state_size > movie->last_save_size && movie->superblock_seq)
16621673
{
16631674
free(movie->superblock_seq);

libretro-db/query.c

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -943,7 +943,11 @@ static struct buffer query_parse_method_call(
943943
invocation->argv = (argi > 0) ? (struct argument*)
944944
malloc(sizeof(struct argument) * argi) : NULL;
945945

946-
if (!invocation->argv)
946+
/* Gate the OOM branch on 'argi > 0 && !argv' - before this
947+
* change a valid zero-arg function call ('foo()') was being
948+
* erroneously treated as OOM because argv is legitimately
949+
* NULL when argi==0. */
950+
if (argi > 0 && !invocation->argv)
947951
{
948952
s[0] = 'O';
949953
s[1] = 'O';
@@ -952,8 +956,9 @@ static struct buffer query_parse_method_call(
952956
*err = s;
953957
goto clean;
954958
}
955-
memcpy(invocation->argv, args,
956-
sizeof(struct argument) * argi);
959+
if (invocation->argv)
960+
memcpy(invocation->argv, args,
961+
sizeof(struct argument) * argi);
957962

958963
return buff;
959964

libretro-db/rmsgpack_dom.c

Lines changed: 38 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -54,14 +54,28 @@ static int rmsgpack_dom_reader_state_push(
5454
{
5555
if ((s->i + 1) == s->capacity)
5656
{
57+
size_t new_capacity;
58+
struct rmsgpack_dom_value **new_stack;
59+
5760
if (!s->growable)
5861
return -1;
5962

60-
s->capacity *= 2;
61-
s->stack = (struct rmsgpack_dom_value **)
62-
realloc(s->stack, s->capacity * sizeof(struct rmsgpack_dom_value *));
63-
if (!s->stack)
63+
/* realloc-to-tmp: pre-patch was 's->stack = realloc(s->stack,
64+
* ...)' which on OOM would leak the old buffer (self-assign
65+
* drops the only reference). Also pre-patch did
66+
* 'capacity *= 2' before the realloc, so on failure the
67+
* capacity claimed the new size while the allocation was
68+
* still the old size - out-of-sync state that would manifest
69+
* as a heap-buffer-overflow later if anything else ignored
70+
* the -1 return. Compute new_capacity into a local, realloc
71+
* into a tmp, commit both only on success. */
72+
new_capacity = s->capacity * 2;
73+
new_stack = (struct rmsgpack_dom_value **)
74+
realloc(s->stack, new_capacity * sizeof(struct rmsgpack_dom_value *));
75+
if (!new_stack)
6476
return -1;
77+
s->stack = new_stack;
78+
s->capacity = new_capacity;
6579
}
6680
s->i++;
6781
s->stack[s->i] = v;
@@ -429,15 +443,35 @@ struct rmsgpack_dom_reader_state *rmsgpack_dom_reader_state_new(void)
429443
{
430444
struct rmsgpack_dom_reader_state *s = (struct rmsgpack_dom_reader_state *)calloc(1,
431445
sizeof(struct rmsgpack_dom_reader_state));
446+
/* NULL-check: the field writes below NULL-deref on OOM. */
447+
if (!s)
448+
return NULL;
432449
s->i = 0;
433450
s->capacity = 1024;
434451
s->growable = true;
435452
s->stack = (struct rmsgpack_dom_value **)calloc(1024, sizeof(struct rmsgpack_dom_value *));
453+
/* NULL-check the stack calloc: consumers (rmsgpack_dom_read
454+
* above at line ~422 writes s->stack[0] unconditionally) would
455+
* NULL-deref on OOM. Free the containing state struct and
456+
* return NULL so the caller (rmsgpack_dom_read does this via
457+
* its own caller chain) can fall through gracefully. */
458+
if (!s->stack)
459+
{
460+
free(s);
461+
return NULL;
462+
}
436463
return s;
437464
}
438465

439466
void rmsgpack_dom_reader_state_free(struct rmsgpack_dom_reader_state *state)
440467
{
468+
/* NULL-check: rmsgpack_dom_reader_state_new can now return
469+
* NULL on OOM (this commit). Callers that defer cleanup to
470+
* the end of a function (e.g. input/bsv/bsvmovie.c's
471+
* decompress path at line ~1833) may hit this with a NULL
472+
* state pointer if the reader_state_new call failed. */
473+
if (!state)
474+
return;
441475
free(state->stack);
442476
free(state);
443477
}

0 commit comments

Comments
 (0)