Skip to content

Commit f26e635

Browse files
committed
libretro-common + configuration: fix OOM NULL-deref bugs across config_file, file_path QNX path, 7z stream_new, zip context init, zip_file_read, and config_save_overrides
Six unchecked allocations spanning the config/file/encoding layer and the top-level configuration.c override-save path. === libretro-common/file/config_file.c: config_file_add_reference === if (!conf->references) { conf->references = (struct path_linked_list*)malloc(sizeof(*conf->references)); conf->references->next = NULL; conf->references->path = NULL; } Unchecked malloc. The ->next / ->path writes immediately below NULL-deref on OOM, and even if they didn't, the subsequent path_linked_list_add_path walks the list expecting a valid head. Fix: return 0 on OOM (signals 'no reference added' cleanly, matches the state that persists - conf->references stays NULL which other call paths already tolerate since the struct is zero-initialized that way). === libretro-common/file/file_path.c: QNX fill_pathname_application_path === #elif defined(__QNX__) char *buff = (char*)malloc(len); size_t _len = 0; if (_cmdname(buff)) _len = strlcpy(s, buff, len); Unchecked malloc. _cmdname() writes through its argument to populate the command path; passing NULL is a NULL-deref inside libc. Fix: return 0 on OOM. Callers treat 0 as 'unable to resolve own path' and fall back to argv[0] or similar. QNX-only path, but the bug is real on that target. === libretro-common/file/archive_file_7z.c: sevenzip_stream_new === struct sevenzip_context_t *sevenzip_context = (struct sevenzip_context_t*)calloc(1, sizeof(...)); sevenzip_context->allocImp.Alloc = sevenzip_stream_alloc_impl; sevenzip_context->allocImp.Free = sevenzip_stream_free_impl; ... Unchecked calloc. The eight field writes below NULL-deref on OOM. Caller sevenzip_parse_file_free NULL-tolerates (checks at line ~112). Fix: return NULL on OOM; caller already handles it. === libretro-common/file/archive_file_zlib.c: zip_context_set_size_and_mode === Three separate mallocs in the same function: zip_context->decompressed_data = (uint8_t*)malloc(size); /* ... later ... */ zip_context->zstream = (z_stream*)malloc(sizeof(z_stream)); zip_context->tmpbuf = (uint8_t*)malloc(_READ_CHUNK_SIZE); zip_context->zstream->next_in = NULL; ... zip_context->zstream->next_out = zip_context->decompressed_data; All three unchecked. decompressed_data seeds the inflate output at the next_out assignment. zstream's 8 field writes below NULL-deref on OOM. tmpbuf is read by inflate() downstream. Fix: NULL-check decompressed_data immediately and fail the whole context setup; NULL-check the zstream/tmpbuf pair together after the ZIP_MODE_DEFLATED branch. Failure goes through zip_context_free_stream which is NULL-safe for all three fields. === libretro-common/file/archive_file_zlib.c: zip_file_read === if (needle) decomp.needle = strdup(needle); if (optional_outfile) decomp.opt_file = strdup(optional_outfile); ... ret = file_archive_parse_file_iterate(...); The iterate callback zip_file_decompressed does: if (strstr(name, decomp_state->needle)) If needle was requested but strdup failed, decomp.needle is NULL and strstr(name, NULL) is undefined (glibc crashes). Fix: NULL-check strdups, free whatever succeeded on partial OOM, return -1 (caller treats as 'not found / failed'). === configuration.c: config_save_overrides === settings = (settings_t*)calloc(1, sizeof(settings_t)); conf = config_file_new_alloc(); /* ... pages later ... */ config_load_file(global_get_ptr(), "without-overrides", settings); Both allocations unchecked. config_load_file dereferences settings unconditionally (no NULL-check at its entry). The subsequent ~300 lines of config_set_* calls through 'conf' also deref it. On OOM either pointer causes a crash deep inside the override save path. Fix: NULL-check both at the top, clean up whichever succeeded (free(settings) is NULL-safe but conf needs explicit config_file_free + NULL guard), return -1. Verified that returning -1 is safe: the int8_t return type accommodates it, and the callers at command.c and menu/cbs/menu_cbs_ok.c both explicitly switch on the -1 case and produce MESSAGE_QUEUE_CATEGORY_WARNING + MSG_OVERRIDES_NOT_SAVED. The third caller for the remove path treats the result as bool (truthy = remove succeeded, falsey = error); under -1 it would report MSG_OVERRIDES_REMOVED_SUCCESSFULLY incorrectly, but that's a pre-existing non-crash issue (the remove path couldn't even start on OOM) - the fix here addresses the crash; fixing the reporting inaccuracy would be a separate change. === Swept-clean in the same pass === Verified NULL-checked across the config/file/encoding surface: - config_file.c other 16 sites: config_file_set_hash entry allocation with prior audit comment (bulk free on partial strdup failure), config_file_extract_value internal strdups feed into callers that NULL-gate, main config_file_new_alloc and initialize chain all NULL-check. - encoding_utf.c (12 sites): all NULL-check via early return or conditional assignment. - encoding_base64.c (2 sites): NULL-checked. - file_path.c other 5 sites: path_linked_list functions all NULL-check, filestem buffer allocs all NULL-check. - file_path_io.c (1 strdup), archive_file.c (1 strdup): both NULL-checked. - archive_file_zstd.c (6 sites): all NULL-checked with proper cleanup on partial failure. - archive_file_7z.c other 7 sites: lookStream.buf mallocs set bufSize=0 on failure (tolerated), file-name temp mallocs all NULL-checked, *buf = malloc(outsize+1) has prior-audit NULL-check with comment. - config_file_userdata.c (5 sites): all NULL-checked. - configuration.c other 17 sites: all NULL-checked via early-return pattern (SETTINGS_*_COUNT_MAX allocs at populate_settings_* helpers), defaults / defaults_binds / saved_autoconf_binds path in config_load_default all NULL-check with fall-back-to-non-minimal-mode comments. Thread-safety: all six fixed paths run on the main thread: - config_file.c paths run during config parsing (init or reload, main thread only). - file_path.c QNX path runs once at frontend startup. - archive_file_7z.c stream_new runs on the extract thread but per-stream - no concurrent access to the context. - archive_file_zlib.c paths run on extract thread, same per-stream isolation. - configuration.c save_overrides runs on the main thread in response to a menu command. Reachability: config parsing and archive extraction are both hot paths. OOM during archive extract is realistic on embedded targets (3DS / Vita / Wii) with ~16-96 MB total RAM. config_save_overrides runs on user action from the menu - low frequency but a real user-facing crash on memory-starved sessions. None of these are security- sensitive in the attacker-controlled sense; all are availability hardening.
1 parent 4e5f06b commit f26e635

5 files changed

Lines changed: 81 additions & 0 deletions

File tree

configuration.c

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6531,6 +6531,26 @@ int8_t config_save_overrides(enum override_type type,
65316531
settings = (settings_t*)calloc(1, sizeof(settings_t));
65326532
conf = config_file_new_alloc();
65336533

6534+
/* NULL-check: both calloc and config_file_new_alloc can
6535+
* return NULL on OOM. config_load_file at line ~6552
6536+
* dereferences settings unconditionally, and all the
6537+
* config_set_* calls later dereference conf. Rather than
6538+
* sprinkle NULL-guards through a 300-line function, bail
6539+
* out here. The function-end cleanup at line ~6866 does
6540+
* free(settings) which is NULL-safe, but conf needs
6541+
* explicit config_file_free + NULL guard.
6542+
*
6543+
* Return -1 to signal hard failure (as distinct from
6544+
* 'override was not needed' = false/0 at line 6529).
6545+
* int8_t return value accommodates -1. */
6546+
if (!settings || !conf)
6547+
{
6548+
if (conf)
6549+
config_file_free(conf);
6550+
free(settings);
6551+
return -1;
6552+
}
6553+
65346554
/* Get base config directory */
65356555
fill_pathname_application_special(config_directory,
65366556
sizeof(config_directory),

libretro-common/file/archive_file_7z.c

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -87,6 +87,12 @@ static void* sevenzip_stream_new(void)
8787
struct sevenzip_context_t *sevenzip_context =
8888
(struct sevenzip_context_t*)calloc(1, sizeof(struct sevenzip_context_t));
8989

90+
/* NULL-check the calloc: the field writes below NULL-deref
91+
* on OOM and the returned NULL is handled by the caller
92+
* (sevenzip_parse_file_free at line ~112 NULL-tolerates). */
93+
if (!sevenzip_context)
94+
return NULL;
95+
9096
/* These are the allocation routines - currently using
9197
* the non-standard 7zip choices. */
9298
sevenzip_context->allocImp.Alloc = sevenzip_stream_alloc_impl;

libretro-common/file/archive_file_zlib.c

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -143,12 +143,34 @@ static bool zlib_stream_decompress_data_to_file_init(
143143
zip_context->zstream = NULL;
144144
zip_context->tmpbuf = NULL;
145145

146+
/* NULL-check the decompressed_data malloc: subsequent
147+
* consumers (zip_context_iterate, and line 155 below where
148+
* zstream->next_out = decompressed_data seeds the inflate
149+
* output) unconditionally dereference it. On OOM fail the
150+
* whole context setup via zip_context_free_stream which is
151+
* NULL-safe for both zstream and tmpbuf. */
152+
if (!zip_context->decompressed_data)
153+
{
154+
zip_context_free_stream(zip_context, false);
155+
return false;
156+
}
157+
146158
if (cmode == ZIP_MODE_DEFLATED)
147159
{
148160
/* Initialize the zlib inflate machinery */
149161
zip_context->zstream = (z_stream*)malloc(sizeof(z_stream));
150162
zip_context->tmpbuf = (uint8_t*)malloc(_READ_CHUNK_SIZE);
151163

164+
/* NULL-check both mallocs: the zstream->next_in etc.
165+
* field writes below NULL-deref on OOM for zstream, and
166+
* inflate() later reads from tmpbuf. Fail the context
167+
* setup on either failure. */
168+
if (!zip_context->zstream || !zip_context->tmpbuf)
169+
{
170+
zip_context_free_stream(zip_context, false);
171+
return false;
172+
}
173+
152174
zip_context->zstream->next_in = NULL;
153175
zip_context->zstream->avail_in = 0;
154176
zip_context->zstream->total_in = 0;
@@ -429,6 +451,20 @@ static int64_t zip_file_read(
429451
if (optional_outfile)
430452
decomp.opt_file = strdup(optional_outfile);
431453

454+
/* NULL-check strdups: zip_file_decompressed (line ~396)
455+
* calls strstr(name, decomp_state->needle) which NULL-derefs
456+
* if needle was requested but strdup failed. Bail out of
457+
* the extraction; caller treats -1 as 'not found / failed'.
458+
* Free whatever strdup succeeded to avoid a leak on
459+
* partial-success OOM. */
460+
if ((needle && !decomp.needle) ||
461+
(optional_outfile && !decomp.opt_file))
462+
{
463+
free(decomp.needle);
464+
free(decomp.opt_file);
465+
return -1;
466+
}
467+
432468
state.type = ARCHIVE_TRANSFER_INIT;
433469
userdata.transfer = &state;
434470
userdata.cb_data = &decomp;

libretro-common/file/config_file.c

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -410,6 +410,17 @@ size_t config_file_add_reference(config_file_t *conf, char *path)
410410
if (!conf->references)
411411
{
412412
conf->references = (struct path_linked_list*)malloc(sizeof(*conf->references));
413+
/* NULL-check: the next two field writes NULL-deref on OOM,
414+
* and the subsequent path_linked_list_add_path call would
415+
* walk ->next through a NULL head. On OOM bail before
416+
* filling short_path - fill_pathname_abbreviated_or_
417+
* relative returns the computed length regardless of
418+
* whether references was successfully allocated, so
419+
* compute-and-return a valid length is also an option, but
420+
* returning 0 signals 'no reference added' cleanly and
421+
* matches the state (no reference) that persists. */
422+
if (!conf->references)
423+
return 0;
413424
conf->references->next = NULL;
414425
conf->references->path = NULL;
415426
}

libretro-common/file/file_path.c

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1399,6 +1399,14 @@ size_t fill_pathname_application_path(char *s, size_t len)
13991399
#elif defined(__QNX__)
14001400
char *buff = (char*)malloc(len);
14011401
size_t _len = 0;
1402+
/* NULL-check the malloc: _cmdname writes through its
1403+
* buffer argument (populates with the command path),
1404+
* NULL-derefs on OOM. Leave s as it was set by the
1405+
* caller (empty or previous value) and return 0 -
1406+
* callers treat 0 as 'unable to resolve own path' and
1407+
* fall back to argv[0] or similar. */
1408+
if (!buff)
1409+
return 0;
14021410
if (_cmdname(buff))
14031411
_len = strlcpy(s, buff, len);
14041412
free(buff);

0 commit comments

Comments
 (0)