Skip to content

Commit 8fd7aa4

Browse files
committed
network/netplay/netplay_frontend: NULL-check istate_out in frame real_input copy path
netplay_sync_pre_frame's frame-replay loop had one unchecked netplay_input_state_for return value: else { /* Copy the previous input */ istate_out = netplay_input_state_for(&frame->real_input[device], client_num, istate_in->size, true, false); memcpy(istate_out->data, istate_in->data, istate_in->size * sizeof(uint32_t)); } netplay_input_state_for can return NULL on three paths: 1. Internal calloc OOM (the fresh-slot allocation path). 2. must_not_create = true and no matching slot found. 3. must_create = true and an existing slot exists with a different size (the 'return NULL' at line 2218). Path 3 is the realistic one here. The callsite passes must_create = true (4th param) and the frame's real_input slot list for this device might already have a state entry for client_num at a different size - e.g. if the client changed device type since this frame was initially written. Under that condition netplay_input_state_for returns NULL and the subsequent memcpy through istate_out->data NULL-derefs. All other callsites of netplay_input_state_for in this file already NULL-check the return: - netplay_device_client_state: 'if (simstate) return simstate' pattern. - several sites use 'if (!state) continue;' in per-client loops. - one netplay_cmd_nak callsite: 'if (!istate) return netplay_cmd_nak(...)' - sibling to this fix: just calls netplay_ input_state_for without using the result, so NULL is harmless. - one site: 'if (!istate) continue;' with an explicit /* FIXME: More severe? */ comment. This one site was the outlier. Match the convention of the other callsites in the surrounding sync-loop context by guarding the memcpy with an 'if (istate_out)'. The outer loop's frame->have_real[client_num] = true marker still fires, so the sync pass treats the frame as processed; the next sync iteration re-attempts the copy with a fresh state_for request. === Swept-clean in the same pass === Verified NULL-checked in network/netplay/: - netplay_frontend.c 22 alloc sites: discovered_hosts.hosts malloc + realloc (both guarded), sbuf->data malloc (guarded), sram_buf malloc (guarded), netplay_input_state callocs with shared NULL-check, newdata malloc (guarded), netplay->connections calloc + realloc (guarded), netplay->buffer[i].state realloc (guarded), mitm_handler calloc (guarded with 'if' wrap), netplay->buffer[i].state / zbuffer / buffer callocs (all guarded), netplay_t calloc (guarded), initial connections calloc in netplay_new (goto failure), input- handler buf mallocs (guarded), ban_list malloc + realloc (guarded), client_info calloc (guarded), core_netpacket_interface malloc (guarded with prior audit comment). - netplay_room_parse.c 3 sites: head / tail room callocs and rooms_data wrapper calloc, all guarded with prior audit comments (the head/tail pair specifically gates on rooms_data->cur being non-NULL to avoid the 'prior room failed' NULL-deref cascade). - All other netplay_input_state_for callsites NULL-check or intentionally ignore the return. Thread-safety: netplay_sync_pre_frame and the surrounding frame-replay code run on the main netplay thread. No concurrent mutation of frame->real_input from other threads. Reachability: the size-mismatch path is triggered by device reconfiguration mid-game - e.g. a player switching their controller between Joypad and Analog types while netplay is running. Not common, but realistic for long-duration sessions where a player wants to switch to a different control scheme. Under the pre-patch code that triggered a NULL-deref crash in the server's sync handler; post-patch the copy is skipped for that device/client and the next sync pass retries.
1 parent ae25da3 commit 8fd7aa4

1 file changed

Lines changed: 16 additions & 2 deletions

File tree

network/netplay/netplay_frontend.c

Lines changed: 16 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6689,8 +6689,22 @@ static void netplay_handle_slaves(netplay_t *netplay)
66896689
/* Copy the previous input */
66906690
istate_out = netplay_input_state_for(&frame->real_input[device],
66916691
client_num, istate_in->size, true, false);
6692-
memcpy(istate_out->data, istate_in->data,
6693-
istate_in->size * sizeof(uint32_t));
6692+
/* NULL-check: netplay_input_state_for can return
6693+
* NULL either on an internal calloc OOM or when
6694+
* a 'must_create' request finds an existing slot
6695+
* with different size. The memcpy through
6696+
* istate_out->data below NULL-derefs either way.
6697+
* Other callsites (3078, 3087, 3095, 3148, 3185,
6698+
* 5477, 5766, 8025) NULL-check and continue;
6699+
* match that convention. Skipping just this one
6700+
* device-slot copy is consistent with how the
6701+
* outer loop handles other per-device failures -
6702+
* the frame's have_real gets set below as a
6703+
* 'we tried' signal and the next sync pass will
6704+
* retry. */
6705+
if (istate_out)
6706+
memcpy(istate_out->data, istate_in->data,
6707+
istate_in->size * sizeof(uint32_t));
66946708
}
66956709
}
66966710
frame->have_real[client_num] = true;

0 commit comments

Comments
 (0)