Skip to content

Commit 8c78693

Browse files
committed
libretro-common/include/array/rbuf: fix OOM safety in RBUF_PUSH and RBUF_RESIZE
The RBUF (stretchy buffer) macros had two latent OOM crash paths and one buffer-overflow path, affecting every RBUF user in the tree (24 RBUF_PUSH call sites, 20+ RBUF_RESIZE sites across gfx, menu, input/bsv, network, playlist, tasks, core_updater_list, m3u_file, nested_list). === The three bugs === Pre-patch macros: #define RBUF_FIT(b, n) \ ((size_t)(n) <= RBUF_CAP(b) ? 0 \ : (*(void**)(&(b)) = rbuf__grow((b), (n), sizeof(*(b))))) #define RBUF_PUSH(b, val) \ (RBUF_FIT((b), 1 + RBUF_LEN(b)), \ (b)[RBUF__HDR(b)->len++] = (val)) #define RBUF_RESIZE(b, sz) \ (RBUF_FIT((b), (sz)), \ ((b) ? RBUF__HDR(b)->len = (sz) : 0)) And rbuf__grow returns: * NULL on first-time (buf==NULL) malloc failure * buf unchanged on realloc failure for non-NULL buf BUG 1: first-time PUSH to NULL buf, malloc fails int *buf = NULL; /* malloc fails */ RBUF_PUSH(buf, 42); /* expands to: rbuf__grow(NULL, 1, sizeof(int)) returns NULL * -> b becomes NULL * -> (b)[RBUF__HDR(b)->len++] = val * = NULL[((rbuf__hdr*)NULL)-1].len++ = ... * NULL-deref */ BUG 2: subsequent PUSH past cap, realloc fails /* buf has len=cap=16, realloc to 32 fails */ RBUF_PUSH(buf, 42); /* rbuf__grow returns original buf unchanged * -> b stays same, cap still 16, len still 16 * -> (b)[16] = val <-- writes past end of allocation * -> len++ <-- len now 17, exceeds cap=16 * * Subsequent PUSHes also write past end, each further * bumping len. Iteration over 0..LEN(b) reads past the * allocation. Memory-corruption territory. */ BUG 3: RESIZE to grow, realloc fails /* buf has len=2, cap=16 */ RBUF_RESIZE(buf, 100); /* rbuf__grow returns unchanged buf, cap still 16 * -> RBUF__HDR(b)->len = 100 <-- len now 100, cap=16 * * Caller iterates 0..100 reading past the 16-elem * allocation. */ === The fix === RBUF_FIT and rbuf__grow are left untouched - their contracts are fine. The fix is in RBUF_PUSH and RBUF_RESIZE: after RBUF_FIT, re-check 'RBUF_CAP(b) >= needed'. If that's false, the grow failed; skip the write / len bump entirely. New RBUF_PUSH (do-while idiom, statement-only): #define RBUF_PUSH(b, val) \ do { \ RBUF_FIT((b), 1 + RBUF_LEN(b)); \ if (RBUF_CAP(b) >= 1 + RBUF_LEN(b)) \ (b)[RBUF__HDR(b)->len++] = (val); \ } while (0) New RBUF_RESIZE (still an expression): #define RBUF_RESIZE(b, sz) \ (RBUF_FIT((b), (sz)), \ ((b) && RBUF_CAP(b) >= (size_t)(sz) \ ? RBUF__HDR(b)->len = (sz) : 0)) Walkthrough for each pre-patch bug: * First-time PUSH + malloc fail: RBUF_FIT makes b=NULL. RBUF_CAP(NULL)=0. 0 >= 1 is false; no write. b stays NULL, len=0. Silent no-op, no crash. * PUSH at cap + realloc fail: RBUF_FIT leaves b unchanged, cap unchanged (say 16, and LEN=16). 16 >= 17 is false; no write, no len bump. Data intact, silent no-op. * RESIZE to grow + realloc fail: RBUF_FIT leaves cap unchanged. cap(16) >= sz(100) false; len not assigned. buf valid at old len/cap. Silent no-op. === Statement-form do-while(0) compatibility === Changing RBUF_PUSH from expression to do-while(0) makes it statement-only. Audited all 24 call sites in the tree: every one uses RBUF_PUSH as a trailing-semicolon statement. None uses it as an expression (no 'x = RBUF_PUSH(...)'). The do-while(0) idiom is the canonical C statement-macro form precisely so it parses correctly as the body of an unbraced if/else/while/for. The five call sites in the tree that do appear in unbraced if/else bodies (gfx_animation.c:850, 852; menu_explore.c:395; menu_explore.c:643; network/drivers_wifi/connmanctl.c:120) compile correctly because 'do {} while(0);' is syntactically one statement, and the trailing semicolon from the call site terminates the do-while naturally. RBUF_RESIZE remains an expression (conditional operator form) to preserve any current expression-context use, though a tree-wide grep shows all existing call sites are also statement-context. === Verification === Unit-tested with a malloc/realloc wrapper that can be toggled to fail on demand: * Test A (first-push OOM): buf stays NULL, len=0 — pre-patch segfaulted, post-patch clean. * Test B (push at cap, OOM): len=16 cap=16 unchanged, data intact — pre-patch wrote past allocation. * Test C (RESIZE grow, OOM): len and cap unchanged at old values — pre-patch set len=100 with cap=16. Also compile-tested all direct consumers: * gfx/gfx_animation.c * input/bsv/uint32s_index.c * menu/menu_explore.c * network/drivers_wifi/connmanctl.c, nmcli.c * core_updater_list.c * libretro-common/formats/m3u/m3u_file.c * libretro-common/lists/nested_list.c * playlist.c * tasks/task_content.c All compile clean with no RBUF-related warnings or errors. === Related prior fix === Commit 1c7e114 (menu + file_list ...) documents this exact issue in its commit message under 'Out of scope' and applies a local workaround in menu_explore.c's ex_arena_grow by refactoring to avoid the vulnerable RBUF_PUSH-on-failure path. This patch is the tree-wide fix that makes that workaround unnecessary, though we leave the menu_explore.c defensive code in place as it is still correct and has additional semantics (NULL-propagation through ex_arena_alloc). === Thread-safety === RBUF operations have always been single-owner (the caller is responsible for its own serialisation). This patch doesn't change that. The new do-while form is still single-expression from the concurrency viewpoint. === Reachability === OOM during RBUF_PUSH / RBUF_RESIZE is reachable from user- driven paths: * playlist growth (every content addition, scan) * menu explore indexing (every Explore menu open/rescan) * gfx animation queueing (every animated UI transition) * core_updater_list population (every online core update fetch) * wifi scan results (every scan) * input/bsv uint32 indexing (movie record/replay) All of these are user-reachable and have been silently fragile against OOM; this patch closes the gap at the foundation.
1 parent 4430028 commit 8c78693

1 file changed

Lines changed: 43 additions & 2 deletions

File tree

  • libretro-common/include/array

libretro-common/include/array/rbuf.h

Lines changed: 43 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,16 @@
5858
* -- To handle running out of memory:
5959
* bool ran_out_of_memory = !RBUF_TRYFIT(buf, 1000);
6060
* -- before RESIZE or PUSH. When out of memory, buf will stay unmodified.
61+
*
62+
* -- OOM behaviour of RBUF_PUSH / RBUF_RESIZE without explicit
63+
* RBUF_TRYFIT:
64+
* Both macros are safe against allocation failure. If the
65+
* underlying realloc / malloc fails, buf is left at its
66+
* previous address (the old allocation remains valid and
67+
* unfreed - realloc contract) and len is NOT advanced. PUSH
68+
* is a silent no-op on OOM; RESIZE leaves len unchanged.
69+
* Callers that need to detect OOM explicitly should use
70+
* RBUF_TRYFIT up-front as shown above.
6171
*/
6272

6373
#include <retro_math.h> /* for MAX */
@@ -72,9 +82,40 @@
7282

7383
#define RBUF_FREE(b) ((b) ? (free(RBUF__HDR(b)), (b) = NULL) : 0)
7484
#define RBUF_FIT(b, n) ((size_t)(n) <= RBUF_CAP(b) ? 0 : (*(void**)(&(b)) = rbuf__grow((b), (n), sizeof(*(b)))))
75-
#define RBUF_PUSH(b, val) (RBUF_FIT((b), 1 + RBUF_LEN(b)), (b)[RBUF__HDR(b)->len++] = (val))
85+
/* RBUF_PUSH: safe against OOM.
86+
*
87+
* Pre-patch this was:
88+
* (RBUF_FIT((b), 1+LEN), (b)[HDR(b)->len++] = (val))
89+
* which on OOM would (a) NULL-deref when b was NULL and grow's
90+
* malloc failed, or (b) write past the end of the existing
91+
* allocation and corrupt the header's len past cap when
92+
* realloc failed on a non-NULL buf.
93+
*
94+
* The fix: re-check 'RBUF_CAP(b) >= 1 + RBUF_LEN(b)' after
95+
* RBUF_FIT returns. If cap didn't grow (meaning grow failed),
96+
* the cap check is false and we skip the write + len bump
97+
* entirely. The old buffer (if any) remains valid - realloc
98+
* contract guarantees the original allocation is intact on
99+
* failure. */
100+
#define RBUF_PUSH(b, val) \
101+
do { \
102+
RBUF_FIT((b), 1 + RBUF_LEN(b)); \
103+
if (RBUF_CAP(b) >= 1 + RBUF_LEN(b)) \
104+
(b)[RBUF__HDR(b)->len++] = (val); \
105+
} while (0)
76106
#define RBUF_POP(b) (b)[--RBUF__HDR(b)->len]
77-
#define RBUF_RESIZE(b, sz) (RBUF_FIT((b), (sz)), ((b) ? RBUF__HDR(b)->len = (sz) : 0))
107+
/* RBUF_RESIZE: safe against OOM.
108+
*
109+
* Pre-patch: (RBUF_FIT((b), sz), ((b) ? HDR(b)->len = sz : 0))
110+
* would set len = sz even when grow failed and cap stayed below
111+
* sz, leaving len > cap. Subsequent iteration of 0..LEN would
112+
* then read past the allocation.
113+
*
114+
* Fix: gate the len assignment on cap >= sz. On OOM len stays
115+
* at its pre-RESIZE value; buf remains valid. */
116+
#define RBUF_RESIZE(b, sz) \
117+
(RBUF_FIT((b), (sz)), \
118+
((b) && RBUF_CAP(b) >= (size_t)(sz) ? RBUF__HDR(b)->len = (sz) : 0))
78119
#define RBUF_CLEAR(b) ((b) ? RBUF__HDR(b)->len = 0 : 0)
79120
#define RBUF_TRYFIT(b, n) (RBUF_FIT((b), (n)), (((b) && RBUF_CAP(b) >= (size_t)(n)) || !(n)))
80121
#define RBUF_REMOVE(b, idx) memmove((b) + (idx), (b) + (idx) + 1, (--RBUF__HDR(b)->len - (idx)) * sizeof(*(b)))

0 commit comments

Comments
 (0)