Skip to content

Commit 1efeddc

Browse files
committed
network/cloud_sync + netplay: fix NULL-deref, OOM crashes, and realloc-to-self leak
Four bugs found in a sweep of network/ alloc sites. === network/cloud_sync/s3.c: s3_url_encode / s3_trim_string === Both helpers had the same dead-code NULL guard: static char* s3_url_encode(const char *input) { size_t input_len = strlen(input); /* segfaults on NULL */ size_t output_pos = 0; char *output = malloc(input_len * 3 + 1); size_t i; if (!output) return NULL; if (!input) /* dead code */ return NULL; ... The 'if (!input)' check sat after both strlen(input) and the malloc sized by strlen's result, so a NULL input crashed in strlen before we ever reached the guard. s3_trim_string at line 443 had the identical pattern. Fix: move the NULL-check to run FIRST, then strlen, then malloc (also NULL-checked). Dropped the dead post-strlen guard. These callers feed arbitrary header values / URL path components through these helpers; a malformed S3 response or a malformed user-supplied bucket/path was one NULL away from crashing the cloud-sync subsystem. === network/cloud_sync/s3.c: s3_build_auth_header === Straight OOM NULL-deref: auth_header = malloc(1024); snprintf(auth_header, 1024, ...); /* NULL-deref on OOM */ Also leaked 'signature' (heap-allocated just above by s3_calculate_signature_v4_with_time) on the crash path - the free at the bottom of the function was unreachable. Fix: NULL-check the malloc, free signature + return NULL on OOM. === network/cloud_sync/google_drive.c: gdrive_do_patch === filestream_seek(cb_st->rfile, 0, SEEK_SET); len = filestream_get_size(cb_st->rfile); buf = malloc((size_t)(len + 1)); /* unchecked */ filestream_read(cb_st->rfile, buf, len); /* NULL-deref */ ... task_push_http_transfer_with_content(url, "PATCH", buf, (size_t)len, ...); /* sends NULL */ Two issues folded together: - malloc not NULL-checked. - filestream_get_size returns int64_t; on file-not-readable / error it returns negative. (size_t)(-1 + 1) == 0, so malloc(0) is either NULL or a valid zero-sized pointer depending on libc, and the subsequent filestream_read with a negative size_t-cast len is undefined at best. Fix: bail early on len <= 0 (the patch is a PUT of the file's contents, empty or error both mean nothing to upload), then NULL-check the malloc. Function is void-returning, so on OOM just silently skip this upload and let the next sync poll retry. === network/netplay/netplay_frontend.c: state-buffer growth === When a peer sends a state_size larger than ours, all buffer entries are grown to match: netplay->state_size = state_size; for (i = 0; i < netplay->buffer_size; i++) { netplay->buffer[i].state = realloc(netplay->buffer[i].state, netplay->state_size); if (!netplay->buffer[i].state) return false; } Classic realloc-assign-self leak: on OOM realloc returns NULL but leaves the prior buffer[i].state allocation intact; the self-assign overwrites the only pointer to it and leaks the whole ring entry. Fix: realloc into a temp, NULL-check, commit to the ivar only on success. Still return false on OOM; the caller disconnects the netplay connection on false return, so the partially-grown buffer (entries 0..i-1 at new size, i..end at old size) is about to be torn down anyway and the inconsistency is transient. === Thread-safety === Unchanged. s3 and google_drive cloud-sync helpers run on the task-queue worker thread; netplay state handling runs on the netplay thread. No new locks or ordering requirements introduced. === Reachability === Cloud-sync helpers fire on every background sync poll (configurable, default ~5-minute interval). s3_url_encode and s3_trim_string are called on every request path / header field. The netplay realloc is hit when a peer with a different-version core joins and the two sides disagree on save-state size. All four are reachable in realistic runtime conditions.
1 parent 7f745a9 commit 1efeddc

3 files changed

Lines changed: 56 additions & 15 deletions

File tree

network/cloud_sync/google_drive.c

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1342,7 +1342,19 @@ static void gdrive_do_patch(gdrive_cb_state_t *cb_st)
13421342

13431343
filestream_seek(cb_st->rfile, 0, SEEK_SET);
13441344
len = filestream_get_size(cb_st->rfile);
1345-
buf = malloc((size_t)(len + 1));
1345+
/* filestream_get_size returns negative on error; malloc of
1346+
* (size_t)(len + 1) would wrap or be zero, and the subsequent
1347+
* filestream_read(rfile, buf, len) would pass a negative size
1348+
* down. Bail early on either error or empty file. */
1349+
if (len <= 0)
1350+
return;
1351+
/* NULL-check the malloc before filestream_read / the HTTP
1352+
* transfer below dereference 'buf'. On OOM there's no
1353+
* recoverable action for a PATCH upload of the save/state
1354+
* file - silently skip this call and let the sync retry on
1355+
* the next poll. */
1356+
if (!(buf = malloc((size_t)(len + 1))))
1357+
return;
13461358
filestream_read(cb_st->rfile, buf, len);
13471359

13481360
snprintf(url, sizeof(url),

network/cloud_sync/s3.c

Lines changed: 31 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -397,20 +397,24 @@ static char* s3_sha256_hash(const char *data, size_t len)
397397
/* URL encode a string according to RFC 3986 */
398398
static char* s3_url_encode(const char *input)
399399
{
400-
size_t input_len = strlen(input);
400+
size_t input_len;
401401
size_t output_pos = 0;
402-
403-
/* Worst case: every char needs encoding */
404-
char *output = malloc(input_len * 3 + 1);
405-
402+
char *output;
406403
size_t i;
407-
408-
if (!output)
409-
return NULL;
410-
404+
405+
/* NULL-check input BEFORE calling strlen. The previous form
406+
* ran strlen(input) first (line-order: strlen, malloc, then
407+
* 'if (!input)'), so the 'if (!input)' check was dead code -
408+
* a NULL input segfaulted in strlen before we ever reached
409+
* the guard. */
411410
if (!input)
412411
return NULL;
413412

413+
input_len = strlen(input);
414+
415+
/* Worst case: every char needs encoding */
416+
if (!(output = malloc(input_len * 3 + 1)))
417+
return NULL;
414418

415419
for (i = 0; i < input_len; i++)
416420
{
@@ -440,15 +444,22 @@ static char* s3_url_encode(const char *input)
440444
/* Trim whitespace from string */
441445
static char* s3_trim_string(const char *input)
442446
{
443-
size_t len = strlen(input);
447+
size_t len;
444448
size_t start = 0;
445-
size_t end = len;
449+
size_t end;
446450
size_t trimmed_len = 0;
447451
char *trimmed = NULL;
448452

453+
/* NULL-check input BEFORE calling strlen. Same dead-code bug
454+
* as s3_url_encode: the 'if (!input)' check sat after the
455+
* strlen call, so NULL input segfaulted before the guard
456+
* could fire. */
449457
if (!input)
450458
return NULL;
451459

460+
len = strlen(input);
461+
end = len;
462+
452463
/* Find start of non-whitespace */
453464
while (start < len && (input[start] == ' ' || input[start] == '\t'))
454465
start++;
@@ -841,7 +852,15 @@ static char* s3_build_auth_header(const char *method, const char *canonical_uri,
841852
s3_st->access_key_id, date, s3_st->region, S3_SERVICE);
842853

843854
/* Build authorization header */
844-
auth_header = malloc(1024);
855+
/* NULL-check the malloc: snprintf-into-NULL segfaults. Also
856+
* free 'signature' on the OOM path - the success path at line
857+
* below already free's it, but the pre-patch code would have
858+
* crashed before reaching that free. */
859+
if (!(auth_header = malloc(1024)))
860+
{
861+
free(signature);
862+
return NULL;
863+
}
845864
snprintf(auth_header, 1024,
846865
"Authorization: %s Credential=%s, SignedHeaders=host;x-amz-content-sha256;x-amz-date, Signature=%s\r\n"
847866
"x-amz-date: %s\r\n"

network/netplay/netplay_frontend.c

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6104,9 +6104,19 @@ static bool netplay_get_cmd(netplay_t *netplay,
61046104
netplay->state_size = state_size;
61056105
for (i = 0; i < netplay->buffer_size; i++)
61066106
{
6107-
netplay->buffer[i].state = realloc(netplay->buffer[i].state, netplay->state_size);
6108-
if (!netplay->buffer[i].state)
6107+
/* realloc-to-tmp to avoid the classic realloc-
6108+
* assign-self leak: on OOM realloc returns NULL
6109+
* but leaves buffer[i].state pointing at the
6110+
* old allocation; self-assign overwrites the
6111+
* only pointer and leaks it. Propagate failure
6112+
* up - the caller tears down the netplay
6113+
* connection on false return, so the partially-
6114+
* grown buffer (0..i-1 at new size, i..end still
6115+
* at old size) is about to be torn down anyway. */
6116+
void *tmp = realloc(netplay->buffer[i].state, netplay->state_size);
6117+
if (!tmp)
61096118
return false;
6119+
netplay->buffer[i].state = tmp;
61106120
}
61116121
}
61126122

0 commit comments

Comments
 (0)