Skip to content

Commit 9faa654

Browse files
committed
network/netplay/netplay_room_parse: NULL-check inner per-room callocs
The two per-room struct netplay_room callocs inside the netplay_json_start_object rjson callback were unchecked; on OOM subsequent code in the same function (->connectable = true, ->is_retroarch = true) and every field-handler callback below (object_member, string, number, boolean) would NULL-deref net_st->rooms_data->cur or NULL-deref the wrong field via '&net_st->rooms_data->cur->port' style address-of expressions. This finishes off a deferred item called out in the commit message for '3785495 scattered OOM fixes: core_backup entries leak, netplay core_netpacket_interface and rooms_data' (this series), which added the outer rooms_data NULL-check but left the inner per-room allocations noted as out-of-scope. === The original code === static bool netplay_json_start_object(void* ctx) { if (p_ctx->state == STATE_FIELDS_START) { p_ctx->state = STATE_FIELDS_OBJECT_START; if (!net_st->rooms_data->head) { net_st->rooms_data->head = calloc(1, ...); /* A */ net_st->rooms_data->cur = net_st->rooms_data->head; } else if (!net_st->rooms_data->cur->next) { net_st->rooms_data->cur->next = calloc(1, ...); /* B */ net_st->rooms_data->cur = net_st->rooms_data->cur->next; } net_st->rooms_data->cur->connectable = true; /* NULL-deref */ net_st->rooms_data->cur->is_retroarch = true; /* NULL-deref */ } ... } Two ways this goes wrong on OOM: 1. calloc A or B returns NULL -> cur becomes NULL -> the two immediate field writes at the bottom of this if-block NULL-deref. 2. If a *previous* room's calloc failed, cur is NULL coming into this call. The 'else if (!cur->next)' branch NULL-derefs cur->next before we even get to the calloc. Every downstream callback (object_member, string, number, boolean) then accesses 'net_st->rooms_data->cur->fieldname' unconditionally and NULL-derefs too. === The fix === 1. Add a new parser state STATE_SKIP_OBJECT to the netplay_parse_state enum. When the inner calloc fails or when cur is NULL from a prior failure, transition into SKIP_OBJECT instead of STATE_FIELDS_OBJECT_START. 2. Every field-handler callback (boolean, string, number, object_member) already gates on state == STATE_FIELDS_OBJECT_START, so they silently skip when state is SKIP_OBJECT. No changes needed in those handlers. 3. netplay_json_end_object transitions both STATE_FIELDS_OBJECT_START and STATE_SKIP_OBJECT back to STATE_ARRAY_START, so the next JSON object in the rooms array can be attempted. 4. Restructure start_object to use a local 'struct netplay_room *new_room' temporary so the calloc return can be NULL-checked before wiring it into head/cur. Guard the 'else if' branch on cur being non-NULL. === Self-healing behaviour === The fix lets the parser self-heal across rooms: * If the first room's calloc fails: head stays NULL. The next iteration of start_object sees !head true and retries calloc. If it succeeds, the new room becomes head as normal. * If a later room's calloc fails: cur points to the last successfully-allocated room, which still has next==NULL. The next iteration re-enters the 'else if (cur && !cur->next)' branch, retries calloc, and links the new room in as if the failed room never existed. No holes in the linked list; only the failed room is dropped. === netplay_rooms_free robustness === Walks the linked list from head, freeing each room. Handles head==NULL (no rooms were allocatable) and partial lists (some rooms succeeded, some failed) correctly - it just walks what was successfully linked in. No changes needed. === Thread-safety === netplay_rooms_parse is called on the main thread (from task callbacks in menu_cbs_ok.c after an HTTP fetch completes). The rjson parser runs synchronously on the same thread. No new locking required. === Reachability === Every netplay lobby browse triggers this parser with the server response. OOM here is plausible on memory-pressured handheld targets (Retroid, Anbernic, etc) where users are most likely to be browsing for netplay opponents. The pre- patch behaviour was a segfault on the first unchecked field write; the post-patch behaviour is a partially-populated rooms list (with the failing rooms silently dropped). === Verification === Compile-tested with: gcc -c -I. -Iinclude -Ilibretro-common/include \ -DHAVE_CONFIG_H -DRARCH_INTERNAL -DHAVE_NETWORKING \ network/netplay/netplay_room_parse.c No warnings or errors. State machine walked through by hand for: empty JSON, one-room success, two-room success, first- room OOM, second-room OOM followed by successful third room.
1 parent 2f048a4 commit 9faa654

1 file changed

Lines changed: 61 additions & 20 deletions

File tree

network/netplay/netplay_room_parse.c

Lines changed: 61 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,14 @@ enum netplay_parse_state
3333
STATE_OBJECT_START,
3434
STATE_FIELDS_START,
3535
STATE_FIELDS_OBJECT_START,
36+
/* Entered when the inner per-room calloc in
37+
* netplay_json_start_object fails or when the preceding
38+
* room's allocation failed. All field-write handlers gate
39+
* on STATE_FIELDS_OBJECT_START so they silently skip while
40+
* state == STATE_SKIP_OBJECT; netplay_json_end_object pops
41+
* back to STATE_ARRAY_START so the next room in the array
42+
* can be attempted. */
43+
STATE_SKIP_OBJECT,
3644
STATE_END
3745
};
3846

@@ -98,19 +106,49 @@ static bool netplay_json_start_object(void* ctx)
98106

99107
if (p_ctx->state == STATE_FIELDS_START)
100108
{
101-
p_ctx->state = STATE_FIELDS_OBJECT_START;
109+
struct netplay_room *new_room = NULL;
102110

103111
if (!net_st->rooms_data->head)
104112
{
105-
net_st->rooms_data->head = (struct netplay_room*)calloc(1, sizeof(*net_st->rooms_data->head));
106-
net_st->rooms_data->cur = net_st->rooms_data->head;
113+
/* First room in the list */
114+
new_room = (struct netplay_room*)
115+
calloc(1, sizeof(*new_room));
116+
if (new_room)
117+
{
118+
net_st->rooms_data->head = new_room;
119+
net_st->rooms_data->cur = new_room;
120+
}
121+
}
122+
else if (net_st->rooms_data->cur && !net_st->rooms_data->cur->next)
123+
{
124+
/* Subsequent room: append to the tail. Gate on
125+
* rooms_data->cur being non-NULL - if a prior room's
126+
* calloc failed, cur was set to NULL and we'd NULL-
127+
* deref 'cur->next' below. */
128+
new_room = (struct netplay_room*)
129+
calloc(1, sizeof(*new_room));
130+
if (new_room)
131+
{
132+
net_st->rooms_data->cur->next = new_room;
133+
net_st->rooms_data->cur = new_room;
134+
}
107135
}
108-
else if (!net_st->rooms_data->cur->next)
136+
137+
/* NULL-check: on OOM (inner calloc returned NULL) or on
138+
* the 'prior room failed' branch above (new_room stayed
139+
* NULL because we didn't enter either 'if' clause), skip
140+
* this entire JSON object. The object-member and value
141+
* handlers gate on STATE_FIELDS_OBJECT_START and so will
142+
* become no-ops; netplay_json_end_object transitions
143+
* SKIP_OBJECT back to ARRAY_START so the next array
144+
* element can be attempted. */
145+
if (!new_room)
109146
{
110-
net_st->rooms_data->cur->next = (struct netplay_room*)calloc(1, sizeof(*net_st->rooms_data->cur->next));
111-
net_st->rooms_data->cur = net_st->rooms_data->cur->next;
147+
p_ctx->state = STATE_SKIP_OBJECT;
148+
return true;
112149
}
113150

151+
p_ctx->state = STATE_FIELDS_OBJECT_START;
114152
net_st->rooms_data->cur->connectable = true;
115153
net_st->rooms_data->cur->is_retroarch = true;
116154
}
@@ -124,7 +162,11 @@ static bool netplay_json_end_object(void* ctx)
124162
{
125163
struct netplay_json_context *p_ctx = (struct netplay_json_context*)ctx;
126164

127-
if (p_ctx->state == STATE_FIELDS_OBJECT_START)
165+
/* Transition both the normal case (FIELDS_OBJECT_START) and
166+
* the OOM-skip case (SKIP_OBJECT) back to ARRAY_START so the
167+
* next room in the array can be attempted. */
168+
if ( p_ctx->state == STATE_FIELDS_OBJECT_START
169+
|| p_ctx->state == STATE_SKIP_OBJECT)
128170
p_ctx->state = STATE_ARRAY_START;
129171

130172
return true;
@@ -288,20 +330,19 @@ int netplay_rooms_parse(const char *buf, size_t len)
288330
net_st->rooms_data = (struct netplay_rooms*)
289331
calloc(1, sizeof(*net_st->rooms_data));
290332

291-
/* NULL-check: the rjson_parse_quick callbacks below (at
292-
* lines ~103, ~108, etc.) dereference net_st->rooms_data
293-
* unconditionally in the JSON member / object-start handlers.
294-
* On OOM bail before invoking the parser - 'return 0' is the
295-
* existing success return, but the caller (parse_lobby_json)
296-
* only iterates rooms if net_st->rooms_data is non-NULL, so
297-
* the no-rooms-available outcome matches the 'no entries in
298-
* the JSON' success path.
333+
/* NULL-check: the rjson_parse_quick callbacks below
334+
* dereference net_st->rooms_data unconditionally in the JSON
335+
* member / object-start handlers. On OOM bail before
336+
* invoking the parser - 'return 0' is the existing success
337+
* return, but the caller (parse_lobby_json) only iterates
338+
* rooms if net_st->rooms_data is non-NULL, so the no-rooms-
339+
* available outcome matches the 'no entries in the JSON'
340+
* success path.
299341
*
300-
* The inner per-room callocs at lines ~105 and ~110 are
301-
* still unchecked (they're deep inside rjson callbacks with
302-
* no practical way to propagate OOM - fixing them would
303-
* require threading an error flag through the entire
304-
* parse-context struct, out of scope here). */
342+
* The inner per-room callocs in netplay_json_start_object
343+
* now handle OOM by transitioning the parser state to
344+
* STATE_SKIP_OBJECT for the failed room; parsing continues
345+
* for subsequent rooms which may succeed. */
305346
if (!net_st->rooms_data)
306347
return 0;
307348

0 commit comments

Comments
 (0)