Skip to content

Commit 5874334

Browse files
committed
runahead: fix OOM NULL-derefs in input-state tracking and savestate allocation
Five bugs in two call paths (input-state list management for runahead's per-frame input replay, and the savestate ring buffer). === runahead.c: input_list_element_constructor === void *ptr = malloc(sizeof(input_list_element)); input_list_element *element = (input_list_element*)ptr; element->port = 0; /* NULL-deref on OOM */ ... element->state = (int16_t*)calloc(NAME_MAX_LENGTH, sizeof(int16_t)); element->state_size = NAME_MAX_LENGTH; return ptr; Two stacked unchecked allocs. The outer malloc failure would NULL-deref in the field writes immediately below. The inner calloc failure is subtler: ptr itself is still valid and would be returned, but element->state = NULL while element->state_size is set to a non-zero value (NAME_MAX_LENGTH). The caller (runahead_input_state_set_last) then does 'element->state[id] = value' on what looks like a valid element and NULL-derefs. Fix: NULL-check the outer malloc (return NULL for the caller, which already handles it via 'if (element)'). NULL-check the inner calloc, free the outer allocation, and return NULL similarly. Also moved the element->state_size = NAME_MAX_LENGTH assignment to after the successful calloc so state/state_size stay consistent. === runahead.c: input_list_element_realloc === if (new_size > element->state_size) { element->state = (int16_t*)realloc(element->state, new_size * sizeof(int16_t)); memset(&element->state[element->state_size], 0, (new_size - element->state_size) * sizeof(int16_t)); element->state_size = new_size; } Classic realloc-assign-self with no NULL check: on OOM element->state becomes NULL, then '&element->state[element-> state_size]' performs pointer arithmetic on NULL (undefined behaviour) and the memset traps on a garbage address. This is hit by runahead's per-frame input-state tracking any time a port/device/index combination not previously seen encounters an id beyond the element's current state buffer. Fix: realloc-to-tmp, NULL-check the tmp, only commit the new pointer and only update state_size on success. Changed return type from void to bool so callers can see the failure. === runahead.c: input_list_element_expand + callers === Propagate the realloc failure up through expand() (also now returns bool) and gate the subsequent element->state[id] = value writes in runahead_input_state_set_last on expand success. Two call-sites in that function (the lookup-hit branch and the append-new-element branch) both now check: if (id >= element->state_size && !input_list_element_expand(element, id)) return; element->state[id] = value; Without the gate the pre-patch OOM path would write to state[id] with state_size still too small, corrupting whatever followed the state buffer in the heap. === runahead.c: runahead_save_state_alloc === if (runloop_st->runahead_save_state_size > 0 && ...) { savestate->data = malloc(runloop_st->runahead_save_state_size); savestate->data_const = savestate->data; savestate->size = runloop_st->runahead_save_state_size; } malloc unchecked and size is set regardless of alloc outcome. On OOM: savestate->data is NULL but savestate->size equals the full requested size. Downstream readers treat that as a valid buffer of that size and dereference data. Fix: only set savestate->size on successful alloc. Keeps the NULL-data/zero-size invariant that the function's own pre-loop initialisation (data=NULL, data_const=NULL, size=0) already establishes for the 'not runahead' case. runahead_save_state_free correctly handles free(NULL) on the data pointer so the empty placeholder teardown is safe. === Thread-safety === All four functions run on the runloop/main thread (runahead state is single-threaded with respect to the input/runloop subsystem). No lock discipline changes. === Reachability === * input_list_element_{constructor,realloc,expand}: hit every time a core polls a new input id for a port/device/index combination during runahead frames. For cores with many input bindings (arcade, racing wheels with button banks, multi-tap console setups), the id space grows quickly. OOM at the initial NAME_MAX_LENGTH (typically 4096) calloc is unlikely on desktops but realistic on memory-tight embedded targets running runahead with a memory-hungry core. * runahead_save_state_alloc: runs once per runahead frame slot at init, plus on every core-requested savestate size change. runahead_save_state_size can be multi-MB on modern emulator cores, making the alloc a realistic OOM site on 128-512MB handhelds. === Scope === Five bugs in two adjacent-ish functions, all OOM-related, all in runahead.c. Left the preempt_allocate at line ~1380 alone - that function does NULL-check its per-frame malloc and preempt_deinit's 'for i < preempt->frames; free(buffer[i])' loop correctly handles the partial-init case because preempt itself is calloc'd (unallocated buffer[i] slots are NULL and free(NULL) is safe).
1 parent 6cf158d commit 5874334

1 file changed

Lines changed: 56 additions & 10 deletions

File tree

runahead.c

Lines changed: 56 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -717,40 +717,69 @@ static void mylist_create(my_list **list_p, int initial_capacity,
717717
static void *input_list_element_constructor(void)
718718
{
719719
void *ptr = malloc(sizeof(input_list_element));
720-
input_list_element *element = (input_list_element*)ptr;
720+
input_list_element *element;
721+
722+
/* NULL-check the outer malloc: the field writes below
723+
* (port/device/index/state_size) would NULL-deref on OOM.
724+
* The caller (runahead_input_state_set_last) already tolerates
725+
* a NULL return: 'if (element) { ... }'. mylist_resize also
726+
* tolerates a NULL constructor result - it just leaves
727+
* list->data[i] = NULL. */
728+
if (!ptr)
729+
return NULL;
721730

731+
element = (input_list_element*)ptr;
722732
element->port = 0;
723733
element->device = 0;
724734
element->index = 0;
725735
element->state = (int16_t*)calloc(NAME_MAX_LENGTH,
726736
sizeof(int16_t));
737+
/* NULL-check the inner calloc too. If it fails the caller's
738+
* 'element->state[id] = value' NULL-derefs. No way to signal
739+
* partial-failure through the constructor callback's return
740+
* type, so free the outer allocation and return NULL to match
741+
* the outer-OOM behaviour. */
742+
if (!element->state)
743+
{
744+
free(ptr);
745+
return NULL;
746+
}
727747
element->state_size = NAME_MAX_LENGTH;
728748

729749
return ptr;
730750
}
731751

732-
static void input_list_element_realloc(input_list_element *element,
752+
static bool input_list_element_realloc(input_list_element *element,
733753
unsigned int new_size)
734754
{
735755
if (new_size > element->state_size)
736756
{
737-
element->state = (int16_t*)realloc(element->state,
757+
/* realloc-to-tmp: the pre-patch 'element->state = realloc(
758+
* element->state, ...)' self-assigns NULL on OOM, which
759+
* then made the very next line '&element->state[element->
760+
* state_size]' perform pointer arithmetic on NULL (UB) and
761+
* the memset trap on a garbage address. */
762+
int16_t *tmp = (int16_t*)realloc(element->state,
738763
new_size * sizeof(int16_t));
764+
if (!tmp)
765+
return false;
766+
element->state = tmp;
739767
memset(&element->state[element->state_size], 0,
740768
(new_size - element->state_size) * sizeof(int16_t));
741769
element->state_size = new_size;
742770
}
771+
return true;
743772
}
744773

745-
static void input_list_element_expand(input_list_element *element,
774+
static bool input_list_element_expand(input_list_element *element,
746775
unsigned int new_index)
747776
{
748777
unsigned int new_size = element->state_size;
749778
if (new_size == 0)
750779
new_size = 32;
751780
while (new_index >= new_size)
752781
new_size *= 2;
753-
input_list_element_realloc(element, new_size);
782+
return input_list_element_realloc(element, new_size);
754783
}
755784

756785
static void input_list_element_destructor(void* element_ptr)
@@ -785,8 +814,12 @@ static void runahead_input_state_set_last(
785814
&& (element->index == index)
786815
)
787816
{
788-
if (id >= element->state_size)
789-
input_list_element_expand(element, id);
817+
/* Gate the state[id] write on expand success: if expand
818+
* OOM'd, state_size is still too small and writing to
819+
* state[id] would corrupt memory past the buffer. */
820+
if (id >= element->state_size
821+
&& !input_list_element_expand(element, id))
822+
return;
790823
element->state[id] = value;
791824
return;
792825
}
@@ -801,8 +834,10 @@ static void runahead_input_state_set_last(
801834
element->port = port;
802835
element->device = device;
803836
element->index = index;
804-
if (id >= element->state_size)
805-
input_list_element_expand(element, id);
837+
/* Same expand-OOM guard as the lookup branch above. */
838+
if (id >= element->state_size
839+
&& !input_list_element_expand(element, id))
840+
return;
806841
element->state[id] = value;
807842
}
808843
}
@@ -917,7 +952,18 @@ static void *runahead_save_state_alloc(void)
917952
{
918953
savestate->data = malloc(runloop_st->runahead_save_state_size);
919954
savestate->data_const = savestate->data;
920-
savestate->size = runloop_st->runahead_save_state_size;
955+
/* Only record a non-zero size on successful alloc. Pre-
956+
* patch: on OOM data was left NULL but size was set to the
957+
* requested value, which made downstream consumers treat
958+
* the entry as a valid serialized-state buffer of that
959+
* size and dereference data when reading. Keeping size=0
960+
* on failure matches the initial-state invariant above
961+
* (data/data_const = NULL, size = 0) and makes the
962+
* savestate a no-op placeholder that teardown
963+
* (runahead_save_state_free) correctly handles via its
964+
* free(savestate->data) with NULL being a safe no-op. */
965+
if (savestate->data)
966+
savestate->size = runloop_st->runahead_save_state_size;
921967
}
922968

923969
return savestate;

0 commit comments

Comments
 (0)