Skip to content

Commit 78c52ab

Browse files
committed
libretro-common/string: add strlcpy_append helper and apply to database_info_build_query_enum
Add strlcpy_append, a bound-checked replacement for the unsafe idiom that's widespread in this codebase: _len += strlcpy(s + _len, src, len - _len); That looks like a self-bounding chain but isn't. strlcpy returns strlen(source) regardless of how much was actually written, so on truncation _len overshoots len and the next (len - _len) underflows to a huge size_t, driving the next strlcpy into an OOB-write or memcpy of attacker-influenced size. This is the same primitive that drove the shader- wildcard fix (commit bac6894). strlcpy_append takes (s, len, *pos, src), checks the truncation condition correctly, and on overflow: - leaves s NUL-terminated at len - 1 (strlcpy's contract) - clamps *pos to len - 1 so subsequent calls in a chain short-circuit (also returning -1) - returns -1 to the caller The clamp is what makes chained use safe with a single check at the end of the chain. Apply to database_info_build_query_enum (database_info.c). That function builds DB query strings of the shape {'KEY':"PATH"} (or numeric variants without the quotes, or {'developer':glob('*PATH*')}) via 22 switch cases, each of which was a hand-unrolled chain of s[++_len] = '...' character writes followed by a path strlcpy and another short s[++_len] = '...' chain to close the brace. None of those writes was bounded against len. A caller passing a buffer smaller than the prefix length got OOB-writes from the prefix; a caller whose buffer was just barely enough for prefix + path got the trailing s[_len]='"';s[++_len]='}' writes off the end (up to 3 bytes OOB). All callers in tree pass query[4096] from the menu displaylist, so practical buffers are large -- but the pattern is wrong and the path component is attacker- influenced when scanning content metadata. Side fix in DATABASE_QUERY_ENTRY_BBFC_RATING: pre-rewrite the case wrote s[++_len] = '"' after the path strlcpy instead of s[_len] = '"', leaving the strlcpy's NUL terminator embedded in the query. BBFC rating searches have been silently returning nothing since this code was written. Fixed while rewriting. Adds libretro-common/samples/string/strlcpy_append/ strlcpy_append_test as a regression test, registered in .github/workflows/Linux-libretro-common-samples.yml's RUN_TARGETS allowlist (which builds with ASan + UBSan default since the cdfs/chd commit). Eight cases covering the contract: basic append, chained append, exact-fit boundary, wide-margin truncation, chain short-circuiting after truncation, empty source, NULL/zero-length args, and *pos already at or past len. Test follows the existing libretro-common/samples pattern (heap-allocated destinations sized to exactly the legal capacity so any reintroduction of the unbounded chain is flagged by ASan). Verified to fire under ASan when the helper is replaced with the unsafe naive idiom: heap- buffer-overflow WRITE of size 2 at offset 26 of an 8-byte allocation in test_chain_short_circuits_after_truncation. With the safe helper all 8 cases pass clean. Verified database_info_build_query_enum produces identical output for all 21 enum values at len=4096 (the production caller's buffer size); also verified that at len=8 the function returns -1 with no OOB write, which is the behaviour the unsafe naive replacement would have failed at.
1 parent c41e955 commit 78c52ab

6 files changed

Lines changed: 495 additions & 399 deletions

File tree

.github/workflows/Linux-libretro-common-samples.yml

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -69,6 +69,7 @@ jobs:
6969
cdrom_cuesheet_overflow_test
7070
cdfs_dir_record_test
7171
chd_meta_overflow_test
72+
strlcpy_append_test
7273
http_parse_test
7374
rjson_test
7475
rtga_test

0 commit comments

Comments
 (0)