Skip to content

Commit b569dd0

Browse files
committed
Four independent defensive fixes, all bundled into a single combined
commit on top of master `b2d4002`. No CI workflow changes required. ``` round4_delivery/ ├── round4-combined.patch single unified patch, ~566 lines ├── commit_message.msg just the commit message body ├── README.md this file └── libretro-common-samples/ └── file/ ├── config_file/config_file_test.c extended sample └── nbio/nbio_test.c extended + cleaned-up sample ``` To apply: ``` cd RetroArch git apply round4-combined.patch git add -A git commit -F round4_delivery/commit_message.msg ``` Or in one step with `git am`: ``` git am round4-combined.patch ``` Full technical detail lives in the patch's commit message; the summary here is for quick reference. | # | File | What | |---|---|---| | 11 | `file/config_file.c` | NULL dangling pointer fields in `config_file_deinitialize` | | 12 | `file/config_file.c` | Check strdup return at three call sites | | 13 | `file/config_file.c` | `isgraph((int)char)` UB → cast to `unsigned char` | | 14 | `file/nbio/nbio_stdio.c` | `nbio_stdio_resize` realloc-before-commit | All four are "latent landmine" tier — correct today only because of indirect invariants or benign libc behaviour, not because of active exploitable corruption. Exception: patch 11 has a reachable UAF under a specific public-API usage pattern (demonstrated via ASan during test development). If triaging: 11 > 12 > 14 > 13. | Test | Kind | What it shows | |---|---|---| | `test_config_file_deinitialize_clears_fields` | **True regression discriminator** | Fails on unpatched source: `[FAILED] deinit left entries as dangling 0x...`; passes on patched | | `test_config_file_high_bit_bytes_smoke` | Smoke test | glibc doesn't fire on the pre-patch code so this passes on both sides on a typical Linux host. Value: would trip under UBSan ctype instrumentation pre-patch, and crashes on stricter libcs pre-patch. Documented in the test comment. | | `nbio_resize_smoke_test` | Smoke test | Forcing realloc to fail from user code would need an allocator hook, which breaks the self-contained-sample convention. Exercises the resize grow path normally; ASan catches buffer/length disagreement. | Honest note on test coverage: patch 12 (the strdup chain) has no targeted regression test. The failure modes are all OOM-triggered, and exercising them would require either an allocator hook or deliberately exhausting memory — both out of scope for the self-contained sample convention. The existing config_file_test cases implicitly exercise the non-OOM code paths, so a refactor that breaks the happy path would still be caught. While I was extending `nbio_test.c` I fixed two pre-existing issues that would have bit the CI workflow: * Return 0 regardless of `[ERROR]` output → now returns 1 if any `[ERROR]` was printed. This makes the test a real pass/fail signal for CI rather than just a build smoke. * Left `test.bin` in CWD after running → now cleans up. Also adds cleanup of the new `resize_test.bin` used by the resize smoke test. For each patch: 1. **Patch first, then test.** Applied each patch to the tree, confirmed clean compile with `-Wall -Werror`, and confirmed the existing sample tests still pass. 2. **Test against patched source.** Ran the new tests expecting they pass. 3. **Revert the source, keep the tests.** Ran the tests again. For patch 11 this fired the documented `[FAILED]` message; for 13 and 14 the smoke tests passed on both sides (documented as smoke tests, not discriminators). The patch 11 UAF was not just hypothetical — during test development I reproduced a concrete heap-use-after-free (ASan trace via `config_file_add_reference` after `config_file_deinitialize` on the same struct). - **14 patches** merged (across rounds 1-3) + **4 patches** this round = **18 total** - **7 regression test files** + extensions this round - **1 CI workflow** (added round 3 work) - Every round verified by discrimination-test methodology where feasible; smoke-test-labelled where not
1 parent b2d4002 commit b569dd0

4 files changed

Lines changed: 323 additions & 10 deletions

File tree

libretro-common/file/config_file.c

Lines changed: 46 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -246,7 +246,7 @@ static char *config_file_extract_value(char *line)
246246
size_t idx = 0;
247247
char *value = NULL;
248248
/* Find next space character */
249-
while (line[idx] && isgraph((int)line[idx]))
249+
while (line[idx] && isgraph((unsigned char)line[idx]))
250250
idx++;
251251

252252
line[idx] = '\0';
@@ -380,7 +380,11 @@ static void config_file_add_sub_conf(config_file_t *conf, char *path,
380380
{
381381
node->next = NULL;
382382
/* Add include list */
383-
node->path = strdup(path);
383+
if (!(node->path = strdup(path)))
384+
{
385+
free(node);
386+
goto realpath;
387+
}
384388

385389
if (head)
386390
{
@@ -393,6 +397,7 @@ static void config_file_add_sub_conf(config_file_t *conf, char *path,
393397
conf->includes = node;
394398
}
395399

400+
realpath:
396401
config_file_get_realpath(s, len, path,
397402
conf->path);
398403
}
@@ -569,7 +574,7 @@ static bool config_file_parse_line(config_file_t *conf,
569574
* then copy once - avoids malloc+realloc growth pattern */
570575
{
571576
const char *key_start = line;
572-
while (isgraph((int)*line))
577+
while (isgraph((unsigned char)*line))
573578
line++;
574579
idx = (size_t)(line - key_start);
575580
if (idx == 0)
@@ -705,6 +710,23 @@ bool config_file_deinitialize(config_file_t *conf)
705710

706711
RHMAP_FREE(conf->entries_map);
707712

713+
/* NULL out all pointer fields so that a caller who reuses the
714+
* struct after deinitialize() -- or who accidentally calls
715+
* deinitialize() twice -- does not chase dangling pointers. The
716+
* free() calls above leave every listed field pointing at freed
717+
* memory; without these NULLs any subsequent config_* call on
718+
* this struct is undefined behaviour. config_file_free() frees
719+
* the struct itself immediately after this, so for that path the
720+
* NULLs are redundant but harmless; config_file_deinitialize()
721+
* is a public API callable on its own. */
722+
conf->entries = NULL;
723+
conf->tail = NULL;
724+
conf->last = NULL;
725+
conf->includes = NULL;
726+
conf->references = NULL;
727+
conf->path = NULL;
728+
/* entries_map is cleared by RHMAP_FREE */
729+
708730
return true;
709731
}
710732

@@ -1116,11 +1138,19 @@ bool config_get_char(config_file_t *conf, const char *key, char *in)
11161138
bool config_get_string(config_file_t *conf, const char *key, char **str)
11171139
{
11181140
const struct config_entry_list *entry = config_get_entry(conf, key);
1141+
char *dup;
11191142

11201143
if (!entry || !entry->value)
11211144
return false;
11221145

1123-
*str = strdup(entry->value);
1146+
/* strdup can fail; pre-patch the function claimed success with
1147+
* *str possibly left as NULL or uninitialised garbage. Callers
1148+
* that don't defensively zero *str ahead of the call end up
1149+
* dereferencing a stale pointer. */
1150+
if (!(dup = strdup(entry->value)))
1151+
return false;
1152+
1153+
*str = dup;
11241154
return true;
11251155
}
11261156

@@ -1239,9 +1269,20 @@ void config_set_string(config_file_t *conf, const char *key, const char *val)
12391269
if (!(entry = (struct config_entry_list*)malloc(sizeof(*entry))))
12401270
return;
12411271
entry->readonly = false;
1272+
entry->next = NULL;
12421273
entry->key = strdup(key);
12431274
entry->value = strdup(val);
1244-
entry->next = NULL;
1275+
/* If either strdup failed, don't insert a half-initialised entry
1276+
* into the list or hash map -- RHMAP_SET_STR with a NULL key
1277+
* is undefined, and subsequent config_get_string/config_set_*
1278+
* calls on this key would chase a NULL key. */
1279+
if (!entry->key || !entry->value)
1280+
{
1281+
free(entry->key);
1282+
free(entry->value);
1283+
free(entry);
1284+
return;
1285+
}
12451286
conf->flags |= CONF_FILE_FLG_MODIFIED;
12461287
if (last)
12471288
last->next = entry;

libretro-common/file/nbio/nbio_stdio.c

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -252,14 +252,19 @@ static void nbio_stdio_resize(void *data, size_t len)
252252
if (len < handle->len)
253253
abort();
254254

255+
/* Attempt the realloc BEFORE committing the new length. If it
256+
* fails, the old pointer and its old size are still valid; the
257+
* caller can retry or abandon. Pre-patch we wrote handle->len
258+
* = len first, then on realloc failure the handle claimed it
259+
* owned a larger buffer than it actually did -- subsequent
260+
* fread/fwrite iterate up to handle->len and walk off the end. */
261+
if (!(new_data = realloc(handle->data, len)))
262+
return;
263+
264+
handle->data = new_data;
255265
handle->len = len;
256266
handle->progress = len;
257267
handle->op = -1;
258-
259-
new_data = realloc(handle->data, handle->len);
260-
261-
if (new_data)
262-
handle->data = new_data;
263268
}
264269

265270
static void *nbio_stdio_get_ptr(void *data, size_t* len)

libretro-common/samples/file/config_file/config_file_test.c

Lines changed: 149 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -227,6 +227,152 @@ static void test_config_get_int_accepts(const char *raw_val, int want_int)
227227
config_file_free(cfg);
228228
}
229229

230+
/* Regression for commit <round4-TBD> (config_file_deinitialize
231+
* leaves dangling pointers).
232+
*
233+
* config_file_deinitialize() is a public API. Pre-patch it freed
234+
* entries, includes, references, path and the hash map but left the
235+
* struct\'s pointer fields pointing at the just-freed memory. Any
236+
* subsequent call on that struct -- whether accidental double-
237+
* deinit, reuse, or another access via the public API -- chased
238+
* dangling pointers. Post-patch all fields are NULLed.
239+
*
240+
* This test loads a config, deinitializes it without freeing the
241+
* struct, then verifies that the struct\'s internal pointers are
242+
* all NULL. On unpatched code several of these would be stale
243+
* non-NULL pointers to freed memory.
244+
*
245+
* Note: this inspects the config_file_t fields directly (white-box
246+
* test). The public header exposes the struct definition so this
247+
* is legal, though a little unusual; there is no public getter for
248+
* "is this struct still live". The alternative -- provoking a
249+
* real UAF via a second API call -- would fire ASan on unpatched
250+
* but also crash on patched for unrelated reasons (add_reference
251+
* dereferences conf->path unconditionally). This direct field
252+
* inspection is the cleanest way to assert the patch\'s invariant.
253+
*/
254+
static void test_config_file_deinitialize_clears_fields(void)
255+
{
256+
config_file_t *cfg;
257+
const char *tmp_path = "rarch_cfg_deinit_test.cfg";
258+
FILE *fp = fopen(tmp_path, "wb");
259+
260+
if (!fp)
261+
abort();
262+
fputs("foo = \"bar\"\nbaz = \"qux\"\n", fp);
263+
fclose(fp);
264+
265+
cfg = config_file_new(tmp_path);
266+
remove(tmp_path);
267+
if (!cfg)
268+
abort();
269+
270+
/* Add a reference so conf->references is non-NULL before deinit. */
271+
config_file_add_reference(cfg, "some_ref");
272+
273+
/* Deinitialize without freeing the struct. */
274+
config_file_deinitialize(cfg);
275+
276+
/* Every pointer field must now be NULL. Pre-patch these would
277+
* be stale pointers to freed memory. */
278+
if (cfg->entries != NULL)
279+
{
280+
printf("[FAILED] deinit left entries as dangling %p\n", (void*)cfg->entries);
281+
free(cfg);
282+
abort();
283+
}
284+
if (cfg->includes != NULL)
285+
{
286+
printf("[FAILED] deinit left includes as dangling %p\n", (void*)cfg->includes);
287+
free(cfg);
288+
abort();
289+
}
290+
if (cfg->references != NULL)
291+
{
292+
printf("[FAILED] deinit left references as dangling %p\n", (void*)cfg->references);
293+
free(cfg);
294+
abort();
295+
}
296+
if (cfg->path != NULL)
297+
{
298+
printf("[FAILED] deinit left path as dangling %p\n", (void*)cfg->path);
299+
free(cfg);
300+
abort();
301+
}
302+
/* entries_map is cleared by RHMAP_FREE on all versions so we do
303+
* not check it here. */
304+
305+
printf("[SUCCESS] config_file_deinitialize cleared all dangling pointer fields\n");
306+
free(cfg);
307+
}
308+
309+
/* Smoke test for commit <round4-TBD> (isgraph((int)char) UB on
310+
* signed-char platforms).
311+
*
312+
* In the config parser, isgraph() is called on each byte of the
313+
* key / unquoted value to find the token end. Pre-patch the cast
314+
* was (int), so bytes >= 0x80 became negative ints on signed-char
315+
* platforms. The C standard says ctype functions must be called
316+
* with EOF or an unsigned-char value; anything else is undefined
317+
* behaviour. glibc and musl happen to handle negative arguments
318+
* gracefully, but stricter libcs (Solaris, some embedded toolchains)
319+
* trip an assert or array-bounds fault. Post-patch the cast is
320+
* (unsigned char).
321+
*
322+
* This is explicitly a smoke test: glibc and musl do not fire on
323+
* the pre-patch code either, so this test passes on both patched
324+
* and unpatched sources when run on a typical Linux host. Its
325+
* value is two-fold:
326+
* - Under UBSan with ctype function-arg instrumentation, the
327+
* pre-patch code would trip (currently not wired into this
328+
* test suite).
329+
* - On a stricter libc, the pre-patch code would crash; this
330+
* test therefore documents the expected contract and catches
331+
* any future regression on such a platform.
332+
*
333+
* The test feeds a config value containing bytes in the 0x80-0xFF
334+
* range and verifies the parser does not crash. Per the isgraph
335+
* contract these bytes are non-graph in the C locale, so the parser
336+
* will reject the key -- which is the CORRECT behaviour. The test
337+
* passes if the parser completes cleanly rather than crashing.
338+
*/
339+
static void test_config_file_high_bit_bytes_smoke(void)
340+
{
341+
/* Config with a high-bit byte (0xC3 0xA9 is UTF-8 "e-acute") in
342+
* both the key and the value. The parser\'s isgraph() check
343+
* terminates the key at the first non-graph byte, so this line
344+
* is rejected as a syntactic error -- that is fine; what we care
345+
* about is that the ctype call did not trip UB on the 0xC3 byte. */
346+
const char *cfgtext = "caf\xc3\xa9 = \"valu\xc3\xa9\"\n"
347+
"plain = \"ok\"\n";
348+
char *copy = strdup(cfgtext);
349+
config_file_t *cfg = config_file_new_from_string(copy, NULL);
350+
char *out = NULL;
351+
free(copy);
352+
353+
if (!cfg)
354+
{
355+
printf("[FAILED] parser refused to load config containing high-bit bytes\n");
356+
abort();
357+
}
358+
359+
/* Sanity: the plain key on the following line should still parse.
360+
* This confirms the parser recovered from the rejected key and
361+
* kept going rather than bailing on the whole file. */
362+
if (!config_get_string(cfg, "plain", &out) || !out || strcmp(out, "ok") != 0)
363+
{
364+
printf("[FAILED] high-bit byte line disrupted subsequent parsing: plain=%s\n",
365+
out ? out : "(null)");
366+
free(out);
367+
config_file_free(cfg);
368+
abort();
369+
}
370+
371+
free(out);
372+
config_file_free(cfg);
373+
printf("[SUCCESS] high-bit byte in config parsed without crash\n");
374+
}
375+
230376
int main(void)
231377
{
232378
test_config_file_parse_contains("foo = \"bar\"\n", "foo", "bar");
@@ -267,4 +413,7 @@ int main(void)
267413
test_config_get_int_accepts("-17", -17);
268414
test_config_get_int_accepts("0x10", 16); /* hex via base-0 detection */
269415
test_config_get_int_accepts("010", 8); /* octal via base-0 detection */
416+
417+
test_config_file_deinitialize_clears_fields();
418+
test_config_file_high_bit_bytes_smoke();
270419
}

0 commit comments

Comments
 (0)