[pull] master from libretro:master#941
Merged
pull[bot] merged 14 commits intoAlexandre1er:masterfrom Apr 19, 2026
Merged
Conversation
A base64 input string must have length divisible by 4. If a caller
passes e.g. "AB=" (len=3, trailing '='), the code computes pad=1,
*flen = 3*3/4 - 1 = 1, allocates 1 byte, then in the pad==1 branch
writes 2 bytes -- a 1-byte heap-buffer-overflow.
This is reachable from two network-facing call sites:
- network/netplay/netplay_frontend.c decodes a session id from a
netplay MITM server response.
- tasks/task_translation.c decodes image/sound data from a cloud
translation service response.
A malicious or MITM'd server can send a deliberately short malformed
base64 string to trigger the overflow in the RetroArch client.
Reject lengths that are not a multiple of 4, or below 4. This kills
both the pad==1 ("AB=") and pad==2 ("A==") short-input overflows as
well as other garbage that previously produced undefined behaviour.
Reproduced under AddressSanitizer with input "AB=":
ERROR: AddressSanitizer: heap-buffer-overflow
WRITE of size 1 at 0 bytes after 1-byte region
Fixes a potential RCE primitive (1-byte OOB write with an
attacker-controllable value, landing on allocator metadata).
file_decompressed() and file_decompressed_subdir() pass the raw archive-central-directory filename through fill_pathname_join_special() without any validation. A ZIP or 7z archive containing an entry named "../../../etc/foo", "/absolute", or "C:\\evil" extracts to an arbitrary filesystem location that the RetroArch process has write permission to. This is the classic Zip Slip vulnerability. The sibling helper file_archive_extract_cb() in libretro-common avoids it accidentally by calling path_basename() on the member name, but this decompress task path has no such filter. Three issues fixed: 1. Zip Slip proper -- names containing a ".." path segment, a leading separator, or a drive-letter prefix are now rejected before path construction. Both '/' and '\\' are treated as separators so we catch Windows-style traversal (..\\..\\foo) in archives opened on a POSIX host. 2. Empty-name crash -- strlen(name)==0 makes name[strlen-1] read name[SIZE_MAX] (OOB). Archives with zero-length filename entries would crash RetroArch. Rejected by archive_name_is_safe(). 3. Applies to both file_decompressed() and the subdir variant; each independently calls fill_pathname_join_special(path, target_dir, name, ...) and is independently reachable. The helper was unit-tested against 23 legitimate and adversarial inputs including "..foo" (not a traversal), "foo/..//bar" (double slash), "C:\\evil", "\\\\server\\share", and empty/NULL.
The POSIX non-symlink branch of path_resolve_realpath() copies a run
of leading '/' characters from the input into the stack buffer `tmp`
with no bound check:
for (p = s; *p == '/'; p++)
tmp[t++] = '/';
If the input starts with more than PATH_MAX_LENGTH (typically 4096)
slashes, `t` walks off the end of the 4 KiB stack array. This is
reachable from playlist load (playlist.c:1192), core-updater
metadata handling (core_updater_list.c:293,474), and the runloop
(runloop.c:586) -- all of which can be driven by adversarial file
content.
Reproduced on a scaled-down test under -fstack-protector-all:
*** stack smashing detected ***
The other per-segment copy in the same function (line 851) already
has a correct size check. This patch applies the same discipline
to the leading-slash loop: bail out if writing another byte would
exceed the buffer.
Behaviour on well-formed inputs is unchanged -- real paths have at
most a couple of leading slashes.
config_file_parse_line() tests whether a '#'-prefixed line is an
'include' or 'reference' directive using fixed-width memcmp:
bool include_found = !memcmp(comment, "include ", 8);
bool reference_found = !memcmp(comment, "reference ", 10);
`comment` is a pointer into a line-buffer produced by
filestream_getline(), which, after its post-read realloc at
file_stream.c:720-724, is sized to exactly strlen+1 bytes. A short
comment like "#hi" therefore has only 3 live bytes at `comment`
(h, i, '\0'), but memcmp reads 8 -- a 4+ byte heap-out-of-bounds
read.
GCC catches this at compile time:
warning: 'memcmp' specified bound 8 exceeds source size 3
[-Wstringop-overread]
Reproduced under AddressSanitizer on the real layout:
ERROR: AddressSanitizer: heap-buffer-overflow
READ of size 8 at 0 bytes after 4-byte region
Config files are user-controlled. Impact on non-ASan builds is
(a) nondeterministic directive misinterpretation -- uninitialized
heap bytes beyond the line buffer decide whether "#hi" gets treated
as an include, and (b) a heap info-leak surface usable as a
building block for more serious attacks.
Fix by bounding each memcmp on the comment's actual length. Note
that strncmp is not a valid alternative here: glibc and musl both
implement strncmp with word-sized loads that read past the NUL,
which ASan still flags. An explicit length gate is the correct
pattern.
config_get_int(), config_get_uint(), config_get_uint64() and
config_get_hex() all used this pattern:
errno = 0;
val = strtol(entry->value, NULL, 0);
if (errno == 0) {
*in = val;
return true;
}
strtol returns 0 (without setting errno) when handed a string that
has no leading digits at all -- so a config line like
width = abc
silently produced width = 0 and the getter reported success. The
user sees the setting "accepted" with a bogus value and no way to
tell anything went wrong. config_get_size_t() in the same file
already used the correct pattern; this patch applies it to the
other four.
Each fixed getter now:
- captures the end pointer from strtol/strtoul/strtoull
- rejects if errno was set (overflow)
- rejects if zero digits were consumed (end == entry->value)
- rejects if trailing garbage remains (*end != '\0')
- for config_get_int / config_get_uint / config_get_hex, also
rejects values outside the destination type's range (strtol
on 64-bit systems returns a 64-bit long that must fit in int)
Behavioural change: values that previously silently became zero
now return false and leave *in untouched. This matches the
documented "@return true if found, otherwise false" contract in
config_file.h -- the prior behaviour violated it by returning true
for not-a-number strings. Callers that relied on the bogus zero
were already silently broken; they now get explicit failure and
can fall back to a default.
Two related issues in the ZIP central-directory walker and filename
callback, both reachable from any ZIP load (ROM scan, content load,
extract-to-dir, etc):
1. zip_parse_file_iterate_step_internal() checked only that the
directory entry pointer fell inside the directory block, not that
a full 46-byte central-file-header plus the variable-length name,
extra and comment fields actually fit. A malformed archive with a
truncated trailing entry caused read_le() to read past the
allocation. Reproduced under AddressSanitizer with a 40-byte
directory:
ERROR: AddressSanitizer: heap-buffer-overflow
READ of size 1 at 2 bytes after 40-byte region
Fix by checking the available entry size (a) before the header
reads, and (b) before the memcpy of the filename, against the
declared namelength/extralength/commentlength.
2. zip_file_decompressed() computed "name[strlen(name) - 1]" without
guarding against an empty filename entry. When strlen == 0 the
index wraps to SIZE_MAX and the dereference reads far out of
bounds. A ZIP central directory is allowed to contain zero-length
name entries only if malformed, but producing such an archive is
trivial.
Both fixes reject the malformed entry and continue, so the parser
skips bad archives rather than crashing.
zip_parse_file_init() reads the central-directory size and offset from the ZIP footer and allocates "sizeof(zip_context_t) + directory_size" bytes for the context + directory. Three issues: 1. 32-bit allocation wraparound. On 32-bit hosts (3DS, Vita, PSP, Wii, Wii U) size_t is 32 bits, so a crafted directory_size near UINT32_MAX makes the "+ sizeof(zip_context_t)" wrap to a tiny value. The subsequent directory_end = directory + directory_size computation then points roughly 4 GiB past a small allocation, and every read from the directory is out of bounds. 2. Unchecked malloc return. Even on 64-bit, a 4 GiB directory_size request from a crafted ZIP can fail allocation; the next line dereferences the returned NULL unconditionally. 3. Incomplete sanity check. The existing check only verifies that directory_size and directory_offset individually fit within the archive. With offset = archive_size - 1 and size = archive_size, each passes but the combination claims the directory runs past EOF. The subsequent short read is caught, but only after the large bogus allocation is already made. Fix: reject the combined "offset + size > archive_size", reject sizes that would overflow the allocation, and check the malloc return.
New workflow that builds every sample under libretro-common/samples on each push/PR touching libretro-common/, runs the self-testing binaries, and fails CI if any test returns non-zero. Runs 8 self-testing samples: compat_fnmatch_test, snprintf, unbase64_test, archive_zip_test, config_file_test, path_resolve_realpath_test, nbio_test, rpng Build-only (need arguments or fixtures, not run): formats/xml, streams/rzip Skipped entirely: net/ -- Makefile references symbols without listing the corresponding sources (fill_pathname_resolve_relative, cpu_features_get_time_usec); remove from SKIP_DIRS once fixed. Dependencies: build-essential, zlib1g-dev. Runs under ubuntu-latest.
Three independent memory-safety / DoS fixes under libretro-common, their regression tests, and a CI workflow update. samples/net: fix undefined references in Makefile The net/ sample Makefile listed only a subset of the sources required to link its three binaries -- make all failed on strlcpy_retro__, string_list_*, fill_pathname_resolve_relative, cpu_features_get_time_usec, getnameinfo_retro. Add the missing libretro-common sources so http_test, http_parse_test and net_ifinfo build on a vanilla Linux host. file/archive_file_zstd: guard content_size truncation and overflow zstd_parse_file_iterate_step truncated the 64-bit content_size into a uint32_t without range check; a crafted .zst declaring content_size >= 2^32 truncated low-32 while ZSTD_decompress still
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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
See Commits and Changes for more details.
Created by
pull[bot] (v2.0.0-alpha.4)
Can you help keep this open source service alive? 💖 Please sponsor : )