Commit 7f745a9
committed
input: fix OOM handling in wl_read_pipe and rwebinput_keyboard_cb
Two independent real OOM bugs in input-path buffer-growth code.
=== input/common/wayland_common.c: wl_read_pipe ===
The function reads one PIPE_BUF-sized chunk off a Wayland clipboard
/ drag-and-drop pipe into '*buffer', growing it per-call. The
caller's loop pattern is:
while (wl_read_pipe(pipefd[0], &buffer, length, null_terminate) > 0);
i.e. keep reading until bytes_read <= 0 (0 = EOF, <0 = error).
Pre-patch, when the malloc / realloc inside the function failed,
the else-branch of the 'if (output_buffer)' alloc check was
missing entirely. The flow was:
pos = *total_length;
*total_length += bytes_read; /* committed early */
...
output_buffer = realloc(*buffer, _len);
if (output_buffer) { memcpy(...); *buffer = output_buffer; }
/* else: fall through, return bytes_read (positive!) */
On OOM:
- The bytes we just read off the pipe are silently dropped
(temp[] is a stack buffer, overwritten on the next read()).
- *total_length has already been incremented, so the caller
believes the buffer has grown.
- *buffer is unchanged - on realloc failure it still points at
the old, smaller allocation. So the invariant 'size of
*buffer is *total_length' is broken: the recorded length is
larger than the actual allocation.
- Return value is bytes_read > 0, so the caller's while-loop
continues. Next iteration:
pos = *total_length /* over-sized */
*total_length += bytes_read
output_buffer = realloc(*buffer, _len)
If that one succeeds, the memcpy at offset 'pos' writes past
the end of what was actually in *buffer - corrupting arbitrary
heap memory if realloc gave us a buffer exactly sized to the
caller's new *total_length but missing the gap from the
failed iteration.
Fix: on allocation failure, rewind *total_length to pos (the
pre-increment value) and return -1. The caller's while-loop
then terminates; the partial data already stored in *buffer
remains valid and consistent with the (now-un-incremented)
*total_length. The current-iteration's bytes_read are lost,
which is the same failure mode as any other mid-transfer pipe
read error (not worse).
=== input/drivers/rwebinput_input.c: rwebinput_keyboard_cb ===
The Emscripten rwebinput driver grows its pending-keyboard-event
ring on demand inside the keyboard callback:
if (rwebinput->keyboard.count >= rwebinput->keyboard.max_size)
{
size_t new_max = MAX(1, rwebinput->keyboard.max_size << 1);
rwebinput->keyboard.events = realloc(rwebinput->keyboard.events,
new_max * sizeof(rwebinput->keyboard.events[0]));
rwebinput->keyboard.max_size = new_max;
}
rwebinput->keyboard.events[rwebinput->keyboard.count].type = event_type;
memcpy(&rwebinput->keyboard.events[...].event, key_event, ...);
Two bugs:
(1) 'events = realloc(events, ...)' is the classic realloc-assign-
self leak: on OOM realloc returns NULL but leaves the original
buffer valid; the self-assign overwrites the only pointer to
it, leaking the entire pending-event ring (and any events
already in it that weren't yet consumed).
(2) max_size is updated unconditionally after the realloc, before
the return value is inspected. Then the field-write and
memcpy below dereference the now-NULL events pointer - a
NULL-deref crash. Any state that survived would claim
'max_size holds 2N events' while actually holding zero.
Fix: realloc into a temp, NULL-check, and only commit both
events-pointer and max_size on success. On OOM drop this one
event on the floor and return EM_TRUE (the Emscripten callback
convention); losing a single key event in a memory-starved
browser tab is survivable - the pre-patch form was going to
crash the whole page.
=== Thread-safety ===
Neither path's locking changes. wl_read_pipe runs on whatever
thread services the Wayland event loop; rwebinput_keyboard_cb
runs on the Emscripten main thread.
=== Reachability ===
wl_read_pipe fires on every clipboard paste / drag-and-drop drop
onto a Wayland-native RetroArch window. rwebinput_keyboard_cb
fires on every keyboard event in the web build; the ring-growth
path triggers only when the input consumer falls behind (rare
under normal play, common during loading screens where the main
thread is blocked).1 parent dc79a1e commit 7f745a9
2 files changed
Lines changed: 47 additions & 3 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
900 | 900 | | |
901 | 901 | | |
902 | 902 | | |
| 903 | + | |
| 904 | + | |
| 905 | + | |
| 906 | + | |
| 907 | + | |
| 908 | + | |
| 909 | + | |
| 910 | + | |
| 911 | + | |
| 912 | + | |
| 913 | + | |
| 914 | + | |
| 915 | + | |
| 916 | + | |
| 917 | + | |
| 918 | + | |
| 919 | + | |
| 920 | + | |
| 921 | + | |
| 922 | + | |
| 923 | + | |
903 | 924 | | |
904 | 925 | | |
905 | 926 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
261 | 261 | | |
262 | 262 | | |
263 | 263 | | |
264 | | - | |
265 | | - | |
| 264 | + | |
| 265 | + | |
266 | 266 | | |
267 | | - | |
| 267 | + | |
| 268 | + | |
| 269 | + | |
| 270 | + | |
| 271 | + | |
| 272 | + | |
| 273 | + | |
| 274 | + | |
| 275 | + | |
| 276 | + | |
| 277 | + | |
| 278 | + | |
| 279 | + | |
| 280 | + | |
| 281 | + | |
| 282 | + | |
| 283 | + | |
| 284 | + | |
| 285 | + | |
| 286 | + | |
| 287 | + | |
| 288 | + | |
| 289 | + | |
| 290 | + | |
268 | 291 | | |
269 | 292 | | |
270 | 293 | | |
| |||
0 commit comments