Skip to content

Commit 2f048a4

Browse files
committed
frontend/drivers: fix OOM bugs in platform startup paths across unix/dos/xdk/emscripten
Seven OOM-related bugs across four frontend/drivers/ files, plus two pre-existing cleanup leaks uncovered during the audit. All on platform-startup or user-exec code paths. === platform_unix.c: android_app_create === Three issues in the Android platform driver's main bootstrap function (runs from ANativeActivity_onCreate on every app launch): 1. slock_new() / scond_new() return values unchecked. slock_new and scond_new can return NULL on OOM. A NULL mutex would silently make every slock_lock/unlock a no-op (slock_lock NULL-tolerates by design), giving a race-prone android_app that 'works' but has subtle concurrency bugs. A NULL cond would NULL-deref on the scond_wait at the bottom of the function (pthread_cond_wait(&NULL->cond, ...)). 2. sthread_create() return value unchecked. If the worker thread fails to spawn, nothing will set android_app-> running=true, and the 'while (!running) scond_wait(...)' loop at the end of the function will block forever. 3. Pre-existing leak (not caused by this series): the pipe(msgpipe) failure path at line 470 freed savedState and android_app itself but leaked the already-created mutex and cond. Fixed in the same pass. Fix for all three: guard each init step, and on any failure fully unwind everything allocated so far (mutex/cond free, savedState free, pipe fds close, android_app free), then return NULL. The caller (ANativeActivity_onCreate at line 500ish) does handle NULL cleanly. === platform_unix.c: frontend_unix_exec === Two issues in the exec path used when restarting RetroArch with a different core: newargv[0] = (char*)malloc(_len); /* _len = strlen(path) */ strlcpy(newargv[0], path, _len); * malloc unchecked, strlcpy NULL-derefs on OOM. * strlcpy wants n bytes to write n-1 chars plus a NUL, so with _len == strlen(path) the final character of path was silently truncated. Pre-existing off-by-one. Fix: _len = strlen(path) + 1, NULL-check malloc, return on OOM. The exec call on the next line uses path directly (not newargv[0]) so exec behaviour isn't affected by the old truncation, but logging and downstream argv consumers see corrupted path. === platform_unix.c: accessibility_speak_unix === char* voice_out = (char*)malloc(3 + strlen(language)); char* speed_out = (char*)malloc(3 + 3); ... voice_out[0] = '-'; /* NULL-deref on OOM */ speed_out[0] = '-'; Fix: NULL-check both, goto the existing 'end:' label which already does NULL-tolerant free()s. The function's contract is always-true return so accessibility speak is silently dropped on OOM (non-critical feature degradation rather than a user-visible error). === platform_dos.c: frontend_dos_exec === Same pattern as frontend_unix_exec but the _len=strlen+1 was already correct in this file. Just needs the NULL-check. === platform_xdk.c: frontend_xdk_get_environment_settings (Xbox 360) === char *extracted_path = (char*)calloc(dwLaunchDataSize, sizeof(char)); BYTE* pLaunchData = (BYTE*)calloc(dwLaunchDataSize, sizeof(BYTE)); XGetLaunchData(pLaunchData, dwLaunchDataSize); /* NULL-deref */ memset(extracted_path, 0, dwLaunchDataSize); /* NULL-deref */ strlcpy(extracted_path, pLaunchData, ...); /* NULL-deref */ Two unchecked callocs feeding three immediate NULL-derefs on OOM, plus a pre-existing leak: the cleanup at the bottom of the block freed pLaunchData but not extracted_path. Fix: joint NULL-check; on success both get freed; on failure both get freed (free(NULL) is a no-op so no extra guards). === platform_emscripten.c: main === emscripten_platform_data = (...)calloc(1, sizeof(...)); ... emscripten_platform_data->browser = host_browser; /* NULL-deref */ This is main() at process entry for the web build. On OOM we can't meaningfully continue - return 1 so the browser shim surfaces the failure. === Not a bug, verified clean === * platform_ps2.c lines 453/460/467: intentional memory- probing loop (allocate max, halve on failure). The 'while (s0 && (p=malloc(s0)) == NULL)' pattern with short-circuit && correctly leaves p at its last-assigned NULL when s0 hits 0, so the subsequent 'if (p) free(p)' is safe. * platform_darwin.m lines 949, 956, 1030: all three already NULL-checked (prior commits in this series). * platform_switch.c lines 487, 555: NULL-checked with realloc-to-tmp for the realloc site. * platform_wii.c:218, platform_ctr.c:329: NULL-checked. * platform_emscripten.c:862: line alloc fed to getline which per POSIX handles NULL line gracefully (allocates its own buffer). * platform_ps3/psp/uwp/win32/orbis/qnx/wiiu/gx: zero raw alloc sites. * frontend/ top-level .c files: zero raw alloc sites. === Thread-safety === android_app_create is called from the Android glue thread during ANativeActivity_onCreate, single-threaded at that point. The new teardown paths happen before the worker thread is spawned (or immediately after its failed spawn), so no concurrent access concerns. frontend_unix_exec, frontend_dos_exec, accessibility_speak_ unix, frontend_xdk_get_environment_settings and main() all run single-threaded on the main thread. === Reachability === Every fix here is on a startup or user-action path: * android_app_create: every Android app launch / restore. * frontend_unix_exec / frontend_dos_exec: every 'restart with a different core' user action. * accessibility_speak_unix: every accessibility-enabled menu navigation event. * platform_xdk: every Xbox 360 content launch (auto-start game from dashboard). * platform_emscripten main: every web-build page load. OOM during platform startup is most realistic on low-memory embedded targets (Android handhelds, Xbox 360, web tabs with memory pressure from other sites).
1 parent 8c78693 commit 2f048a4

4 files changed

Lines changed: 104 additions & 9 deletions

File tree

frontend/drivers/platform_dos.c

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -121,6 +121,15 @@ static void frontend_dos_exec(const char *path, bool should_load_game)
121121
#endif
122122

123123
newargv[0] = (char*)malloc(_len);
124+
/* NULL-check malloc: the strlcpy on the next line
125+
* NULL-derefs on OOM. Void function called from within an
126+
* exec/fork flow; logging and returning leaves the caller
127+
* able to surface the failure. */
128+
if (!newargv[0])
129+
{
130+
RARCH_ERR("Failed to allocate argv for exec.\n");
131+
return;
132+
}
124133
strlcpy(newargv[0], path, _len);
125134

126135
execv(path, newargv);

frontend/drivers/platform_emscripten.c

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -998,8 +998,17 @@ int main(int argc, char *argv[])
998998
pthread_attr_t attr;
999999
pthread_t thread;
10001000
#endif
1001-
/* this never gets freed */
1001+
/* this never gets freed - emscripten_platform_data is held
1002+
* for the lifetime of the web-build process */
10021003
emscripten_platform_data = (emscripten_platform_data_t *)calloc(1, sizeof(emscripten_platform_data_t));
1004+
/* NULL-check: the field writes a few lines down
1005+
* (emscripten_platform_data->browser, ->os, ->...) NULL-deref
1006+
* on OOM. This is main() at process entry - if we can't even
1007+
* allocate the platform state struct there's no viable path
1008+
* forward; bail with non-zero so the browser shim can surface
1009+
* the failure. */
1010+
if (!emscripten_platform_data)
1011+
return 1;
10031012

10041013
PlatformEmscriptenGetSystemInfo(&host_browser, &host_os);
10051014
emscripten_platform_data->browser = host_browser;

frontend/drivers/platform_unix.c

Lines changed: 62 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -449,6 +449,24 @@ static struct android_app* android_app_create(ANativeActivity* activity,
449449

450450
android_app->mutex = slock_new();
451451
android_app->cond = scond_new();
452+
/* NULL-check slock_new / scond_new: both can fail on OOM.
453+
* Without the guards here, a NULL mutex would silently turn
454+
* every slock_lock/unlock below into a no-op (slock_lock
455+
* NULL-tolerates by design), giving a race-prone android_app,
456+
* and a NULL cond would NULL-deref in scond_wait below
457+
* (pthread_cond_wait(&NULL->cond, ...)). Fail the whole
458+
* android_app construction so ANativeActivity_onCreate returns
459+
* cleanly without half-initialised state. */
460+
if (!android_app->mutex || !android_app->cond)
461+
{
462+
if (android_app->mutex)
463+
slock_free(android_app->mutex);
464+
if (android_app->cond)
465+
scond_free(android_app->cond);
466+
free(android_app);
467+
RARCH_ERR("Failed to allocate android_app locks.\n");
468+
return NULL;
469+
}
452470

453471
if (savedState)
454472
{
@@ -471,6 +489,8 @@ static struct android_app* android_app_create(ANativeActivity* activity,
471489
{
472490
if (android_app->savedState)
473491
free(android_app->savedState);
492+
slock_free(android_app->mutex);
493+
scond_free(android_app->cond);
474494
free(android_app);
475495
return NULL;
476496
}
@@ -479,6 +499,23 @@ static struct android_app* android_app_create(ANativeActivity* activity,
479499
android_app->msgwrite = msgpipe[1];
480500

481501
android_app->thread = sthread_create(android_app_entry, android_app);
502+
/* NULL-check sthread_create: on OOM the thread won't be
503+
* spawned and nothing will set android_app->running to true,
504+
* so the scond_wait loop below would block indefinitely.
505+
* Tear down the partially-constructed android_app (including
506+
* the just-created pipe fds) and bail. */
507+
if (!android_app->thread)
508+
{
509+
close(msgpipe[0]);
510+
close(msgpipe[1]);
511+
if (android_app->savedState)
512+
free(android_app->savedState);
513+
slock_free(android_app->mutex);
514+
scond_free(android_app->cond);
515+
free(android_app);
516+
RARCH_ERR("Failed to spawn android_app thread.\n");
517+
return NULL;
518+
}
482519

483520
/* Wait for thread to start. */
484521
slock_lock(android_app->mutex);
@@ -2691,10 +2728,24 @@ static bool frontend_unix_set_fork(enum frontend_fork fork_mode)
26912728
static void frontend_unix_exec(const char *path, bool should_load_content)
26922729
{
26932730
char *newargv[] = { NULL, NULL };
2694-
size_t _len = strlen(path);
2731+
size_t _len = strlen(path) + 1;
26952732

26962733
newargv[0] = (char*)malloc(_len);
26972734

2735+
/* NULL-check malloc: the strlcpy on the next line
2736+
* NULL-derefs on OOM. Void function called from within
2737+
* an exec/fork flow; logging and returning leaves the
2738+
* caller able to retry or surface an error. Prior to
2739+
* this patch _len was strlen(path) (not +1), which meant
2740+
* strlcpy silently truncated the last character of the
2741+
* path because strlcpy needs n bytes to write n-1 chars
2742+
* plus a NUL. */
2743+
if (!newargv[0])
2744+
{
2745+
RARCH_ERR("Failed to allocate argv for exec.\n");
2746+
return;
2747+
}
2748+
26982749
strlcpy(newargv[0], path, _len);
26992750

27002751
execv(path, newargv);
@@ -3267,6 +3318,16 @@ static bool accessibility_speak_unix(int speed,
32673318
char* speed_out = (char*)malloc(3 + 3);
32683319
const char* speeds[10] = {"80", "100", "125", "150", "170", "210", "260", "310", "380", "450"};
32693320

3321+
/* NULL-check both mallocs: voice_out[0]='-' and speed_out[0]='-'
3322+
* below NULL-deref on OOM. The 'end:' label does NULL-tolerant
3323+
* free()s on both pointers, so we can simply skip to it on
3324+
* either failure. Returning true matches the function's
3325+
* existing always-true return contract and means the
3326+
* accessibility request is silently dropped rather than
3327+
* surfacing a user-visible error for a non-critical feature. */
3328+
if (!voice_out || !speed_out)
3329+
goto end;
3330+
32703331
if (speed < 1)
32713332
speed = 1;
32723333
else if (speed > 10)

frontend/drivers/platform_xdk.c

Lines changed: 23 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -185,17 +185,33 @@ static void frontend_xdk_get_environment_settings(int *argc, char *argv[],
185185
char *extracted_path = (char*)calloc(dwLaunchDataSize, sizeof(char));
186186
BYTE* pLaunchData = (BYTE*)calloc(dwLaunchDataSize, sizeof(BYTE));
187187

188-
XGetLaunchData(pLaunchData, dwLaunchDataSize);
189-
memset(extracted_path, 0, dwLaunchDataSize);
188+
/* NULL-check both callocs: memset / XGetLaunchData /
189+
* strlcpy below NULL-deref on OOM. Pre-patch
190+
* extracted_path was also leaked on the happy path
191+
* because the teardown at the bottom of this block
192+
* only freed pLaunchData; the early return on OOM
193+
* would have leaked both. Fix: bail with both
194+
* pointers freed (free(NULL) is a no-op so no extra
195+
* guard needed). */
196+
if (!extracted_path || !pLaunchData)
197+
{
198+
free(extracted_path);
199+
free(pLaunchData);
200+
}
201+
else
202+
{
203+
XGetLaunchData(pLaunchData, dwLaunchDataSize);
204+
memset(extracted_path, 0, dwLaunchDataSize);
190205

191-
strlcpy(extracted_path, pLaunchData, dwLaunchDataSize);
206+
strlcpy(extracted_path, pLaunchData, dwLaunchDataSize);
192207

193-
/* Auto-start game */
194-
if (extracted_path && *extracted_path)
195-
strlcpy(path, extracted_path, sizeof(path));
208+
/* Auto-start game */
209+
if (*extracted_path)
210+
strlcpy(path, extracted_path, sizeof(path));
196211

197-
if (pLaunchData)
198212
free(pLaunchData);
213+
free(extracted_path);
214+
}
199215
}
200216
#endif
201217
if (path && *path)

0 commit comments

Comments
 (0)