Skip to content

Commit 747a8ab

Browse files
committed
command: fix OOM leaks and NULL-deref in command_{network,stdin,emscripten}_new
Three sibling functions that construct command_t handles (command_network_new, command_stdin_new, command_emscripten_new) all followed the same broken allocation order: cmd = calloc(...); inner = calloc(...); if (!cmd) return NULL; if (!inner) { free(cmd); return NULL; } If cmd's calloc fails but inner's succeeds, the '!cmd' early return leaks inner. Likewise in command_network_new, whose error handler was actually fine at the end of the function, the code before it did netcmd->net_fd = fd; cmd->userptr = netcmd; with no NULL check between the two callocs and the assignments - an OOM on either calloc would segfault on the very next line, never reaching the error label. The sibling command_uds_new already uses the correct sequential pattern: allocate, check, allocate, check-with-cleanup-of-first. Apply that pattern to the other three. command_network_new also gets one unrelated but same-function fix: the 'error:' label's teardown did not call socket_close() on fd, so the socket_nonblock / socket_bind failure paths (which run AFTER fd was opened and assigned into netcmd->net_fd) leaked the socket file descriptor. Moved fd into a local initialised to -1 so the single 'if (fd >= 0) socket_close(fd)' branch in the error handler covers every exit path that had a real fd, without relying on netcmd being non-NULL. Also moves the 'socket_init' call after the two callocs rather than interleaved with them. This has no behavioural consequence - socket_init's side effects (network_init, getaddrinfo) are idempotent / reversible via freeaddrinfo_retro - but keeps the allocate-check-allocate-check block together, which is what the whole refactor is aiming for in the first place. No scope for OOM-on-small-calloc to actually hit these paths in current practice (all three functions run once at RetroArch startup, not per-command-message), so this is correctness-only rather than a user-visible fix. Still worth doing - the NULL-deref in particular is a real bug waiting for the right-shaped OOM.
1 parent a36d996 commit 747a8ab

1 file changed

Lines changed: 42 additions & 14 deletions

File tree

command.c

Lines changed: 42 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -236,10 +236,25 @@ static void command_network_poll(command_t *handle)
236236
command_t* command_network_new(uint16_t port)
237237
{
238238
struct addrinfo *res = NULL;
239-
command_t *cmd = (command_t*)calloc(1, sizeof(*cmd));
240-
command_network_t *netcmd = (command_network_t*)calloc(
241-
1, sizeof(command_network_t));
242-
int fd = socket_init((void**)&res, port, NULL,
239+
command_t *cmd = NULL;
240+
command_network_t *netcmd = NULL;
241+
int fd = -1;
242+
243+
/* Allocate-then-check in sequence, matching command_uds_new.
244+
* The previous code allocated cmd + netcmd back-to-back and then
245+
* assigned netcmd->net_fd / cmd->userptr before NULL-checking
246+
* either pointer - an OOM on either calloc would NULL-deref on
247+
* line 252/253. It also leaked netcmd on the '!cmd' failure
248+
* path via the 'error' label (which now cleanly frees both
249+
* because they are initialised to NULL up-front; free(NULL) is
250+
* a no-op). */
251+
if (!(cmd = (command_t*)calloc(1, sizeof(*cmd))))
252+
goto error;
253+
254+
if (!(netcmd = (command_network_t*)calloc(1, sizeof(command_network_t))))
255+
goto error;
256+
257+
fd = socket_init((void**)&res, port, NULL,
243258
SOCKET_TYPE_DATAGRAM, AF_INET);
244259

245260
RARCH_LOG("[NetCMD] %s %hu.\n",
@@ -271,6 +286,12 @@ command_t* command_network_new(uint16_t port)
271286
error:
272287
if (res)
273288
freeaddrinfo_retro(res);
289+
/* The only path that reaches 'error' with fd >= 0 is the
290+
* socket_nonblock / socket_bind failure after fd was already
291+
* stored in netcmd->net_fd. Close it so we don't leak the
292+
* socket file descriptor along with the allocations. */
293+
if (fd >= 0)
294+
socket_close(fd);
274295
free(netcmd);
275296
free(cmd);
276297
return NULL;
@@ -351,16 +372,20 @@ command_t* command_stdin_new(void)
351372
#endif
352373
#endif
353374

354-
cmd = (command_t*)calloc(1, sizeof(command_t));
355-
stdincmd = (command_stdin_t*)calloc(1, sizeof(command_stdin_t));
356-
357-
if (!cmd)
375+
/* Allocate in order with per-step failure handling. The earlier
376+
* form allocated both cmd and stdincmd back-to-back then checked
377+
* them, which leaked stdincmd on the '!cmd' failure path (the
378+
* '!cmd' return simply dropped the stdincmd pointer). Also
379+
* matches the command_uds_new pattern further down. */
380+
if (!(cmd = (command_t*)calloc(1, sizeof(command_t))))
358381
return NULL;
359-
if (!stdincmd)
382+
383+
if (!(stdincmd = (command_stdin_t*)calloc(1, sizeof(command_stdin_t))))
360384
{
361385
free(cmd);
362386
return NULL;
363387
}
388+
364389
cmd->userptr = stdincmd;
365390
cmd->poll = command_stdin_poll;
366391
cmd->replier = stdin_command_reply;
@@ -402,16 +427,19 @@ command_t* command_emscripten_new(void)
402427
command_t *cmd;
403428
command_emscripten_t *emscriptencmd;
404429

405-
cmd = (command_t*)calloc(1, sizeof(command_t));
406-
emscriptencmd = (command_emscripten_t*)calloc(1, sizeof(command_emscripten_t));
407-
408-
if (!cmd)
430+
/* Same sequential-allocation-with-per-step-cleanup pattern as
431+
* command_stdin_new / command_uds_new. Previously allocated both
432+
* structs before any NULL check, leaking emscriptencmd on the
433+
* '!cmd' failure path. */
434+
if (!(cmd = (command_t*)calloc(1, sizeof(command_t))))
409435
return NULL;
410-
if (!emscriptencmd)
436+
437+
if (!(emscriptencmd = (command_emscripten_t*)calloc(1, sizeof(command_emscripten_t))))
411438
{
412439
free(cmd);
413440
return NULL;
414441
}
442+
415443
cmd->userptr = emscriptencmd;
416444
cmd->poll = command_emscripten_poll;
417445
cmd->replier = emscripten_command_reply;

0 commit comments

Comments
 (0)