Skip to content

Commit f07549b

Browse files
committed
overlay: fix OOM crash cascade across task_overlay producer and input_driver consumer
Four bugs forming one OOM cascade in the input-overlay load pipeline. Fixing any one of them in isolation just pushes the crash one call-frame deeper; they have to land together. === tasks/task_overlay.c: task_overlay_desc_populate_eightway_config === desc->eightway_config = (overlay_eightway_config_t *) calloc(1, sizeof(overlay_eightway_config_t)); eightway = desc->eightway_config; switch (desc->type) { case OVERLAY_TYPE_DPAD_AREA: BIT256_SET(eightway->up, ...); /* NULL-deref */ ... Unchecked calloc; the BIT256_SET writes in the switch body NULL-deref on OOM. void-returning function so we can't signal failure to the caller directly. Fix: NULL-check and early return. This leaves desc->eightway_config = NULL for an area-type desc that needs it, which then fires the consumer bug below. === input/input_driver.c: input_overlay_poll eightway dispatch === case OVERLAY_TYPE_DPAD_AREA: case OVERLAY_TYPE_ABXY_AREA: input_overlay_get_eightway_state( desc, desc->eightway_config, &out->buttons, x_dist, y_dist); break; input_overlay_get_eightway_state dereferences eightway-> slope_high unconditionally (line ~2781). With the producer fix above leaving eightway_config NULL under OOM, this dispatch path needs to gate on non-NULL: if (desc->eightway_config) input_overlay_get_eightway_state(...); User-visible consequence on OOM: the one area whose config alloc failed is dead until overlay reload. Strictly better than a segfault on the next touch input. === tasks/task_overlay.c: task_overlay_handler data alloc === overlay_task_data_t *data = (overlay_task_data_t*) calloc(1, sizeof(*data)); data->overlays = loader->overlays; /* NULL-deref */ ... Unchecked calloc in the task's 'finished and not cancelled' branch; the seven field writes below NULL-deref on OOM. Fix: NULL-check and early return (skip the task_set_data call). The consumer (input_overlay_loaded in input/input_driver.c) then sees NULL task_data, which cascades to the next bug. === input/input_driver.c: input_overlay_loaded task_data and ol allocs === overlay_task_data_t *data = (overlay_task_data_t*)task_data; ... if (err) return; ol = (input_overlay_t*)calloc(1, sizeof(*ol)); ol->overlays = data->overlays; /* NULL-deref */ ... Two stacked bugs: 1. task_data can be NULL (from the producer fix above) but is dereferenced as data->overlays on the very next usable line. Add an early return. 2. The ol calloc is unchecked. The field writes below NULL-deref on OOM. Unlike typical OOM-bail patterns, bailing here has a non-trivial cleanup obligation: task_data was transferred to us by the retro_task framework and we own it, *and* the image_list inside it has heap texture_image attachments on each entry's .attr.p that the success path would transfer into ol->images via input_overlay_loaded_move_images. Since ol doesn't exist, we need to free each texture explicitly before string_list_free (string_list_free only frees the list struct and its entries' .data strings, not the .attr.p attachments). Fix both: if (!data) return; ol = (input_overlay_t*)calloc(1, sizeof(*ol)); if (!ol) { if (data->image_list) { size_t i; for (i = 0; i < data->image_list->size; i++) image_texture_free( (struct texture_image*)data->image_list->elems[i].attr.p); string_list_free(data->image_list); } free(data); return; } === Thread-safety === task_overlay_handler runs on the retro_task worker thread pool; input_overlay_loaded runs on the main thread as the task's completion callback. The ownership transfer of task_data via task_set_data is already correct (producer populates, consumer frees), so no lock discipline changes. === Reachability === Overlay loading runs every time the user picks an overlay from the menu, configures an auto-load overlay, or switches between overlays (e.g. OSK toggle). On handheld and embedded targets with the memory pressure typical of running an emulator core + overlay textures, OOM at any of these four sites is realistic. === Scope === Left image_transfer / texture loading paths in task_overlay.c alone (the per-image malloc + image_texture_load calls all NULL-check and goto their cleanup labels already). The four fixes here are the OOM sites that didn't have any guard.
1 parent 0563af5 commit f07549b

2 files changed

Lines changed: 61 additions & 3 deletions

File tree

input/input_driver.c

Lines changed: 39 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3032,9 +3032,16 @@ static bool input_overlay_poll(
30323032
break;
30333033
case OVERLAY_TYPE_DPAD_AREA:
30343034
case OVERLAY_TYPE_ABXY_AREA:
3035-
input_overlay_get_eightway_state(
3036-
desc, desc->eightway_config,
3037-
&out->buttons, x_dist, y_dist);
3035+
/* Gate on eightway_config because task_overlay_desc_
3036+
* populate_eightway_config can legitimately fail its
3037+
* calloc under OOM and leave this field NULL for an
3038+
* area-type desc that requires it. Without the
3039+
* gate input_overlay_get_eightway_state NULL-derefs
3040+
* on eightway->slope_high below. */
3041+
if (desc->eightway_config)
3042+
input_overlay_get_eightway_state(
3043+
desc, desc->eightway_config,
3044+
&out->buttons, x_dist, y_dist);
30383045
break;
30393046
case OVERLAY_TYPE_ANALOG_RIGHT:
30403047
base = 2;
@@ -6220,7 +6227,36 @@ static void input_overlay_loaded(retro_task_t *task,
62206227
if (err)
62216228
return;
62226229

6230+
/* NULL-check task_data: the task producer (task_overlay_handler
6231+
* in tasks/task_overlay.c) can legitimately fail its data
6232+
* alloc under OOM and pass NULL here; data->overlays below
6233+
* would NULL-deref. Nothing useful to do with a NULL payload
6234+
* so bail cleanly. */
6235+
if (!data)
6236+
return;
6237+
62236238
ol = (input_overlay_t*)calloc(1, sizeof(*ol));
6239+
/* NULL-check the ol calloc: the field writes below NULL-deref
6240+
* on OOM. Task_data ownership transferred to us by the
6241+
* retro_task framework via task_set_data - we must tear it
6242+
* down cleanly. image_list entries each have a heap
6243+
* texture_image* in elems[i].attr.p that move_images would
6244+
* normally transfer into ol->images; since ol doesn't exist,
6245+
* free each texture explicitly before string_list_free
6246+
* (string_list_free only frees the list struct and its
6247+
* entries' .data strings, not their .attr.p attachments). */
6248+
if (!ol)
6249+
{
6250+
if (data->image_list)
6251+
{
6252+
size_t i;
6253+
for (i = 0; i < data->image_list->size; i++)
6254+
image_texture_free((struct texture_image*)data->image_list->elems[i].attr.p);
6255+
string_list_free(data->image_list);
6256+
}
6257+
free(data);
6258+
return;
6259+
}
62246260
ol->overlays = data->overlays;
62256261
ol->size = data->size;
62266262
ol->active = data->active;

tasks/task_overlay.c

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -231,6 +231,18 @@ static void task_overlay_desc_populate_eightway_config(
231231
calloc(1, sizeof(overlay_eightway_config_t));
232232
eightway = desc->eightway_config;
233233

234+
/* NULL-check: the switch body below writes eightway->up etc.
235+
* via BIT256_SET which would NULL-deref on OOM. void-returning
236+
* function, so we can't signal failure to the caller. Leaving
237+
* eightway_config = NULL on failure makes input_driver.c's
238+
* OVERLAY_TYPE_DPAD_AREA / ABXY_AREA dispatch path take its
239+
* 'if (desc->eightway_config)' gate and skip the area's input
240+
* handling - strictly better than a crash. The user-visible
241+
* consequence is that this one eightway area is dead until
242+
* the overlay is reloaded. */
243+
if (!eightway)
244+
return;
245+
234246
/* Populate default vals for the eightway type.
235247
*/
236248
switch (desc->type)
@@ -1111,6 +1123,16 @@ static void task_overlay_handler(retro_task_t *task)
11111123
overlay_task_data_t *data = (overlay_task_data_t*)
11121124
calloc(1, sizeof(*data));
11131125

1126+
/* NULL-check before the seven field writes below NULL-deref.
1127+
* On OOM skip the task_set_data call so the consumer
1128+
* (input/input_driver.c input_overlay_loaded) sees NULL
1129+
* task_data. That consumer now also NULL-checks its
1130+
* task_data argument (fixed in the same commit) - without
1131+
* that second fix this branch still crashes at the
1132+
* consumer, just one call further down the stack. */
1133+
if (!data)
1134+
return;
1135+
11141136
data->overlays = loader->overlays;
11151137
data->active = loader->active;
11161138
data->size = loader->size;

0 commit comments

Comments
 (0)