Skip to content

Commit 3a5775d

Browse files
committed
cheat_manager: fix three OOM bug clusters in cheats realloc and memory buffer init
Three separate bug clusters in cheat_manager.c, all OOM paths, all on hot paths (cheat loading, cheat list grow, memory-map init at core start). === cheat_manager_realloc: realloc-assign-self + leak === val = (struct item_cheat*) realloc(cheat_st->cheats, new_size * sizeof(struct item_cheat)); cheat_st->cheats = val ? val : NULL; This looks like a safe realloc-to-tmp but isn't. On OOM 'val' is NULL and 'cheats' is still the valid old buffer per the C realloc contract - but the line above explicitly sets cheats to NULL, dropping the only reference and leaking the entire container. Worse, each entry in that container has live .code and .desc strdup'd strings that were about to be freed by the shrink branch above (lines 561-569 handled indices [new_size, orig_size) but [0, min(new_size, orig_size)) still held live strings at the moment of the realloc call). Fix: free the old buffer AND its per-entry .code/.desc strings explicitly on OOM before NULL'ing cheats. Preserves the existing failure contract (size=0, cheats=NULL) that callers at lines ~515 and ~1206 rely on: /* caller at 515: gates iteration on cheats non-NULL */ for (i = orig_size; cheat_st->cheats && i < cheats; i++) /* caller at 1206: returns false if realloc returned false */ if (!cheat_manager_realloc(new_size, CHEAT_HANDLER_TYPE_RETRO)) return false; === cheat_manager_initialize_memory descriptor loop: two stacked bugs === cheat_st->num_memory_buffers++; if (!cheat_st->memory_buf_list) cheat_st->memory_buf_list = calloc(1, sizeof(uint8_t *)); else cheat_st->memory_buf_list = realloc( cheat_st->memory_buf_list, sizeof(uint8_t *) * cheat_st->num_memory_buffers); if (!cheat_st->memory_size_list) cheat_st->memory_size_list = calloc(1, sizeof(unsigned)); else { unsigned *val = realloc(...); if (val) cheat_st->memory_size_list = val; } cheat_st->memory_buf_list [num - 1] = ...; cheat_st->memory_size_list[num - 1] = ...; Two bugs: 1. memory_buf_list uses realloc-assign-self: on OOM the old buffer leaks and the [num - 1] write below NULL-derefs. 2. memory_size_list uses realloc-into-tmp with a NULL-check but silently ignores the failure. That leaves the array one element too short while num_memory_buffers claims the new size. The [num - 1] write below is then a heap-buffer-overflow past the end of memory_size_list. Plus num_memory_buffers++ happens eagerly at the top, so on OOM the count and the array lengths are out of sync. Fix: compute new_count into a local temp, grow both lists into temp pointers, commit both and bump num_memory_buffers only after both succeed. On either OOM continue to the next descriptor (the post-loop 'if (num_memory_buffers == 0)' check then takes the RETRO_MEMORY_SYSTEM_RAM fallback path). The one-slot overallocation on a partial-success path (buf_list grew, size_list failed) is a minor waste of memory but can't corrupt state because num_memory_buffers never advanced. === cheat_manager_initialize_memory fallback path: unchecked callocs === cheat_st->memory_buf_list = calloc(1, sizeof(uint8_t *)); cheat_st->memory_size_list = calloc(1, sizeof(unsigned)); cheat_st->num_memory_buffers = 1; cheat_st->memory_buf_list[0] = ...; /* NULL-deref on OOM */ cheat_st->memory_size_list[0] = ...; /* NULL-deref on OOM */ When num_memory_buffers == 0 after the descriptor loop, the RETRO_MEMORY_SYSTEM_RAM fallback allocates two one-element arrays and writes to [0]. Both callocs are unchecked. Fix: NULL-check both and bail via the same MSG_CHEAT_INIT_FAIL path used by the meminfo-failed branch a few lines above. Free whichever calloc succeeded (free(NULL) is safe for the other). === Not a bug, verified clean === Other alloc sites in cheat_manager.c, also checked during this pass: * line 366 (cheats initial calloc) - NULL-checked with graceful zero-out. * line 552 (cheats create-if-null branch in realloc) - flows into the same post-alloc check that fires in the realloc case. * line 939 (prev_memory_buf) - NULL-checked with MSG_CHEAT_INIT_FAIL bail. * line 957 (matches) - NULL-checked with matching teardown (frees prev_memory_buf on failure). Other top-level files swept in this pass and found clean: * configuration.c, dynamic.c, retroarch.c - zero raw alloc sites. * core_option_manager.c - 13 sites, all use the 'if (!(x = alloc(...)))' idiom. * core_info.c - 8 sites NULL-checked including the realloc-to-tmp at line 400. * database_info.c - 3 sites NULL-checked (lines 988, 1022, 1204). * runloop.c - 2 sites NULL-checked (lines 2746, 2794) in the SET_SUBSYSTEM_INFO and SET_CONTROLLER_INFO environ handlers. === Thread-safety === cheat_manager_realloc runs on the main thread (invoked from menu or cheat-file load). cheat_manager_initialize_memory runs on the main thread at core load (after retro_load_game). No shared-state changes; no lock discipline changes. === Reachability === * cheat_manager_realloc: every cheat file load, every user- driven cheat list grow/shrink via the menu. * cheat_manager_initialize_memory: every core load with HAVE_CHEEVOS or cheats feature enabled. On handhelds with sub-256MB RAM running a memory-hungry core, the one-element allocs on the fallback path are cheap but the multi- descriptor realloc path can realloc arrays with dozens of entries for cores that expose many memory regions (arcade cores, multi-chip emulators).
1 parent 44b5197 commit 3a5775d

1 file changed

Lines changed: 87 additions & 16 deletions

File tree

cheat_manager.c

Lines changed: 87 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -572,7 +572,38 @@ bool cheat_manager_realloc(unsigned new_size, unsigned default_handler)
572572
realloc(cheat_st->cheats,
573573
new_size * sizeof(struct item_cheat));
574574

575-
cheat_st->cheats = val ? val : NULL;
575+
/* realloc-to-tmp: on OOM 'val' is NULL, the original
576+
* cheat_st->cheats is still valid. Pre-patch did
577+
* 'cheats = val ? val : NULL' which silently leaked the
578+
* valid old buffer (the pointer was the only reference).
579+
*
580+
* Free the old buffer explicitly on failure. Also free
581+
* per-entry .code/.desc strings for indices still present
582+
* in the old buffer - the shrink branch above (lines
583+
* 561-569) already handled [new_size, orig_size) but
584+
* the [0, min(new_size, orig_size)) entries still hold
585+
* live strings. Without this cleanup we'd swap one leak
586+
* (the container) for another (the strings inside).
587+
*
588+
* The failure contract here is 'size goes to 0, cheats
589+
* cleared' (see the 'if (!cheat_st->cheats)' block below
590+
* that sets size=0 and buf_size=0, and caller checks
591+
* at line ~515 that gate on 'cheat_st->cheats &&'). */
592+
if (val)
593+
cheat_st->cheats = val;
594+
else
595+
{
596+
unsigned keep = (new_size < orig_size) ? new_size : orig_size;
597+
for (i = 0; i < keep; i++)
598+
{
599+
if (cheat_st->cheats[i].code)
600+
free(cheat_st->cheats[i].code);
601+
if (cheat_st->cheats[i].desc)
602+
free(cheat_st->cheats[i].desc);
603+
}
604+
free(cheat_st->cheats);
605+
cheat_st->cheats = NULL;
606+
}
576607
}
577608

578609
if (!cheat_st->cheats)
@@ -813,30 +844,54 @@ int cheat_manager_initialize_memory(rarch_setting_t *setting, size_t idx, bool w
813844
&& sys_info->mmaps.descriptors[i].core.ptr
814845
&& sys_info->mmaps.descriptors[i].core.len > 0)
815846
{
816-
cheat_st->num_memory_buffers++;
847+
/* Grow both lists atomically and bail on OOM.
848+
* Pre-patch:
849+
* - num_memory_buffers++ was done first
850+
* - memory_buf_list = realloc(memory_buf_list, ...)
851+
* self-assigned on OOM (leak + NULL-deref at [i])
852+
* - memory_size_list used realloc-into-tmp with a
853+
* NULL-check but ignored the failure, which left
854+
* the array one element too small while
855+
* num_memory_buffers claimed the new size -
856+
* heap buffer overflow on the [num-1] write
857+
* below.
858+
*
859+
* Fix: compute new_count and grow both lists into
860+
* temp pointers before incrementing num_memory_buffers.
861+
* Skip this descriptor on OOM (continue outer loop);
862+
* the loop's post-check 'if (cheat_st->num_memory_
863+
* buffers == 0)' then falls through to the
864+
* RETRO_MEMORY_SYSTEM_RAM fallback path below. */
865+
unsigned new_count = cheat_st->num_memory_buffers + 1;
866+
uint8_t **new_bufs;
867+
unsigned *new_sizes;
817868

818869
if (!cheat_st->memory_buf_list)
819-
cheat_st->memory_buf_list = (uint8_t**)calloc(1, sizeof(uint8_t *));
870+
new_bufs = (uint8_t**)calloc(1, sizeof(uint8_t *));
820871
else
821-
cheat_st->memory_buf_list = (uint8_t**)realloc(
822-
cheat_st->memory_buf_list, sizeof(uint8_t *) * cheat_st->num_memory_buffers);
872+
new_bufs = (uint8_t**)realloc(
873+
cheat_st->memory_buf_list,
874+
sizeof(uint8_t *) * new_count);
875+
876+
if (!new_bufs)
877+
continue;
878+
cheat_st->memory_buf_list = new_bufs;
823879

824880
if (!cheat_st->memory_size_list)
825-
cheat_st->memory_size_list = (unsigned*)calloc(1, sizeof(unsigned));
881+
new_sizes = (unsigned*)calloc(1, sizeof(unsigned));
826882
else
827-
{
828-
unsigned *val = (unsigned*)realloc(
883+
new_sizes = (unsigned*)realloc(
829884
cheat_st->memory_size_list,
830-
sizeof(unsigned) *
831-
cheat_st->num_memory_buffers);
885+
sizeof(unsigned) * new_count);
832886

833-
if (val)
834-
cheat_st->memory_size_list = val;
835-
}
887+
if (!new_sizes)
888+
continue;
889+
cheat_st->memory_size_list = new_sizes;
836890

837-
cheat_st->memory_buf_list[cheat_st->num_memory_buffers - 1] = (uint8_t*)sys_info->mmaps.descriptors[i].core.ptr;
838-
cheat_st->memory_size_list[cheat_st->num_memory_buffers - 1] = (unsigned)sys_info->mmaps.descriptors[i].core.len;
839-
cheat_st->total_memory_size += sys_info->mmaps.descriptors[i].core.len;
891+
cheat_st->num_memory_buffers = new_count;
892+
cheat_st->memory_buf_list[new_count - 1] = (uint8_t*)sys_info->mmaps.descriptors[i].core.ptr;
893+
cheat_st->memory_size_list[new_count - 1] = (unsigned)sys_info->mmaps.descriptors[i].core.len;
894+
cheat_st->total_memory_size += sys_info->mmaps.descriptors[i].core.len;
840895

841896
if (!cheat_st->curr_memory_buf)
842897
cheat_st->curr_memory_buf = (uint8_t*)sys_info->mmaps.descriptors[i].core.ptr;
@@ -863,6 +918,22 @@ int cheat_manager_initialize_memory(rarch_setting_t *setting, size_t idx, bool w
863918
calloc(1, sizeof(uint8_t *));
864919
cheat_st->memory_size_list = (unsigned*)
865920
calloc(1, sizeof(unsigned));
921+
/* NULL-check both callocs: the [0] writes below NULL-deref
922+
* on OOM. If either failed, free whichever succeeded and
923+
* bail out to MSG_CHEAT_INIT_FAIL with the same path as
924+
* the meminfo failure above. */
925+
if (!cheat_st->memory_buf_list || !cheat_st->memory_size_list)
926+
{
927+
char msg[128];
928+
size_t _len = strlcpy(msg, msg_hash_to_str(MSG_CHEAT_INIT_FAIL), sizeof(msg));
929+
free(cheat_st->memory_buf_list);
930+
cheat_st->memory_buf_list = NULL;
931+
free(cheat_st->memory_size_list);
932+
cheat_st->memory_size_list = NULL;
933+
runloop_msg_queue_push(msg, _len, 1, 180, true, NULL,
934+
MESSAGE_QUEUE_ICON_DEFAULT, MESSAGE_QUEUE_CATEGORY_INFO);
935+
return 0;
936+
}
866937
cheat_st->num_memory_buffers = 1;
867938
cheat_st->memory_buf_list[0] = (uint8_t*)meminfo.data;
868939
cheat_st->memory_size_list[0] = (unsigned)meminfo.size;

0 commit comments

Comments
 (0)