Skip to content

Commit 6cf158d

Browse files
committed
libretro-common/lists/string_list: fail string_list_clone on inner malloc OOM
Pre-patch: if per-element data malloc failed partway through the clone loop, the inner branch was: if (slen != 0) { char *ret = (char*)malloc(slen + 1); if (ret) { memcpy(ret, _src, slen + 1); dest->elems[i].data = ret; } } On OOM, dest->elems[i].data silently stayed NULL (pre-set at the top of the iteration) and the loop continued. The final return handed back a list with dest->size set to src->size but some .data pointers NULL. This trap fires on every downstream consumer that assumes .data is non-NULL, which is all of them: * string_list_find_elem at line 349 dereferences elems[i].data unconditionally in the (*p1 | 32) compare. * string_list_join_concat at line 200 strlcpy's from .data; strlcpy(dst, NULL, ...) is UB (EFAULT on glibc, crash elsewhere). * string_list_join_concat_special at line 214 - same. * string_list_find_elem_prefix at line 377 - tolower on *data. Empty-string list entries are represented with slen == 0 and never enter the malloc branch, so a NULL .data after the loop unambiguously means 'OOM happened here' rather than 'this was intended to be empty'. Fix: on malloc failure, tear down everything we've allocated so far (the j < i .data buffers, the elems array, the dest struct) and return NULL. Callers of string_list_clone already handle NULL returns: * core_info.c clones eight lists with the pattern 'dst->x = src->x ? string_list_clone(src->x) : NULL', treating NULL as valid cloned-state. * audio/drivers/{pipewire,pulse}.c return the clone result directly; their callers must handle NULL anyway for the no-devicelist case. * runahead.c and tasks/task_netplay_find_content.c similarly assign and later test. NULL is strictly safer and more informative than a silently-corrupted list. Thread-safety: string_list_clone takes a const src and produces a fresh dest; no shared-state mutations. No lock discipline changes. Reachability: every deep-clone of a string_list, which is common for core_info records and audio device enumerations. Per-element OOM is realistic on memory-tight platforms (embedded handhelds, PS2, Vita) where core_info can carry hundreds of entries each with several sub-lists of short strings. Scope: one function, one failure mode. The surrounding string_list_new, string_list_initialize, and other allocators in this file were also on my audit list but they're already NULL-checked properly - verified during this pass, no changes needed there.
1 parent 689fc11 commit 6cf158d

1 file changed

Lines changed: 27 additions & 3 deletions

File tree

libretro-common/lists/string_list.c

Lines changed: 27 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -431,11 +431,35 @@ struct string_list *string_list_clone(const struct string_list *src)
431431
if (slen != 0)
432432
{
433433
char *ret = (char*)malloc(slen + 1);
434-
if (ret)
434+
/* Pre-patch: on malloc failure 'dest->elems[i].data'
435+
* silently stayed NULL and the loop moved on. That left
436+
* the caller holding a dest with dest->size set to
437+
* src->size but some .data pointers NULL - a trap for
438+
* every consumer that assumes .data is non-NULL.
439+
* string_list_find_elem (line 349), string_list_
440+
* join_concat (line 200), and others all dereference
441+
* .data unconditionally. Single empty-string inputs
442+
* are represented by slen == 0 and never hit this
443+
* branch, so NULL here always means OOM, not 'empty
444+
* element was intended'.
445+
*
446+
* Tear down the partially-cloned destination and
447+
* return NULL. Callers of string_list_clone already
448+
* handle NULL returns (clone-failure is an OOM signal)
449+
* - NULL is both safer and more informative than a
450+
* silently-corrupted list. */
451+
if (!ret)
435452
{
436-
memcpy(ret, _src, slen + 1); /* memcpy > strcpy: no NUL scan */
437-
dest->elems[i].data = ret;
453+
size_t j;
454+
for (j = 0; j < i; j++)
455+
if (dest->elems[j].data)
456+
free(dest->elems[j].data);
457+
free(dest->elems);
458+
free(dest);
459+
return NULL;
438460
}
461+
memcpy(ret, _src, slen + 1); /* memcpy > strcpy: no NUL scan */
462+
dest->elems[i].data = ret;
439463
}
440464
}
441465

0 commit comments

Comments
 (0)